On Sun, Feb 1, 2015 at 4:23 PM, Benjamin Herrenschmidt <benh@xxxxxxxxxxx> 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 from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
