Re: [patch] x86: optimise page fault entry

[Ingo Molnar]

* Nick Piggin <npiggin@xxxxxxx> wrote:

> On Tue, Jan 20, 2009 at 11:09:46AM +0100, Ingo Molnar wrote:
> > 
> > * Nick Piggin <npiggin@xxxxxxx> 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.

i think it could be structured in a way so that every step is either 
small, or yields no change in the vmlinux binary. The former is 
reviewable, the latter is machine-checkable.

At least that's how i do such changes and i have yet to find a code 
transformation where such techniques cannot be used to minimize regression 
risks.

> > 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;

applied, thanks Nick.

Lets try it with the current large patch once more ... but if there's one 
more regression then i guess we need to do the splitup, to be on the safe 
side.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/