On Sun, Feb 1, 2015 at 4:23 PM, Benjamin Herrenschmidt <benh@au1.ibm.com> wrote:
I prefer having the test inside mm_fault_error(), even if that makes the patch a bit bigger, it keeps the logic in a single place. Untested patch:
I'm certainly ok with that, but I wanted to make the code that I wasn't going to compile (much less test) for various architectures be as simple and straightforward as possible. So feel free to send a patch that fixes it up to do it in a single place after testing it. Of course, what I *really* want would be to make a new "generic_mm_fault()" helper that would do all the normal stuff: - find_vma() - check permissions and ranges - call 'handle_mm_fault()' - do the proper error, retry and minor/major fault handling and then most architectures could just call that. Everybody has their own thing that they do *before* they get the mmap semaphore (x86 has at least kprobe, mmio tracing, vmalloc-fault, spurious prefetch faults due to AMD CPU bugs, in-atomic debugging irq re-enablement, etc etc). Those aren't even worth it trying to make into generic things. But just looking at the original patch, a _lot_ of the stuff around the actual handle_mm_fault() call is very very generic indeed, and I think it could be made into a generic helper function. For example, I not that long ago fixed the x86 handling of VM_FAULT_RETRY when there was a fatal signal pending and we were returning to kernel mode. The code would just return, "knowing" that the fatal signal would mean that rather than faulting again, the process would be killed. Except if the fault happened from kernel space, that wouldn't happen, and it really would just fault forever. I would not be surprised if somebody copied the old incorrect x86 case for VM_FAULT_RETRY and fatal signals, and it just never got fixed anywhere else. Doing all that in a generic helper routine (that then just returns a value saying "SIGBUS", "SIGSEGV" - we'd probably have to split it up into "SIGSEGV due to access error" vs "SIGSEGV due to map error" to make everybody happy, "out of memory" or "success" would have made it trivial to add the whole VM_FAULT_SIGSEGV, but it would also mean that everybody got fixes to the retry handling etc. Because right now there's a *lot* of basically duplicated code (although powerpc and s390 in particular have made some changes of their own). Anybody willing to see if they could encapsulate that part of the x86 code, and make it more widely useful? I say "x86 code", because that's the most tested one, and I think it gets the odd retry and error cases right (and minor/major fault counting etc), unlike some. Linus -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org