Re: [rfc] x86: optimise page fault path a little
From: Ingo Molnar
Date: Thu Nov 13 2008 - 02:41:38 EST
* Nick Piggin <npiggin@xxxxxxx> 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 ;)
> +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.
> +/* TODO: fixup for oom handling */
please fix todo ;-)
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)
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/