Re: [rfc] x86: optimise page fault path a little

[Nick Piggin <npiggin@suse.de>]

On Thu, Nov 13, 2008 at 08:41:09AM +0100, Ingo Molnar wrote:
> 
> * Nick Piggin <npiggin@suse.de> wrote:
> 
> > Hi,
> > 
> > I was just looking around the page fault code for any obvious 
> > performance improvements. I noticed do_page_fault is rather big, 
> > uses a lot of stack, and generates some branch mispredicts.
> > 
> > It's only about 1.1% on the profile of the workload I'm looking at, 
> > so my improvement is pretty close to in the noise, but I wonder if 
> > micro optimisations like the following would be welcome?
> 
> it's definitely welcome!
> 
> > This patch adds branch hints and moves error condition code out of 
> > line. It shrinks do_page_fault from 2410 bytes to 603 bytes, and 
> > from 352 to 64 bytes of stack. Total text size does grow by about 
> > 500 bytes due to the additional functions added.
> 
> Some small cleanliness nits:
> 
> > +/* TODO: match order of arguments */
> 
> please fix TODO ;)

OK, I actually already fixed it but forgot to remove that. Basically just
checking whether keeping regs,error_code,address always passed in the
same order reduces code size (there were one or two places already that
did different order for no good reason). Not surprisingly, it saved a
few bytes.

 
> > +static noinline void no_context(struct pt_regs *regs,
> > +			unsigned long error_code, unsigned long address)
> > +{
> > +	struct task_struct *tsk = current;
> > +#ifdef CONFIG_X86_64
> > +	unsigned long flags;
> > +#endif
> 
> we should just do this unconditionally on 32-bit too.
> 
> > +	/*
> > +	 * Oops. The kernel tried to access some bad page. We'll have to
> > +	 * terminate things with extreme prejudice.
> > +	 */
> > +#ifdef CONFIG_X86_32
> > +	bust_spinlocks(1);
> > +#else
> > +	flags = oops_begin();
> > +#endif
> 
> 32-bit should use oops_begin() too. Solves the previous comment as 
> well.
> 
> > +#ifdef CONFIG_X86_32
> > +	die("Oops", regs, error_code);
> > +	bust_spinlocks(0);
> > +	do_exit(SIGKILL);
> > +#else
> > +	if (__die("Oops", regs, error_code))
> > +		regs = NULL;
> > +	/* Executive summary in case the body of the oops scrolled away */
> > +	printk(KERN_EMERG "CR2: %016lx\n", address);
> > +	oops_end(flags, regs, SIGKILL);
> > +#endif
> 
> this difference seems unnecessary too - 32-bit should use oops_end() 
> too.

Probably all 3 good comments, but I didn't want to be tempted into changing
behaviour (modulo adding bugs). Easy to merge them up in a subsequent patch,
however...

 
> > +/* TODO: fixup for oom handling */
> 
> please fix todo ;-)

Oh... actually it's OK (we don't loop back again, but that's OK, the init
task will just retry the fault). The comment was actually just in relation
to my oom-killer page fault patches that are in -mm. I'm not proposing
this for merge just yet, but will wait for Andrew.

 
> this flow could be cleaned up further:
> 
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> [...]
> > +			bad_area(regs, error_code, address);
> > +			return;
> [...]
> > +		bad_area(regs, error_code, address);
> > +		return;
> 
> Any reason why that pattern shouldnt be changed to an appropriate goto 
> bad_area? (probably the same goes for the nosemaphore error paths too)

No... not really any reason. gcc effectively turns the code into a goto
anyway. Some days I can't make up my mind which one is better :)