[PATCH] powerpc: save trap number in bad_stack

[PATCH] powerpc: save trap number in bad_stack

[Olof Johansson]

Save the trap number in the case of getting a bad stack in an exception
handler. It is sometimes useful to know what exception it was that caused
this to happen. Without this, no trap is reported.


Signed-off-by: Olof Johansson <olof@lixom.net>

Index: linux-2.6/arch/powerpc/kernel/asm-offsets.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/asm-offsets.c
+++ linux-2.6/arch/powerpc/kernel/asm-offsets.c
@@ -139,6 +139,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
Index: linux-2.6/include/asm-powerpc/paca.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/paca.h
+++ linux-2.6/include/asm-powerpc/paca.h
@@ -81,6 +81,8 @@ struct paca_struct {
 	u64 exslb[10];		/* used for SLB/segment table misses
  				 * on the linear mapping */
 
+	u64 trap_save;			/* Used when bad stack is encountered */
+
 	mm_context_t context;
 	u16 vmalloc_sllp;
 	u16 slb_cache[SLB_CACHE_ENTRIES];
Index: linux-2.6/arch/powerpc/kernel/head_64.S
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/head_64.S
+++ linux-2.6/arch/powerpc/kernel/head_64.S
@@ -278,8 +278,12 @@ exception_marker:
 	beq-	1f;							   \
 	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
 1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
-	bge-	cr1,bad_stack;		/* abort if it is		*/ \
-	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
+	bge-	cr1,2f;			/* abort if it is		*/ \
+	b	3f;							   \
+2:	li	r1,(n);			/* will be reloaded later	*/ \
+	std	r1,PACA_TRAP_SAVE(r13);					   \
+	b	bad_stack;						   \
+3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
 	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
 	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
 	std	r10,0(r1);		/* make stack chain pointer	*/ \
@@ -940,6 +944,8 @@ bad_stack:
 	SAVE_2GPRS(7,r1)
 	SAVE_10GPRS(12,r1)
 	SAVE_10GPRS(22,r1)
+	ld	r12,PACA_TRAP_SAVE(r13)
+	std	r12,_TRAP(r1)
 	addi	r11,r1,INT_FRAME_SIZE
 	std	r11,0(r1)
 	li	r12,0

Re: [PATCH] powerpc: save trap number in bad_stack

[Benjamin Herrenschmidt]

On Mon, 2007-04-16 at 01:23 -0500, Olof Johansson wrote:
> Save the trap number in the case of getting a bad stack in an exception
> handler. It is sometimes useful to know what exception it was that caused
> this to happen. Without this, no trap is reported.
> 
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> 
> Index: linux-2.6/arch/powerpc/kernel/asm-offsets.c
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/kernel/asm-offsets.c
> +++ linux-2.6/arch/powerpc/kernel/asm-offsets.c
> @@ -139,6 +139,7 @@ int main(void)
>  	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
>  	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
>  	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
> +	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
>  
>  	DEFINE(SLBSHADOW_STACKVSID,
>  	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
> Index: linux-2.6/include/asm-powerpc/paca.h
> ===================================================================
> --- linux-2.6.orig/include/asm-powerpc/paca.h
> +++ linux-2.6/include/asm-powerpc/paca.h
> @@ -81,6 +81,8 @@ struct paca_struct {
>  	u64 exslb[10];		/* used for SLB/segment table misses
>   				 * on the linear mapping */
>  
> +	u64 trap_save;			/* Used when bad stack is encountered */

Can you put it elsewhere in the PACA (around less frequently used
bits) ? This is a pretty hot area of the PACA...

Ben.

Re: [PATCH] powerpc: save trap number in bad_stack

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]

On Fri, 2007-04-20 at 09:17 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2007-04-16 at 01:23 -0500, Olof Johansson wrote:
> > Save the trap number in the case of getting a bad stack in an exception
> > handler. It is sometimes useful to know what exception it was that caused
> > this to happen. Without this, no trap is reported.
> > 
> > 
> > Signed-off-by: Olof Johansson <olof@lixom.net>
> > 
> > Index: linux-2.6/arch/powerpc/kernel/asm-offsets.c
> > ===================================================================
> > --- linux-2.6.orig/arch/powerpc/kernel/asm-offsets.c
> > +++ linux-2.6/arch/powerpc/kernel/asm-offsets.c
> > @@ -139,6 +139,7 @@ int main(void)
> >  	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
> >  	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
> >  	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
> > +	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
> >  
> >  	DEFINE(SLBSHADOW_STACKVSID,
> >  	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
> > Index: linux-2.6/include/asm-powerpc/paca.h
> > ===================================================================
> > --- linux-2.6.orig/include/asm-powerpc/paca.h
> > +++ linux-2.6/include/asm-powerpc/paca.h
> > @@ -81,6 +81,8 @@ struct paca_struct {
> >  	u64 exslb[10];		/* used for SLB/segment table misses
> >   				 * on the linear mapping */
> >  
> > +	u64 trap_save;			/* Used when bad stack is encountered */
> 
> Can you put it elsewhere in the PACA (around less frequently used
> bits) ? This is a pretty hot area of the PACA...

It's only set in the bad_stack case though.

You should put it after cpu_start, there's a bit hole there, and
trap_save is readonly during normal operation, so it shouldn't be a
performance hit.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] powerpc: save trap number in bad_stack

[Segher Boessenkool]

>>> +	u64 trap_save;			/* Used when bad stack is encountered */
>>
>> Can you put it elsewhere in the PACA (around less frequently used
>> bits) ? This is a pretty hot area of the PACA...
>
> It's only set in the bad_stack case though.
>
> You should put it after cpu_start, there's a bit hole there, and
> trap_save is readonly during normal operation, so it shouldn't be a
> performance hit.

That's not a performance hit compared to the current
state of the code, sure; but that "hot slot" could
be used for a hotter variable, instead.


Segher

Re: [PATCH] powerpc: save trap number in bad_stack

[Olof Johansson]

On Fri, Apr 20, 2007 at 09:17:58AM +1000, Benjamin Herrenschmidt wrote:
> Can you put it elsewhere in the PACA (around less frequently used
> bits) ? This is a pretty hot area of the PACA...

Good point, I didn't think much about layout when I added it. There's
actually room right before if it's reduced in size (which is easy to do).
See patch posted separately.


-Olof

[PATCH] [v2] powerpc: save trap number in bad_stack

[Olof Johansson]

Save the trap number in the case of getting a bad stack in an exception
handler. It is sometimes useful to know what exception it was that caused
this to happen. Without this, no trap is reported.

Signed-off-by: Olof Johansson <olof@lixom.net>

---

Changes since last time: Reduce to a u16 and move it.

Index: powerpc/arch/powerpc/kernel/asm-offsets.c
===================================================================
--- powerpc.orig/arch/powerpc/kernel/asm-offsets.c
+++ powerpc/arch/powerpc/kernel/asm-offsets.c
@@ -139,6 +139,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
Index: powerpc/include/asm-powerpc/paca.h
===================================================================
--- powerpc.orig/include/asm-powerpc/paca.h
+++ powerpc/include/asm-powerpc/paca.h
@@ -68,6 +68,7 @@ struct paca_struct {
 	void *emergency_sp;		/* pointer to emergency stack */
 	u64 data_offset;		/* per cpu data offset */
 	s16 hw_cpu_id;			/* Physical processor number */
+	u16 trap_save;			/* Used when bad stack is encountered */
 	u8 cpu_start;			/* At startup, processor spins until */
 					/* this becomes non-zero. */
 	struct slb_shadow *slb_shadow_ptr;
Index: powerpc/arch/powerpc/kernel/head_64.S
===================================================================
--- powerpc.orig/arch/powerpc/kernel/head_64.S
+++ powerpc/arch/powerpc/kernel/head_64.S
@@ -278,8 +278,12 @@ exception_marker:
 	beq-	1f;							   \
 	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
 1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
-	bge-	cr1,bad_stack;		/* abort if it is		*/ \
-	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
+	bge-	cr1,2f;			/* abort if it is		*/ \
+	b	3f;							   \
+2:	li	r1,(n);			/* will be reloaded later	*/ \
+	stw	r1,PACA_TRAP_SAVE(r13);					   \
+	b	bad_stack;						   \
+3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
 	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
 	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
 	std	r10,0(r1);		/* make stack chain pointer	*/ \
@@ -940,6 +944,8 @@ bad_stack:
 	SAVE_2GPRS(7,r1)
 	SAVE_10GPRS(12,r1)
 	SAVE_10GPRS(22,r1)
+	lwz	r12,PACA_TRAP_SAVE(r13)
+	std	r12,_TRAP(r1)
 	addi	r11,r1,INT_FRAME_SIZE
 	std	r11,0(r1)
 	li	r12,0

Re: [PATCH] [v2] powerpc: save trap number in bad_stack

[Gabriel Paubert]

On Sun, Apr 22, 2007 at 06:38:21PM -0500, Olof Johansson wrote:
> Save the trap number in the case of getting a bad stack in an exception
> handler. It is sometimes useful to know what exception it was that caused
> this to happen. Without this, no trap is reported.
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> 
> ---
> 
> Changes since last time: Reduce to a u16 and move it.
> 
> Index: powerpc/arch/powerpc/kernel/asm-offsets.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/kernel/asm-offsets.c
> +++ powerpc/arch/powerpc/kernel/asm-offsets.c
> @@ -139,6 +139,7 @@ int main(void)
>  	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
>  	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
>  	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
> +	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
>  
>  	DEFINE(SLBSHADOW_STACKVSID,
>  	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
> Index: powerpc/include/asm-powerpc/paca.h
> ===================================================================
> --- powerpc.orig/include/asm-powerpc/paca.h
> +++ powerpc/include/asm-powerpc/paca.h
> @@ -68,6 +68,7 @@ struct paca_struct {
>  	void *emergency_sp;		/* pointer to emergency stack */
>  	u64 data_offset;		/* per cpu data offset */
>  	s16 hw_cpu_id;			/* Physical processor number */
> +	u16 trap_save;			/* Used when bad stack is encountered */
>  	u8 cpu_start;			/* At startup, processor spins until */
>  					/* this becomes non-zero. */
>  	struct slb_shadow *slb_shadow_ptr;
> Index: powerpc/arch/powerpc/kernel/head_64.S
> ===================================================================
> --- powerpc.orig/arch/powerpc/kernel/head_64.S
> +++ powerpc/arch/powerpc/kernel/head_64.S
> @@ -278,8 +278,12 @@ exception_marker:
>  	beq-	1f;							   \
>  	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
>  1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
> -	bge-	cr1,bad_stack;		/* abort if it is		*/ \
> -	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
> +	bge-	cr1,2f;			/* abort if it is		*/ \
> +	b	3f;							   \
> +2:	li	r1,(n);			/* will be reloaded later	*/ \
> +	stw	r1,PACA_TRAP_SAVE(r13);					   \

stW to an u16? stH seems safer!

> +	b	bad_stack;						   \
> +3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
>  	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
>  	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
>  	std	r10,0(r1);		/* make stack chain pointer	*/ \
> @@ -940,6 +944,8 @@ bad_stack:
>  	SAVE_2GPRS(7,r1)
>  	SAVE_10GPRS(12,r1)
>  	SAVE_10GPRS(22,r1)
> +	lwz	r12,PACA_TRAP_SAVE(r13)

Same, well actually lwz->lhz.

> +	std	r12,_TRAP(r1)
>  	addi	r11,r1,INT_FRAME_SIZE
>  	std	r11,0(r1)
>  	li	r12,0


	Gabriel

Re: [PATCH] [v2] powerpc: save trap number in bad_stack

[Olof Johansson]

On Mon, Apr 23, 2007 at 11:29:19AM +0200, Gabriel Paubert wrote:

> stW to an u16? stH seems safer!

Yes, thanks. my bad.

-Olof

[PATCH] [v3] powerpc: save trap number in bad_stack

[Olof Johansson]

Save the trap number in the case of getting a bad stack in an exception
handler. It is sometimes useful to know what exception it was that caused
this to happen. Without this, no trap is reported.


Signed-off-by: Olof Johansson <olof@lixom.net>


Index: powerpc/arch/powerpc/kernel/asm-offsets.c
===================================================================
--- powerpc.orig/arch/powerpc/kernel/asm-offsets.c
+++ powerpc/arch/powerpc/kernel/asm-offsets.c
@@ -139,6 +139,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
Index: powerpc/include/asm-powerpc/paca.h
===================================================================
--- powerpc.orig/include/asm-powerpc/paca.h
+++ powerpc/include/asm-powerpc/paca.h
@@ -68,6 +68,7 @@ struct paca_struct {
 	void *emergency_sp;		/* pointer to emergency stack */
 	u64 data_offset;		/* per cpu data offset */
 	s16 hw_cpu_id;			/* Physical processor number */
+	u16 trap_save;			/* Used when bad stack is encountered */
 	u8 cpu_start;			/* At startup, processor spins until */
 					/* this becomes non-zero. */
 	struct slb_shadow *slb_shadow_ptr;
Index: powerpc/arch/powerpc/kernel/head_64.S
===================================================================
--- powerpc.orig/arch/powerpc/kernel/head_64.S
+++ powerpc/arch/powerpc/kernel/head_64.S
@@ -278,8 +278,12 @@ exception_marker:
 	beq-	1f;							   \
 	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
 1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
-	bge-	cr1,bad_stack;		/* abort if it is		*/ \
-	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
+	bge-	cr1,2f;			/* abort if it is		*/ \
+	b	3f;							   \
+2:	li	r1,(n);			/* will be reloaded later	*/ \
+	sth	r1,PACA_TRAP_SAVE(r13);					   \
+	b	bad_stack;						   \
+3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
 	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
 	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
 	std	r10,0(r1);		/* make stack chain pointer	*/ \
@@ -940,6 +944,8 @@ bad_stack:
 	SAVE_2GPRS(7,r1)
 	SAVE_10GPRS(12,r1)
 	SAVE_10GPRS(22,r1)
+	lhz	r12,PACA_TRAP_SAVE(r13)
+	std	r12,_TRAP(r1)
 	addi	r11,r1,INT_FRAME_SIZE
 	std	r11,0(r1)
 	li	r12,0

[PATCH] [v4] powerpc: save trap number in bad_stack

[Olof Johansson]

Save the trap number in the case of getting a bad stack in an exception
handler. It is sometimes useful to know what exception it was that caused
this to happen. Without this, no trap is reported.


Signed-off-by: Olof Johansson <olof@lixom.net>

---

Moved to a hole in a later cacheline instead of in the read-only line.


Index: linux-2.6/arch/powerpc/kernel/asm-offsets.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/asm-offsets.c
+++ linux-2.6/arch/powerpc/kernel/asm-offsets.c
@@ -140,6 +140,7 @@ int main(void)
 	DEFINE(PACA_SYSTEM_TIME, offsetof(struct paca_struct, system_time));
 	DEFINE(PACA_SLBSHADOWPTR, offsetof(struct paca_struct, slb_shadow_ptr));
 	DEFINE(PACA_DATA_OFFSET, offsetof(struct paca_struct, data_offset));
+	DEFINE(PACA_TRAP_SAVE, offsetof(struct paca_struct, trap_save));
 
 	DEFINE(SLBSHADOW_STACKVSID,
 	       offsetof(struct slb_shadow, save_area[SLB_NUM_BOLTED - 1].vsid));
Index: linux-2.6/include/asm-powerpc/paca.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/paca.h
+++ linux-2.6/include/asm-powerpc/paca.h
@@ -93,6 +93,7 @@ struct paca_struct {
 	u64 stab_rr;			/* stab/slb round-robin counter */
 	u64 saved_r1;			/* r1 save for RTAS calls */
 	u64 saved_msr;			/* MSR saved here by enter_rtas */
+	u16 trap_save;			/* Used when bad stack is encountered */
 	u8 soft_enabled;		/* irq soft-enable flag */
 	u8 hard_enabled;		/* set if irqs are enabled in MSR */
 	u8 io_sync;			/* writel() needs spin_unlock sync */
Index: linux-2.6/arch/powerpc/kernel/head_64.S
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/head_64.S
+++ linux-2.6/arch/powerpc/kernel/head_64.S
@@ -278,8 +278,12 @@ exception_marker:
 	beq-	1f;							   \
 	ld	r1,PACAKSAVE(r13);	/* kernel stack to use		*/ \
 1:	cmpdi	cr1,r1,0;		/* check if r1 is in userspace	*/ \
-	bge-	cr1,bad_stack;		/* abort if it is		*/ \
-	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
+	bge-	cr1,2f;			/* abort if it is		*/ \
+	b	3f;							   \
+2:	li	r1,(n);			/* will be reloaded later	*/ \
+	sth	r1,PACA_TRAP_SAVE(r13);					   \
+	b	bad_stack;						   \
+3:	std	r9,_CCR(r1);		/* save CR in stackframe	*/ \
 	std	r11,_NIP(r1);		/* save SRR0 in stackframe	*/ \
 	std	r12,_MSR(r1);		/* save SRR1 in stackframe	*/ \
 	std	r10,0(r1);		/* make stack chain pointer	*/ \
@@ -940,6 +944,8 @@ bad_stack:
 	SAVE_2GPRS(7,r1)
 	SAVE_10GPRS(12,r1)
 	SAVE_10GPRS(22,r1)
+	lhz	r12,PACA_TRAP_SAVE(r13)
+	std	r12,_TRAP(r1)
 	addi	r11,r1,INT_FRAME_SIZE
 	std	r11,0(r1)
 	li	r12,0

Re: [PATCH] [v4] powerpc: save trap number in bad_stack

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

On Mon, 23 Apr 2007 10:11:55 -0500 olof@lixom.net (Olof Johansson) wrote:
>
> Save the trap number in the case of getting a bad stack in an exception
> handler. It is sometimes useful to know what exception it was that caused
> this to happen. Without this, no trap is reported.
>
>
> Signed-off-by: Olof Johansson <olof@lixom.net>

I'm happy this time :-)

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [v2] powerpc: save trap number in bad_stack

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]

On Sun, 22 Apr 2007 18:38:21 -0500 olof@lixom.net (Olof Johansson) wrote:
>
> --- powerpc.orig/include/asm-powerpc/paca.h
> +++ powerpc/include/asm-powerpc/paca.h
> @@ -68,6 +68,7 @@ struct paca_struct {
>  	void *emergency_sp;		/* pointer to emergency stack */
>  	u64 data_offset;		/* per cpu data offset */
>  	s16 hw_cpu_id;			/* Physical processor number */
> +	u16 trap_save;			/* Used when bad stack is encountered */

Please read the comment at the top of the struct:

        /*
         * Because hw_cpu_id, unlike other paca fields, is accessed
         * routinely from other CPUs (from the IRQ code), we stick to
         * read-only (after boot) fields in the first cacheline to
         * avoid cacheline bouncing.
         */

So please move this out of the first cache line.

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [v2] powerpc: save trap number in bad_stack

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On Mon, 2007-04-23 at 23:22 +1000, Stephen Rothwell wrote:
> On Sun, 22 Apr 2007 18:38:21 -0500 olof@lixom.net (Olof Johansson) wrote:
> >
> > --- powerpc.orig/include/asm-powerpc/paca.h
> > +++ powerpc/include/asm-powerpc/paca.h
> > @@ -68,6 +68,7 @@ struct paca_struct {
> >  	void *emergency_sp;		/* pointer to emergency stack */
> >  	u64 data_offset;		/* per cpu data offset */
> >  	s16 hw_cpu_id;			/* Physical processor number */
> > +	u16 trap_save;			/* Used when bad stack is encountered */
> 
> Please read the comment at the top of the struct:
> 
>         /*
>          * Because hw_cpu_id, unlike other paca fields, is accessed
>          * routinely from other CPUs (from the IRQ code), we stick to
>          * read-only (after boot) fields in the first cacheline to
>          * avoid cacheline bouncing.
>          */
> 
> So please move this out of the first cache line.

The only time it's written to is when we detect a bad stack pointer,
after which we promptly panic, so it's essentially read-only (in fact
unused) during normal operation. Plus there's a massive gap there in
paca_struct before the start of the second cacheline.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [v2] powerpc: save trap number in bad_stack

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1312 bytes --]

On Mon, Apr 23, 2007 at 11:22:17PM +1000, Stephen Rothwell wrote:
> On Sun, 22 Apr 2007 18:38:21 -0500 olof@lixom.net (Olof Johansson) wrote:
> >
> > --- powerpc.orig/include/asm-powerpc/paca.h
> > +++ powerpc/include/asm-powerpc/paca.h
> > @@ -68,6 +68,7 @@ struct paca_struct {
> >  	void *emergency_sp;		/* pointer to emergency stack */
> >  	u64 data_offset;		/* per cpu data offset */
> >  	s16 hw_cpu_id;			/* Physical processor number */
> > +	u16 trap_save;			/* Used when bad stack is encountered */
> 
> Please read the comment at the top of the struct:
> 
>         /*
>          * Because hw_cpu_id, unlike other paca fields, is accessed
>          * routinely from other CPUs (from the IRQ code), we stick to
>          * read-only (after boot) fields in the first cacheline to
>          * avoid cacheline bouncing.
>          */
> 
> So please move this out of the first cache line.

Afaict, Olof's patch only writes to that field when we're about to
enter the bad_stack path.  In that case we're sufficiently screwed
that I don't think some extra cacheline bouncing matters.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [v2] powerpc: save trap number in bad_stack

[Olof Johansson]

On Mon, Apr 23, 2007 at 11:22:17PM +1000, Stephen Rothwell wrote:
> On Sun, 22 Apr 2007 18:38:21 -0500 olof@lixom.net (Olof Johansson) wrote:
> >
> > --- powerpc.orig/include/asm-powerpc/paca.h
> > +++ powerpc/include/asm-powerpc/paca.h
> > @@ -68,6 +68,7 @@ struct paca_struct {
> >  	void *emergency_sp;		/* pointer to emergency stack */
> >  	u64 data_offset;		/* per cpu data offset */
> >  	s16 hw_cpu_id;			/* Physical processor number */
> > +	u16 trap_save;			/* Used when bad stack is encountered */
> 
> Please read the comment at the top of the struct:
> 
>         /*
>          * Because hw_cpu_id, unlike other paca fields, is accessed
>          * routinely from other CPUs (from the IRQ code), we stick to
>          * read-only (after boot) fields in the first cacheline to
>          * avoid cacheline bouncing.
>          */
> 
> So please move this out of the first cache line.

Never thought I'd say this: Too many comments! I missed that one. :)

The variable should be written very rarely, so there should be no
performance impact for a normal system. Still, it's a valid point and
for consistency it should be moved.

There's room right before the u8 variables further down, I'll move
it there.


-Olof