parisc: increase kernel stack size to 32k

PATCH In review Helge Deller 3 participants 6 messages Apr 23, 2013 – Apr 27, 2013 PA-RISC architecture

lore

Share & tools

lore

Helge Deller

author maintainer

2013-04-23 20:22 UTC #0 lore raw

PATCH 1/1 parisc: increase kernel stack size to 32k

commit e4e1e78facf7565cada909a69c7fb6415b6e7b83

Author: Helge Deller <[email protected]>
Date:   Tue Apr 23 17:19:37 2013 +0200

parisc: increase kernel stack size to 32k

This commit temporarily increases the kernel stack size to 32k to avoid stack overflows which led to strange and hard-to-debug kernel crashes all over various kernel components.

The upcoming real fix will implement an own irq stack which should avoid those crashes without the need for 32k stacks.

Signed-off-by Helge Deller <[email protected]>

arch/parisc/include/asm/thread_info.h +1 -1

index d1fb79a..55335c1 100644
@@ -40,7 +40,7 @@ struct thread_info {
 
 /* thread information allocation */
 
-#define THREAD_SIZE_ORDER            2
+#define THREAD_SIZE_ORDER            3	/* 32k stack */
 /* Be sure to hunt all references to this down when you change the size of
  * the kernel stack */
 #define THREAD_SIZE             (PAGE_SIZE << THREAD_SIZE_ORDER)

Helge Deller

author maintainer

2013-04-24 07:33 UTC #1 lore raw

arch/parisc/include/asm/thread_info.h struct thread_info { · line 43

Quoted 10 lines · Helge Deller

commit e4e1e78facf7565cada909a69c7fb6415b6e7b83
Author: Helge Deller <[email protected]>
Date:   Tue Apr 23 17:19:37 2013 +0200
parisc: increase kernel stack size to 32k
arch/parisc/include/asm/thread_info.h +1 -1
-#define THREAD_SIZE_ORDER            2
+#define THREAD_SIZE_ORDER            3	/* 32k stack */

I tested again, and it actually needs to be 64k stacks to not crash any longer. So, the right temporary fix is:

Quoted 1 line

+#define THREAD_SIZE_ORDER 4 /* 64k stack */

Will send updated patch soon.

Helge

James Bottomley

maintainer

2013-04-24 20:41 UTC #2 lore raw

Quoted 15 lines · Helge Deller

arch/parisc/include/asm/thread_info.h struct thread_info { · line 43
Quoted 10 lines · Helge Deller
commit e4e1e78facf7565cada909a69c7fb6415b6e7b83
Author: Helge Deller <[email protected]>
Date:   Tue Apr 23 17:19:37 2013 +0200
parisc: increase kernel stack size to 32k
arch/parisc/include/asm/thread_info.h +1 -1
-#define THREAD_SIZE_ORDER            2
+#define THREAD_SIZE_ORDER            3	/* 32k stack */
I tested again, and it actually needs to be 64k stacks to not crash any longer. So, the right temporary fix is:

Quoted 3 lines

Quoted 1 line
+#define THREAD_SIZE_ORDER 4 /* 64k stack */
Will send updated patch soon.

This is an indicator of something seriously wrong somewhere. We've always had the 16k stack just because of our large frames. In theory, the IRQ stack should only be the same size as the kernel stack, so if we have both in the same place, we should only need at max 32k ... if we're still seeing problems related to stack overrun, then it might be we have an IRQ recursion where we shouldn't have. To be honest, I have a hard time explaining why our stacks should be over 8k.

James

Helge Deller

author maintainer

2013-04-26 22:30 UTC #3 lore raw

* James Bottomley <[email protected]>:

Quoted 20 lines · Helge Deller

Quoted 15 lines · Helge Deller
arch/parisc/include/asm/thread_info.h
Quoted 10 lines
commit e4e1e78facf7565cada909a69c7fb6415b6e7b83
Author: Helge Deller <[email protected]>
Date:   Tue Apr 23 17:19:37 2013 +0200
parisc: increase kernel stack size to 32k
arch/parisc/include/asm/thread_info.h +1 -1
-#define THREAD_SIZE_ORDER            2
+#define THREAD_SIZE_ORDER            3	/* 32k stack */
I tested again, and it actually needs to be 64k stacks to not crash any longer. So, the right temporary fix is:
Quoted 3 lines
Quoted 1 line
+#define THREAD_SIZE_ORDER 4 /* 64k stack */
Will send updated patch soon.

Quoted 7 lines

This is an indicator of something seriously wrong somewhere. We've always had the 16k stack just because of our large frames. In theory, the IRQ stack should only be the same size as the kernel stack, so if we have both in the same place, we should only need at max 32k ... if we're still seeing problems related to stack overrun, then it might be we have an IRQ recursion where we shouldn't have. To be honest, I have a hard time explaining why our stacks should be over 8k.

You are probably right.

Below is now a first implementation to support IRQ stacks for parisc.

I booted it successfully on 32- and 64bit kernels. Uncommenting the WARN_ON_ONCE(1) will show that the stacks get activated. As a nice feature we now have CONFIG_DEBUG_STACKOVERFLOW as well, which should help us to find the problematic functions (if there are any).

I have not tested it with my testcases yet, but if people want to play, here it is...

Helge

 Kconfig.debug             |   11 ++++++
 include/asm/irq.h         |    2 +
 include/asm/processor.h   |   17 ++++++++--
 include/asm/thread_info.h |    2 -
 kernel/entry.S            |   22 ++++++++++++
 kernel/irq.c              |   78
+++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 128 insertions(+), 4 deletions(-)

arch/parisc/Kconfig.debug +11 -0

index 7305ac8..ed88c37 100644
@@ -12,6 +12,17 @@ config DEBUG_RODATA
          portion of the kernel code won't be covered by a TLB anymore.
          If in doubt, say "N".
 
+config DEBUG_STACKOVERFLOW
+	bool "Check for stack overflows"
+	depends on DEBUG_KERNEL
+	---help---
+	  Say Y here if you want to check the overflows of kernel, IRQ
+	  and exception stacks. This option will cause messages of the
+	  stacks in detail when free stack space drops below a certain
+	  limit.
+	  If in doubt, say "N".
+
+
 config DEBUG_STRICT_USER_COPY_CHECKS
 	bool "Strict copy size checks"
 	depends on DEBUG_KERNEL && !TRACE_BRANCH_PROFILING

arch/parisc/include/asm/irq.h +2 -0

index 1073599..0417ebf 100644
@@ -10,6 +10,8 @@
 #include <linux/cpumask.h>
 #include <asm/types.h>
 
+#define __ARCH_HAS_DO_SOFTIRQ
+
 #define NO_IRQ		(-1)
 
 #ifdef CONFIG_GSC

arch/parisc/include/asm/processor.h +15 -2

index 09b54a5..1c126c6 100644
@@ -20,8 +20,6 @@
 
 #endif /* __ASSEMBLY__ */
 
-#define KERNEL_STACK_SIZE 	(4*PAGE_SIZE)
-
 /*
  * Default implementation of macro that returns current
  * instruction pointer ("program counter").

@@ -60,6 +58,21 @@
 
 #ifndef __ASSEMBLY__
 
+#ifdef __KERNEL__
+
+/*
+ * IRQ STACK - used for irq and irq bh handler
+ */
+#define IRQ_STACK_SIZE (4096 * 4) // PAGE_SIZE
+
+union irq_stack_union {
+	unsigned long irq_stack[IRQ_STACK_SIZE/sizeof(unsigned long)];
+};
+
+DECLARE_PER_CPU(union irq_stack_union, irq_stack_union);
+DECLARE_PER_CPU(unsigned int, irq_count);
+#endif /* __KERNEL__ */
+
 /*
  * Data detected about CPUs at boot time which is the same for all CPU's.
  * HP boxes are SMP - ie identical processors.

arch/parisc/include/asm/thread_info.h +1 -1

index d1fb79a..8642ea8 100644
@@ -40,7 +40,7 @@ struct thread_info {
 
 /* thread information allocation */
 
-#define THREAD_SIZE_ORDER            2
+#define THREAD_SIZE_ORDER            2 /* keep on 2 for 16k */
 /* Be sure to hunt all references to this down when you change the size of
  * the kernel stack */
 #define THREAD_SIZE             (PAGE_SIZE << THREAD_SIZE_ORDER)

arch/parisc/kernel/entry.S +22 -0

index f33201b..117d516 100644
@@ -1997,6 +1997,28 @@ ftrace_stub:
 ENDPROC(return_to_handler)
 #endif	/* CONFIG_FUNCTION_TRACER */
 
+/* void call_on_stack(unsigned long param1, void *func, unsigned long new_stack) */
+ENTRY(call_on_stack)
+	STREG	%sp, 8(%arg2)
+	STREG	%rp, 16(%arg2)
+
+	/* HPPA calling convention for function pointers */
+#ifdef CONFIG_64BIT
+	LDREG	16(%arg1), %arg1
+	bve,l	(%arg1), %rp
+	addi    0x40, %arg2, %sp
+#else
+	addi    0x40, %arg2, %sp
+	be,l	0(%sr4,%arg1), %sr0, %r31
+	copy	%r31, %rp
+#endif
+
+	addi    -0x40, %sp, %sp
+	LDREG	16(%sp),%rp
+	bv	(%rp)
+	LDREG	8(%sp),%sp
+ENDPROC(call_on_stack)
+
 
 get_register:
 	/*

arch/parisc/kernel/irq.c +77 -1

93 lines not on this page · raw message

Helge Deller

author maintainer

2013-04-27 21:53 UTC #4 lore raw

Quoted 28 lines · Helge Deller

* James Bottomley <[email protected]>:
Quoted 27 lines · Helge Deller
Quoted 15 lines · Helge Deller
arch/parisc/include/asm/thread_info.h
Quoted 10 lines
commit e4e1e78facf7565cada909a69c7fb6415b6e7b83
Author: Helge Deller <[email protected]>
Date:   Tue Apr 23 17:19:37 2013 +0200
parisc: increase kernel stack size to 32k
arch/parisc/include/asm/thread_info.h +1 -1
-#define THREAD_SIZE_ORDER            2
+#define THREAD_SIZE_ORDER            3	/* 32k stack */
I tested again, and it actually needs to be 64k stacks to not crash any longer. So, the right temporary fix is:
Quoted 3 lines
Quoted 1 line
+#define THREAD_SIZE_ORDER 4 /* 64k stack */
Will send updated patch soon.
This is an indicator of something seriously wrong somewhere. We've always had the 16k stack just because of our large frames. In theory, the IRQ stack should only be the same size as the kernel stack, so if we have both in the same place, we should only need at max 32k ... if we're still seeing problems related to stack overrun, then it might be we have an IRQ recursion where we shouldn't have. To be honest, I have a hard time explaining why our stacks should be over 8k.

Attached is a new version of my irq-stack-patch, which made my system really stable :-) As test I did used "hackbench 300" (from the LTP project) which created 12000 threads:
uptime: 23:44:09 up 17 min, 2 users, load average: 1232.23, 2966.09, 2466.39

My findings so far:
* kernel stack: THREAD_SIZE_ORDER needs to be at least 2 (=16k). x86 has 1 (8k). With 8k kernel stacks and DEBUG_STACKOVERFLOW enabled, I get directly after bootup:
stackcheck: swapper/0 has overflown a kernel stack (sp:bfc52030, stk bottom-top:bfc50000-bfc52000)

* IRQ stack: 16k seems sufficient as well.

So, the combination of 16k kernel stack and 16k irq stacks seems OK.

I still need to clean up my patch, test if backtraces still work (with which I currently have problems) and prepare a final patch.

Helge

arch/parisc/Kconfig.debug +11 -0

index 7305ac8..ed88c37 100644
@@ -12,6 +12,17 @@ config DEBUG_RODATA
          portion of the kernel code won't be covered by a TLB anymore.
          If in doubt, say "N".
 
+config DEBUG_STACKOVERFLOW
+	bool "Check for stack overflows"
+	depends on DEBUG_KERNEL
+	---help---
+	  Say Y here if you want to check the overflows of kernel, IRQ
+	  and exception stacks. This option will cause messages of the
+	  stacks in detail when free stack space drops below a certain
+	  limit.
+	  If in doubt, say "N".
+
+
 config DEBUG_STRICT_USER_COPY_CHECKS
 	bool "Strict copy size checks"
 	depends on DEBUG_KERNEL && !TRACE_BRANCH_PROFILING

arch/parisc/include/asm/irq.h +2 -0

index 1073599..0417ebf 100644
@@ -10,6 +10,8 @@
 #include <linux/cpumask.h>
 #include <asm/types.h>
 
+#define __ARCH_HAS_DO_SOFTIRQ
+
 #define NO_IRQ		(-1)
 
 #ifdef CONFIG_GSC

arch/parisc/include/asm/processor.h +16 -2

index 09b54a5..d725591 100644
@@ -20,8 +20,6 @@
 
 #endif /* __ASSEMBLY__ */
 
-#define KERNEL_STACK_SIZE 	(4*PAGE_SIZE)
-
 /*
  * Default implementation of macro that returns current
  * instruction pointer ("program counter").

@@ -33,6 +31,8 @@
 #endif
 #define current_text_addr() ({ void *pc; current_ia(pc); pc; })
 
+#define get_current_sp(sp)	__asm__("copy %%r30, %0" : "=r"(sp))
+
 #define TASK_SIZE_OF(tsk)       ((tsk)->thread.task_size)
 #define TASK_SIZE	        TASK_SIZE_OF(current)
 #define TASK_UNMAPPED_BASE      (current->thread.map_base)

@@ -61,6 +61,20 @@
 #ifndef __ASSEMBLY__
 
 /*
+ * IRQ STACK - used for irq and irq bh handler
+ */
+#ifdef __KERNEL__
+
+#define IRQ_STACK_SIZE (4096 << 2) /* = 16k, todo: use: PAGE_SIZE instead of 4096 */
+
+union irq_stack_union {
+	unsigned long irq_stack[IRQ_STACK_SIZE/sizeof(unsigned long)];
+};
+
+DECLARE_PER_CPU(union irq_stack_union, irq_stack_union);
+#endif /* __KERNEL__ */
+
+/*
  * Data detected about CPUs at boot time which is the same for all CPU's.
  * HP boxes are SMP - ie identical processors.
  *

arch/parisc/include/asm/thread_info.h +1 -1

index d1fb79a..85568cc 100644
@@ -40,7 +40,7 @@ struct thread_info {
 
 /* thread information allocation */
 
-#define THREAD_SIZE_ORDER            2
+#define THREAD_SIZE_ORDER            2 /* keep value 2 for 16k, use 3 for 32k */
 /* Be sure to hunt all references to this down when you change the size of
  * the kernel stack */
 #define THREAD_SIZE             (PAGE_SIZE << THREAD_SIZE_ORDER)

arch/parisc/kernel/entry.S +22 -0

index f33201b..117d516 100644
@@ -1997,6 +1997,28 @@ ftrace_stub:
 ENDPROC(return_to_handler)
 #endif	/* CONFIG_FUNCTION_TRACER */
 
+/* void call_on_stack(unsigned long param1, void *func, unsigned long new_stack) */
+ENTRY(call_on_stack)
+	STREG	%sp, 8(%arg2)
+	STREG	%rp, 16(%arg2)
+
+	/* HPPA calling convention for function pointers */
+#ifdef CONFIG_64BIT
+	LDREG	16(%arg1), %arg1
+	bve,l	(%arg1), %rp
+	addi    0x40, %arg2, %sp
+#else
+	addi    0x40, %arg2, %sp
+	be,l	0(%sr4,%arg1), %sr0, %r31
+	copy	%r31, %rp
+#endif
+
+	addi    -0x40, %sp, %sp
+	LDREG	16(%sp),%rp
+	bv	(%rp)
+	LDREG	8(%sp),%sp
+ENDPROC(call_on_stack)
+
 
 get_register:
 	/*

arch/parisc/kernel/irq.c +76 -1

92 lines not on this page · raw message

John David Anglin

2013-04-27 23:09 UTC #5 lore raw

Quoted 40 lines · Helge Deller

Quoted 28 lines · Helge Deller
* James Bottomley <[email protected]>:
Quoted 27 lines · Helge Deller
Quoted 15 lines · Helge Deller
arch/parisc/include/asm/thread_info.h
Quoted 10 lines
commit e4e1e78facf7565cada909a69c7fb6415b6e7b83
Author: Helge Deller <[email protected]>
Date:   Tue Apr 23 17:19:37 2013 +0200
parisc: increase kernel stack size to 32k
arch/parisc/include/asm/thread_info.h +1 -1
-#define THREAD_SIZE_ORDER            2
+#define THREAD_SIZE_ORDER            3	/* 32k stack */
I tested again, and it actually needs to be 64k stacks to not crash any longer. So, the right temporary fix is:
Quoted 3 lines
Quoted 1 line
+#define THREAD_SIZE_ORDER 4 /* 64k stack */
Will send updated patch soon.
This is an indicator of something seriously wrong somewhere. We've always had the 16k stack just because of our large frames. In theory, the IRQ stack should only be the same size as the kernel stack, so if we have both in the same place, we should only need at max 32k ... if we're still seeing problems related to stack overrun, then it might be we have an IRQ recursion where we shouldn't have. To be honest, I have a hard time explaining why our stacks should be over 8k.
Attached is a new version of my irq-stack-patch, which made my system really stable :-) As test I did used "hackbench 300" (from the LTP project) which created 12000 threads:
uptime: 23:44:09 up 17 min, 2 users, load average: 1232.23, 2966.09, 2466.39
My findings so far:
* kernel stack: THREAD_SIZE_ORDER needs to be at least 2 (=16k). x86 has 1 (8k). With 8k kernel stacks and DEBUG_STACKOVERFLOW enabled, I get
directly after bootup:
stackcheck: swapper/0 has overflown a kernel stack (sp:bfc52030, stk bottom-top:bfc50000-bfc52000)

Quoted 6 lines

* IRQ stack: 16k seems sufficient as well.
So, the combination of 16k kernel stack and 16k irq stacks seems OK.
I still need to clean up my patch, test if backtraces still work (with which I currently have problems) and prepare a final patch.

So far, I haven't been able to break the first version of the irqstack patch. It's working far better than setting THREAD_SIZE_ORDER to 4. There were various application errors with the latter. Building new version.

I have the following comments:

+#define get_current_sp(sp) __asm__("copy %%r30, %0" : "=r"(sp))

Probably, should be __asm__ __volatile__. I'm not sure that all changes to the stack pointer are known to GCC.

+/* void call_on_stack(unsigned long param1, void *func, unsigned long new_stack) */ +ENTRY(call_on_stack)
+ STREG %sp, 8(%arg2)
+ STREG %rp, 16(%arg2)
+
+ /* HPPA calling convention for function pointers */
+#ifdef CONFIG_64BIT
+ LDREG 16(%arg1), %arg1
+ bve,l (%arg1), %rp
+ addi 0x40, %arg2, %sp
+#else
+ addi 0x40, %arg2, %sp
+ be,l 0(%sr4,%arg1), %sr0, %r31
+ copy %r31, %rp
+#endif
+
+ addi -0x40, %sp, %sp
+ LDREG 16(%sp),%rp
+ bv (%rp)
+ LDREG 8(%sp),%sp
+ENDPROC(call_on_stack)

This doesn't full adhere to calling conventions but may work in limited circumstances. For example, 64-bit calls nominally require setup of the argument pointer. There's also save and restore of the PIC register if it can be modified.

Dave
--
John David Anglin [email protected]