Re: [patch] compress the stack layout of do_page_fault(), x86
* Andi Kleen <[EMAIL PROTECTED]> wrote: > On Sat, Jul 09, 2005 at 09:42:20PM +0200, Ingo Molnar wrote: > > > > * Andi Kleen <[EMAIL PROTECTED]> wrote: > > > > > Ingo Molnar <[EMAIL PROTECTED]> writes: > > > > > > > > +static void force_sig_info_fault(int si_signo, int si_code, > > > > +unsigned long address, struct > > > > task_struct *tsk) > > > > > > This won't work with a unit-at-a-time compiler which happily inlines > > > everything static with only a single caller. Use noinline > > > > this function has two callers. > > Even then it's still better to mark it noinline, otherwise a different > inlining algorithm in a new compiler might do the wrong thing. It's > also useful documentation. ok, agreed. Delta patch below. Ingo make force_sig_info_fault() noinline, suggested by Andi Kleen. Signed-off-by: Ingo Molnar <[EMAIL PROTECTED]> --- linux/arch/i386/mm/fault.c.orig +++ linux/arch/i386/mm/fault.c @@ -199,8 +199,8 @@ static inline int is_prefetch(struct pt_ return 0; } -static void force_sig_info_fault(int si_signo, int si_code, -unsigned long address, struct task_struct *tsk) +static noinline void force_sig_info_fault(int si_signo, int si_code, + unsigned long address, struct task_struct *tsk) { siginfo_t info; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] compress the stack layout of do_page_fault(), x86
On Sat, Jul 09, 2005 at 09:42:20PM +0200, Ingo Molnar wrote: > > * Andi Kleen <[EMAIL PROTECTED]> wrote: > > > Ingo Molnar <[EMAIL PROTECTED]> writes: > > > > > > +static void force_sig_info_fault(int si_signo, int si_code, > > > + unsigned long address, struct task_struct *tsk) > > > > This won't work with a unit-at-a-time compiler which happily inlines > > everything static with only a single caller. Use noinline > > this function has two callers. Even then it's still better to mark it noinline, otherwise a different inlining algorithm in a new compiler might do the wrong thing. It's also useful documentation. -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] compress the stack layout of do_page_fault(), x86
On Sat, Jul 09, 2005 at 04:41:16PM +0200, Ingo Molnar wrote: > this patch pushes the creation of a rare signal frame (SIGBUS or > SIGSEGV) into a separate function, thus saving stackspace in the > main do_page_fault() stackframe. The effect is 132 bytes less of > stack used by the typical do_page_fault() invocation - resulting in > a denser cache-layout. does the benefit actually show up anywhere? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] compress the stack layout of do_page_fault(), x86
On Sat, 2005-07-09 at 21:15 +0200, Andi Kleen wrote: > On Sat, Jul 09, 2005 at 07:31:04PM +0200, Arjan van de Ven wrote: > > On Sat, 2005-07-09 at 19:05 +0200, Andi Kleen wrote: > > > Ingo Molnar <[EMAIL PROTECTED]> writes: > > > > > > > > +static void force_sig_info_fault(int si_signo, int si_code, > > > > +unsigned long address, struct > > > > task_struct *tsk) > > > > > > This won't work with a unit-at-a-time compiler which happily > > > inlines everything static with only a single caller. Use noinline > > > > but the x86 kernel is -fno-unit-at-a-time for stack reasons ;) > > The gcc people are making noises of removing that. So eventually > it will need to go. the gcc people also fixed the stack usage on inlining (at least this specific class) in CVS HEAD so the "problem" is a lot smaller whenever they make it go away. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] compress the stack layout of do_page_fault(), x86
* Andi Kleen <[EMAIL PROTECTED]> wrote: > Ingo Molnar <[EMAIL PROTECTED]> writes: > > > > +static void force_sig_info_fault(int si_signo, int si_code, > > +unsigned long address, struct task_struct *tsk) > > This won't work with a unit-at-a-time compiler which happily inlines > everything static with only a single caller. Use noinline this function has two callers. Ingo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] compress the stack layout of do_page_fault(), x86
On Sat, Jul 09, 2005 at 07:31:04PM +0200, Arjan van de Ven wrote: > On Sat, 2005-07-09 at 19:05 +0200, Andi Kleen wrote: > > Ingo Molnar <[EMAIL PROTECTED]> writes: > > > > > > +static void force_sig_info_fault(int si_signo, int si_code, > > > + unsigned long address, struct task_struct *tsk) > > > > This won't work with a unit-at-a-time compiler which happily > > inlines everything static with only a single caller. Use noinline > > but the x86 kernel is -fno-unit-at-a-time for stack reasons ;) The gcc people are making noises of removing that. So eventually it will need to go. Also some kernels are compiled with that option removed and it works just fine. -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] compress the stack layout of do_page_fault(), x86
On Sat, 2005-07-09 at 19:05 +0200, Andi Kleen wrote: > Ingo Molnar <[EMAIL PROTECTED]> writes: > > > > +static void force_sig_info_fault(int si_signo, int si_code, > > +unsigned long address, struct task_struct *tsk) > > This won't work with a unit-at-a-time compiler which happily > inlines everything static with only a single caller. Use noinline but the x86 kernel is -fno-unit-at-a-time for stack reasons ;) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] compress the stack layout of do_page_fault(), x86
Ingo Molnar <[EMAIL PROTECTED]> writes: > > +static void force_sig_info_fault(int si_signo, int si_code, > + unsigned long address, struct task_struct *tsk) This won't work with a unit-at-a-time compiler which happily inlines everything static with only a single caller. Use noinline -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch] compress the stack layout of do_page_fault(), x86
Guillaume noticed a bug in the patch, si_signo should be used instead of SIGSEGV. (doh) New patch below. Ingo - this patch pushes the creation of a rare signal frame (SIGBUS or SIGSEGV) into a separate function, thus saving stackspace in the main do_page_fault() stackframe. The effect is 132 bytes less of stack used by the typical do_page_fault() invocation - resulting in a denser cache-layout. (another minor effect is that in case of kernel crashes that come from a pagefault, we add less space to the already existing frame, giving the crash functions a slightly higher chance to do their stuff without overflowing the stack.) (the changes also result in slightly cleaner code.) build and boot tested. Signed-off-by: Ingo Molnar <[EMAIL PROTECTED]> argument bugfix from "Guillaume C." <[EMAIL PROTECTED]> Index: linux/arch/i386/mm/fault.c === --- linux.orig/arch/i386/mm/fault.c +++ linux/arch/i386/mm/fault.c @@ -201,6 +201,18 @@ static inline int is_prefetch(struct pt_ return 0; } +static void force_sig_info_fault(int si_signo, int si_code, +unsigned long address, struct task_struct *tsk) +{ + siginfo_t info; + + info.si_signo = si_signo; + info.si_errno = 0; + info.si_code = si_code; + info.si_addr = (void __user *)address; + force_sig_info(si_signo, &info, tsk); +} + fastcall void do_invalid_op(struct pt_regs *, unsigned long); /* @@ -220,9 +232,8 @@ fastcall notrace void do_page_fault(stru struct vm_area_struct * vma; unsigned long address; unsigned long page; - int write; - siginfo_t info; - + int write, si_code; + /* get the address */ __asm__("movl %%cr2,%0":"=r" (address)); trace_special(regs->eip, error_code, address); @@ -236,7 +247,7 @@ fastcall notrace void do_page_fault(stru tsk = current; - info.si_code = SEGV_MAPERR; + si_code = SEGV_MAPERR; /* * We fault-in kernel-space virtual memory on-demand. The @@ -316,7 +327,7 @@ fastcall notrace void do_page_fault(stru * we can handle it.. */ good_area: - info.si_code = SEGV_ACCERR; + si_code = SEGV_ACCERR; write = 0; switch (error_code & 3) { default:/* 3: write, present */ @@ -390,11 +401,7 @@ bad_area_nosemaphore: /* Kernel addresses are always protection faults */ tsk->thread.error_code = error_code | (address >= TASK_SIZE); tsk->thread.trap_no = 14; - info.si_signo = SIGSEGV; - info.si_errno = 0; - /* info.si_code has been set above */ - info.si_addr = (void __user *)address; - force_sig_info(SIGSEGV, &info, tsk); + force_sig_info_fault(SIGSEGV, si_code, address, tsk); return; } @@ -500,11 +507,7 @@ do_sigbus: tsk->thread.cr2 = address; tsk->thread.error_code = error_code; tsk->thread.trap_no = 14; - info.si_signo = SIGBUS; - info.si_errno = 0; - info.si_code = BUS_ADRERR; - info.si_addr = (void __user *)address; - force_sig_info(SIGBUS, &info, tsk); + force_sig_info_fault(SIGBUS, BUS_ADRERR, address, tsk); return; vmalloc_fault: - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/