Re: [patch] compress the stack layout of do_page_fault(), x86

Re: [patch] compress the stack layout of do_page_fault(), x86

[Andi Kleen]

Ingo Molnar <mingo@elte.hu> 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

Re: [patch] compress the stack layout of do_page_fault(), x86

[Arjan van de Ven]

On Sat, 2005-07-09 at 19:05 +0200, Andi Kleen wrote:
> Ingo Molnar <mingo@elte.hu> 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 ;)

Re: [patch] compress the stack layout of do_page_fault(), x86

[Andi Kleen]

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 <mingo@elte.hu> 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

Re: [patch] compress the stack layout of do_page_fault(), x86

[Arjan van de Ven]

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 <mingo@elte.hu> 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.

Re: [patch] compress the stack layout of do_page_fault(), x86

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> Ingo Molnar <mingo@elte.hu> 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

Re: [patch] compress the stack layout of do_page_fault(), x86

[Andi Kleen]

On Sat, Jul 09, 2005 at 09:42:20PM +0200, Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > Ingo Molnar <mingo@elte.hu> 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

Re: [patch] compress the stack layout of do_page_fault(), x86

[Ingo Molnar]

* Andi Kleen <ak@suse.de> wrote:

> On Sat, Jul 09, 2005 at 09:42:20PM +0200, Ingo Molnar wrote:
> > 
> > * Andi Kleen <ak@suse.de> wrote:
> > 
> > > Ingo Molnar <mingo@elte.hu> 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 <mingo@elte.hu>

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

[patch] compress the stack layout of do_page_fault(), x86

[Ingo Molnar]

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.

	Ingo

Signed-off-by: Ingo Molnar <mingo@elte.hu>

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 = SIGSEGV;
+	info.si_errno = 0;
+	info.si_code = si_code;
+	info.si_addr = (void __user *)address;
+	force_sig_info(SIGSEGV, &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:

Re: [patch] compress the stack layout of do_page_fault(), x86

[Ingo Molnar]

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 <mingo@elte.hu>

argument bugfix from "Guillaume C." <guichaz@gmail.com>

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:

Re: [patch] compress the stack layout of do_page_fault(), x86

[randy_dunlap]

On Sat, 9 Jul 2005 17:29:24 +0200 Ingo Molnar wrote:

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

Hi Ingo-

Just to clarify this, "boot tested" does not mean that this
changed code was tested, right?
or do you have a test case that was tested?

Thanks,
---
~Randy

Re: [patch] compress the stack layout of do_page_fault(), x86

[Ingo Molnar]

* randy_dunlap <rdunlap@xenotime.net> wrote:

> Just to clarify this, "boot tested" does not mean that this changed 
> code was tested, right? or do you have a test case that was tested?

i tested segfaults, but not bus-faults. The bug would have resulted in 
sigsegv being delivered not a sigbus.

	Ingo

Re: [patch] compress the stack layout of do_page_fault(), x86

[Chris Wedgwood]

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?