Re: [patch] x86: optimise page fault entry

Nick Piggin
2009-01-20T12:07:44Z

On Tue, Jan 20, 2009 at 11:09:46AM +0100, Ingo Molnar wrote:
>
> * Nick Piggin <npiggin@suse.de> wrote:
>
> > Hi,
> >
> > Sorry for the delay with this. The kernel ended up unbootable for me
> > when I last dusted off the patch, so I couldn't test it and then
> > promptly got sidetracked with other things.
> >
> > Anyway, this one is tested with a boot, some basic segfault sigbus etc
> > tests, and passes various of the mmap and mprotect etc. ltp tests.
> >
> > Ingo, would you merge this into the x86 tree, please? (unless Linus has
> > any objections to this version)
>
> -tip testing found a 32-bit boot regression, caused by this patch. The
> bootup hangs early, during the WP write-test check:
>
> [ 0.004000] .data : 0xc0691f05 - 0xc09c746c (3285 kB)
> [ 0.004000] .text : 0xc0100000 - 0xc0691f05 (5703 kB)
> [ 0.004000] Checking if this processor honours the WP bit even in supervisor mode...
>
> i've excluded x86/mm from tip/master for now, you can find the broken tree
> in the tip/tip.x86.mm.broken [v2.6.29-rc2-1069-g583f1b9] branch that i
> just pushed out:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git tip.x86.mm.broken

Gah, I knew I should have tested with 32-bit. Sorry, I had actually tested
it at some point, so I must have dropped this hunk along the way :(

> Also, a patch structure sidenote, the diffstat is rather large:
>
> arch/x86/mm/fault.c | 436 ++++++++++++++++++++++++++++++---------------------
> 1 files changed, 255 insertions(+), 181 deletions(-)
>
> this shuffles 300 lines of highly critical x86 code around - which makes
> me nervous. A finegrained, bisectable series would be far more debuggable.
> Had we such a lineup i could have auto-bisected it for you already - while
> now you have to see which bit of the ~500 lines of code flux broke the
> 32-bit WP test.
>
> This hang might be easy to find and fix (the WP detect logic is simple),
> but other failure modes might be less debuggable and this codepath deals
> with a lot of obscure details like CPU errata. So it would be really nice
> to have a finegrained splitup of this patch.

I guess breaking out the shuffling of parameters (where this bug lies),
breaking out functions from do_page_fault, and added branch annotations
could be done.... that would still leave a fair hunk in the breakout
patch, which I didn't see a really pleasing way to split out.

> Three separate testsystems triggered this hang so it should be readily
> reproducible.

Yes, thanks,
Nick

---
arch/x86/mm/fault.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/arch/x86/mm/fault.c
===================================================================
--- linux-2.6.orig/arch/x86/mm/fault.c
+++ linux-2.6/arch/x86/mm/fault.c
@@ -828,7 +828,7 @@ void __kprobes do_page_fault(struct pt_r
return;

/* Can handle a stale RO->RW TLB */
- if (spurious_fault(address, error_code))
+ if (spurious_fault(error_code, address))
return;

/*