arm64: Add support for IRQ stack

PATCH In review James Morse 4 participants 44 messages Dec 4, 2015 – Dec 21, 2015 arm64

lore

Share & tools

lore

James Morse

author

2015-12-04 11:02 UTC #0 lore raw

v8 cover arm64: Add support for IRQ stack

This consolidated series adds a per-cpu irq_stack, which will eventually allow us to reduce the size of task stacks.

Code in entry.S switches to the per-cpu irq_stack when irq_count is zero. This counter is updated in do_softirq_own_stack(), which is called before __do_softirq() re-enables interrupts, which could cause recursive use of the irq stack.

sp_el0 is used as a scratch register to store struct thread_info during el1 execution, this means code to find it by masking the stack pointer no longer needs to be inlined. This also lets us remove the alignment requirements for the irq stack, (task stacks still need to be naturally aligned).

Patch 2 is a combination of Akashi Takahiro's 'arm64: unwind_frame for interrupt stack' and 'arm64: fix dump_backtrace() to show correct pt_regs at interrupt', both of which need to be present before irq_stack is enabled in Patch 3.

Patch 4 is new, following Catalin's comments, but I don't think it is necessary unless we also decrease the stack size, which I don't think we should do immediatly - it would be good to collect some max_stack_size values for various workloads first.

This series is based on the v4.4-rc3
The series can be pulled from git://linux-arm.org/linux-jm.git -b irq_stack/v8

Comments welcome,

James

Changes since v7 [0]:
* Removed independent irq_stack_ptr and irq_count values. The lowest irq_stack value is used as irq_count, irq_stack_entry adds IRQ_STACK_START_SP to this when switching stack.
* Moved irq_stack definition from patch 1 to patch 2.
* Added patch 4

Changes since v6 [1]:
* Add irq_count and do_softirq_own_stack().
* Removed requirement that the irq_stack is naturally aligned.
* Made irq_stack per_cpu regardless of page size.

[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/385337.html
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/382238.html

AKASHI Takahiro (1):
arm64: Modify stack trace and dump for use with irq_stack

James Morse (2):
arm64: Add do_softirq_own_stack() and enable irq_stacks
arm64: switch to irq_stack during softirq

Jungseok Lee (1):
arm64: Store struct task_info in sp_el0

 arch/arm64/include/asm/irq.h         | 36 ++++++++++++++++
 arch/arm64/include/asm/thread_info.h | 10 ++++-
 arch/arm64/kernel/entry.S            | 82 +++++++++++++++++++++++++++++++++---
 arch/arm64/kernel/head.S             |  5 +++
 arch/arm64/kernel/irq.c              | 37 ++++++++++++++++
 arch/arm64/kernel/sleep.S            |  3 ++
 arch/arm64/kernel/stacktrace.c       | 29 ++++++++++++-
 arch/arm64/kernel/traps.c            | 14 +++++-
 8 files changed, 206 insertions(+), 10 deletions(-)

Signature · 1 line

2.6.2

James Morse

author

2015-12-04 11:02 UTC #1 lore raw

PATCH v8 1/4 arm64: Store struct task_info in sp_el0

From: Jungseok Lee <[email protected]>

There is need for figuring out how to manage struct thread_info data when IRQ stack is introduced. struct thread_info information should be copied to IRQ stack under the current thread_info calculation logic whenever context switching is invoked. This is too expensive to keep supporting the approach.

Instead, this patch pays attention to sp_el0 which is an unused scratch register in EL1 context. sp_el0 utilization not only simplifies the management, but also prevents text section size from being increased largely due to static allocated IRQ stack as removing masking operation using THREAD_SIZE in many places.

Signed-off-by Jungseok Lee <[email protected]>

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/include/asm/thread_info.h | 10 ++++++++--
 arch/arm64/kernel/entry.S            | 15 ++++++++++++---
 arch/arm64/kernel/head.S             |  5 +++++
 arch/arm64/kernel/sleep.S            |  3 +++
 4 files changed, 28 insertions(+), 5 deletions(-)

arch/arm64/include/asm/thread_info.h +8 -2

index 90c7ff233735..abd64bd1f6d9 100644
@@ -73,10 +73,16 @@ register unsigned long current_stack_pointer asm ("sp");
  */
 static inline struct thread_info *current_thread_info(void) __attribute_const__;
 
+/*
+ * struct thread_info can be accessed directly via sp_el0.
+ */
 static inline struct thread_info *current_thread_info(void)
 {
-	return (struct thread_info *)
-		(current_stack_pointer & ~(THREAD_SIZE - 1));
+	unsigned long sp_el0;
+
+	asm ("mrs %0, sp_el0" : "=r" (sp_el0));
+
+	return (struct thread_info *)sp_el0;
 }
 
 #define thread_saved_pc(tsk)	\

arch/arm64/kernel/entry.S +12 -3

index 7ed3d75f6304..fc87373d3f88 100644
@@ -88,7 +88,8 @@
 
 	.if	\el == 0
 	mrs	x21, sp_el0
-	get_thread_info tsk			// Ensure MDSCR_EL1.SS is clear,
+	mov	tsk, sp
+	and	tsk, tsk, #~(THREAD_SIZE - 1)	// Ensure MDSCR_EL1.SS is clear,
 	ldr	x19, [tsk, #TI_FLAGS]		// since we can unmask debug
 	disable_step_tsk x19, x20		// exceptions when scheduling.
 	.else

@@ -108,6 +109,13 @@
 	.endif
 
 	/*
+	 * Set sp_el0 to current thread_info.
+	 */
+	.if	\el == 0
+	msr	sp_el0, tsk
+	.endif
+
+	/*
 	 * Registers that may be useful after this macro is invoked:
 	 *
 	 * x21 - aborted SP

@@ -164,8 +172,7 @@ alternative_endif
 	.endm
 
 	.macro	get_thread_info, rd
-	mov	\rd, sp
-	and	\rd, \rd, #~(THREAD_SIZE - 1)	// top of stack
+	mrs	\rd, sp_el0
 	.endm
 
 /*

@@ -599,6 +606,8 @@ ENTRY(cpu_switch_to)
 	ldp	x29, x9, [x8], #16
 	ldr	lr, [x8]
 	mov	sp, x9
+	and	x9, x9, #~(THREAD_SIZE - 1)
+	msr	sp_el0, x9
 	ret
 ENDPROC(cpu_switch_to)

arch/arm64/kernel/head.S +5 -0

index 23cfc08fc8ba..b363f340f2c7 100644
@@ -424,6 +424,9 @@ __mmap_switched:
 	b	1b
 2:
 	adr_l	sp, initial_sp, x4
+	mov	x4, sp
+	and	x4, x4, #~(THREAD_SIZE - 1)
+	msr	sp_el0, x4			// Save thread_info
 	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
 	str_l	x24, memstart_addr, x6		// Save PHYS_OFFSET
 	mov	x29, #0

@@ -606,6 +609,8 @@ ENDPROC(secondary_startup)
 ENTRY(__secondary_switched)
 	ldr	x0, [x21]			// get secondary_data.stack
 	mov	sp, x0
+	and	x0, x0, #~(THREAD_SIZE - 1)
+	msr	sp_el0, x0			// save thread_info
 	mov	x29, #0
 	b	secondary_start_kernel
 ENDPROC(__secondary_switched)

arch/arm64/kernel/sleep.S +3 -0

index f586f7c875e2..e33fe33876ab 100644
@@ -173,6 +173,9 @@ ENTRY(cpu_resume)
 	/* load physical address of identity map page table in x1 */
 	adrp	x1, idmap_pg_dir
 	mov	sp, x2
+	/* save thread_info */
+	and	x2, x2, #~(THREAD_SIZE - 1)
+	msr	sp_el0, x2
 	/*
 	 * cpu_do_resume expects x0 to contain context physical address
 	 * pointer and x1 to contain physical address of 1:1 page tables

Signature · 1 line

2.6.2

Catalin Marinas

maintainer

2015-12-04 13:27 UTC #2 lore raw

PATCH v8 1/4 arm64: Store struct task_info in sp_el0

Quoted 2 lines · James Morse

arch/arm64/kernel/entry.S +0 -0

[...]

arch/arm64/kernel/entry.S ENTRY(cpu_switch_to) · line 612

Quoted 8 lines

arch/arm64/kernel/entry.S +2 -0

@@ -599,6 +606,8 @@ ENTRY(cpu_switch_to)
 	ldp	x29, x9, [x8], #16
 	ldr	lr, [x8]
 	mov	sp, x9
+	and	x9, x9, #~(THREAD_SIZE - 1)
+	msr	sp_el0, x9
 	ret
 ENDPROC(cpu_switch_to)

At the beginning of the cpu_switch_to function, could we do "mrs x9, sp_el0" instead to avoid the "and ... ~(THREAD_SIZE-1)"?

Otherwise:

Reviewed-by Catalin Marinas <[email protected]>

James Morse

author

2015-12-04 14:55 UTC #3 lore raw

PATCH v8 1/4 arm64: Store struct task_info in sp_el0

Hi Catalin,

Quoted 4 lines · Catalin Marinas

Quoted 2 lines · James Morse
arch/arm64/kernel/entry.S +0 -0
[...]

Quoted 11 lines

arch/arm64/kernel/entry.S ENTRY(cpu_switch_to) · line 612
Quoted 8 lines
arch/arm64/kernel/entry.S +2 -0
@@ -599,6 +606,8 @@ ENTRY(cpu_switch_to)
 	ldp	x29, x9, [x8], #16
 	ldr	lr, [x8]
 	mov	sp, x9
+	and	x9, x9, #~(THREAD_SIZE - 1)
+	msr	sp_el0, x9
 	ret
 ENDPROC(cpu_switch_to)
At the beginning of the cpu_switch_to function, could we do "mrs x9, sp_el0" instead to avoid the "and ... ~(THREAD_SIZE-1)"?

I'm not sure I follow - are you suggesting to store struct thread_info in the thread_cpu_context?

This would change a ldr to a ldp, and save the 'and', so its definitely fewer instructions.

James

Catalin Marinas

maintainer

2015-12-04 16:18 UTC #4 lore raw

PATCH v8 1/4 arm64: Store struct task_info in sp_el0

Quoted 19 lines · James Morse

Hi Catalin,
Quoted 4 lines · Catalin Marinas
Quoted 2 lines · James Morse
arch/arm64/kernel/entry.S +0 -0
[...]
Quoted 11 lines
arch/arm64/kernel/entry.S ENTRY(cpu_switch_to) · line 612
Quoted 8 lines
arch/arm64/kernel/entry.S +2 -0
@@ -599,6 +606,8 @@ ENTRY(cpu_switch_to)
 	ldp	x29, x9, [x8], #16
 	ldr	lr, [x8]
 	mov	sp, x9
+	and	x9, x9, #~(THREAD_SIZE - 1)
+	msr	sp_el0, x9
 	ret
 ENDPROC(cpu_switch_to)
At the beginning of the cpu_switch_to function, could we do "mrs x9, sp_el0" instead to avoid the "and ... ~(THREAD_SIZE-1)"?

Quoted 2 lines

I'm not sure I follow - are you suggesting to store struct thread_info in the thread_cpu_context?

I was thinking of context switching sp_el0 as well but see below.

Quoted 2 lines

This would change a ldr to a ldp, and save the 'and', so its definitely fewer instructions.

I think we end up with an additional memory access, which is usually more expensive than arithmetic ops. So just leave it as it is.

Signature · 1 line

Catalin

Jungseok Lee

2015-12-06 13:15 UTC #5 lore raw

PATCH v8 1/4 arm64: Store struct task_info in sp_el0

In reply to Catalin Marinas

On Dec 4, 2015, at 10:27 PM, Catalin Marinas wrote:

Hi Catalin,

Quoted 16 lines · James Morse

Quoted 2 lines
arch/arm64/kernel/entry.S +0 -0
[...]
arch/arm64/kernel/entry.S ENTRY(cpu_switch_to) · line 612
Quoted 8 lines
arch/arm64/kernel/entry.S +2 -0
@@ -599,6 +606,8 @@ ENTRY(cpu_switch_to)
 	ldp	x29, x9, [x8], #16
 	ldr	lr, [x8]
 	mov	sp, x9
+	and	x9, x9, #~(THREAD_SIZE - 1)
+	msr	sp_el0, x9
 	ret
 ENDPROC(cpu_switch_to)
At the beginning of the cpu_switch_to function, could we do "mrs x9, sp_el0" instead to avoid the "and ... ~(THREAD_SIZE-1)"?

Quoted 3 lines

Otherwise:
Reviewed-by Catalin Marinas <[email protected]>

Thanks for reviewing this one!

If this one is picked up without re-spin, it would be better to change the word 'task_info' in the subject to 'thread_info' for clarification.

Best Regards
Jungseok Lee

James Morse

author

2015-12-04 11:02 UTC #6 lore raw

PATCH v8 2/4 arm64: Modify stack trace and dump for use with irq_stack

In reply to James Morse

From: AKASHI Takahiro <[email protected]>

This patch allows unwind_frame() to traverse from interrupt stack to task stack correctly. It requires data from a dummy stack frame, created during irq_stack_entry(), added by a later patch.

A similar approach is taken to modify dump_backtrace(), which expects to find struct pt_regs underneath any call to functions marked __exception. When on an irq_stack, the struct pt_regs is stored on the old task stack, the location of which is stored in the dummy stack frame.

Signed-off-by AKASHI Takahiro <[email protected]>

[merged two patches, reworked for per_cpu irq_stacks, and no alignment
guarantees, added irq_stack definitions]

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/include/asm/irq.h   | 32 ++++++++++++++++++++++++++++++++
 arch/arm64/kernel/irq.c        |  3 +++
 arch/arm64/kernel/stacktrace.c | 29 +++++++++++++++++++++++++++--
 arch/arm64/kernel/traps.c      | 14 +++++++++++++-
 4 files changed, 75 insertions(+), 3 deletions(-)

arch/arm64/include/asm/irq.h +32 -0

index 8e8d30684392..e2f3f135a3bc 100644
@@ -1,10 +1,32 @@
 #ifndef __ASM_IRQ_H
 #define __ASM_IRQ_H
 
+#define IRQ_STACK_SIZE			THREAD_SIZE
+#define IRQ_STACK_START_SP		THREAD_START_SP
+
+#ifndef __ASSEMBLER__
+
+#include <linux/percpu.h>
+
 #include <asm-generic/irq.h>
+#include <asm/thread_info.h>
 
 struct pt_regs;
 
+DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);
+
+/*
+ * The highest address on the stack, and the first to be used. Used to
+ * find the dummy-stack frame put down by el?_irq() in entry.S.
+ */
+#define IRQ_STACK_PTR(cpu) ((unsigned long)per_cpu(irq_stack, cpu) + IRQ_STACK_START_SP)
+
+/*
+ * The offset from irq_stack_ptr where entry.S will store the original
+ * stack pointer. Used by unwind_frame() and dump_backtrace().
+ */
+#define IRQ_STACK_TO_TASK_STACK(ptr) *((unsigned long *)(ptr - 0x10));
+
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
 
 static inline int nr_legacy_irqs(void)

@@ -12,4 +34,14 @@ static inline int nr_legacy_irqs(void)
 	return 0;
 }
 
+static inline bool on_irq_stack(unsigned long sp, int cpu)
+{
+	/* variable names the same as kernel/stacktrace.c */
+	unsigned long low = (unsigned long)per_cpu(irq_stack, cpu);
+	unsigned long high = low + IRQ_STACK_START_SP;
+
+	return (low <= sp && sp <= high);
+}
+
+#endif /* !__ASSEMBLER__ */
 #endif

arch/arm64/kernel/irq.c +3 -0

index 9f17ec071ee0..1e3cef578e21 100644
@@ -30,6 +30,9 @@
 
 unsigned long irq_err_count;
 
+/* irq stack only needs to be 16 byte aligned - not IRQ_STACK_SIZE aligned */
+DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(16);
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 	show_ipi_list(p, prec);

arch/arm64/kernel/stacktrace.c +27 -2

index ccb6078ed9f2..b947eeffa5b2 100644
@@ -20,6 +20,7 @@
 #include <linux/sched.h>
 #include <linux/stacktrace.h>
 
+#include <asm/irq.h>
 #include <asm/stacktrace.h>
 
 /*

@@ -39,17 +40,41 @@ int notrace unwind_frame(struct stackframe *frame)
 {
 	unsigned long high, low;
 	unsigned long fp = frame->fp;
+	unsigned long irq_stack_ptr;
+
+	/*
+	 * Use raw_smp_processor_id() to avoid false-positives from
+	 * CONFIG_DEBUG_PREEMPT. get_wchan() calls unwind_frame() on sleeping
+	 * task stacks, we can be pre-empted in this case, so
+	 * {raw_,}smp_processor_id() may give us the wrong value. Sleeping
+	 * tasks can't ever be on an interrupt stack, so regardless of cpu,
+	 * the checks will always fail.
+	 */
+	irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id());
 
 	low  = frame->sp;
-	high = ALIGN(low, THREAD_SIZE);
+	/* irq stacks are not THREAD_SIZE aligned */
+	if (on_irq_stack(frame->sp, raw_smp_processor_id()))
+		high = irq_stack_ptr;
+	else
+		high = ALIGN(low, THREAD_SIZE) - 0x20;
 
-	if (fp < low || fp > high - 0x18 || fp & 0xf)
+	if (fp < low || fp > high || fp & 0xf)
 		return -EINVAL;
 
 	frame->sp = fp + 0x10;
 	frame->fp = *(unsigned long *)(fp);
 	frame->pc = *(unsigned long *)(fp + 8);
 
+	/*
+	 * Check whether we are going to walk through from interrupt stack
+	 * to task stack.
+	 * If we reach the end of the stack - and its an interrupt stack,
+	 * read the original task stack pointer from the dummy frame.
+	 */
+	if (frame->sp == irq_stack_ptr)
+		frame->sp = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+
 	return 0;
 }

arch/arm64/kernel/traps.c +13 -1

index e9b9b5364393..8a0084541f84 100644
@@ -146,6 +146,7 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
 static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 {
 	struct stackframe frame;
+	unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
 
 	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);

@@ -180,9 +181,20 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 		if (ret < 0)
 			break;
 		stack = frame.sp;
-		if (in_exception_text(where))
+		if (in_exception_text(where)) {
+			/*
+			 * If we switched to the irq_stack before calling this
+			 * exception handler, then the pt_regs will be on the
+			 * task stack. The easiest way to tell is if the large
+			 * pt_regs would overlap with the end of the irq_stack.
+			 */
+			if (stack < irq_stack_ptr &&
+			    (stack + sizeof(struct pt_regs)) > irq_stack_ptr)
+				stack = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+
 			dump_mem("", "Exception stack", stack,
 				 stack + sizeof(struct pt_regs), false);
+		}
 	}
 }

Signature · 1 line

2.6.2

Jungseok Lee

2015-12-04 12:21 UTC #7 lore raw

PATCH v8 2/4 arm64: Modify stack trace and dump for use with irq_stack

Quoted 27 lines · James Morse

From: AKASHI Takahiro <[email protected]>
This patch allows unwind_frame() to traverse from interrupt stack to task stack correctly. It requires data from a dummy stack frame, created during irq_stack_entry(), added by a later patch.
A similar approach is taken to modify dump_backtrace(), which expects to find struct pt_regs underneath any call to functions marked __exception. When on an irq_stack, the struct pt_regs is stored on the old task stack, the location of which is stored in the dummy stack frame.
Signed-off-by AKASHI Takahiro <[email protected]>
[merged two patches, reworked for per_cpu irq_stacks, and no alignment guarantees, added irq_stack definitions]
Signed-off-by James Morse <[email protected]>
---
arch/arm64/include/asm/irq.h   | 32 ++++++++++++++++++++++++++++++++
arch/arm64/kernel/irq.c        |  3 +++
arch/arm64/kernel/stacktrace.c | 29 +++++++++++++++++++++++++++--
arch/arm64/kernel/traps.c      | 14 +++++++++++++-
4 files changed, 75 insertions(+), 3 deletions(-)
arch/arm64/include/asm/irq.h +0 -0
index 8e8d30684392..e2f3f135a3bc 100644

arch/arm64/include/asm/irq.h line 4

Quoted 4 lines

arch/arm64/include/asm/irq.h +1 -0

@@ -1,10 +1,32 @@
 #ifndef __ASM_IRQ_H
 #define __ASM_IRQ_H
 
+#define IRQ_STACK_SIZE			THREAD_SIZE

I agree that it is unnecessary to reduce a process stack size at this stage. But, it would be no issue to set IRQ_STACK_SIZE to 8KB now, which is aligned with Catalin's comment [1].

Best Regards
Jungseok Lee

[1] http://www.spinics.net/lists/arm-kernel/msg463868.html

Best Regards
Jungseok Lee

Catalin Marinas

maintainer

2015-12-04 14:31 UTC #8 lore raw

PATCH v8 2/4 arm64: Modify stack trace and dump for use with irq_stack

In reply to James Morse

Quoted 4 lines · James Morse

arch/arm64/include/asm/irq.h +0 -0
index 8e8d30684392..e2f3f135a3bc 100644

[...]

Quoted 15 lines

arch/arm64/include/asm/irq.h +10 -0

+static inline bool on_irq_stack(unsigned long sp, int cpu)
+{
+	/* variable names the same as kernel/stacktrace.c */
+	unsigned long low = (unsigned long)per_cpu(irq_stack, cpu);
+	unsigned long high = low + IRQ_STACK_START_SP;
+
+	return (low <= sp && sp <= high);
+}
+
+#endif /* !__ASSEMBLER__ */
 #endif

arch/arm64/kernel/irq.c +0 -0

index 9f17ec071ee0..1e3cef578e21 100644

[...]

Quoted 13 lines

arch/arm64/kernel/irq.c +9 -0

@@ -39,17 +40,41 @@ int notrace unwind_frame(struct stackframe *frame)
 {
 	unsigned long high, low;
 	unsigned long fp = frame->fp;
+	unsigned long irq_stack_ptr;
+
+	/*
+	 * Use raw_smp_processor_id() to avoid false-positives from
+	 * CONFIG_DEBUG_PREEMPT. get_wchan() calls unwind_frame() on sleeping
+	 * task stacks, we can be pre-empted in this case, so
+	 * {raw_,}smp_processor_id() may give us the wrong value. Sleeping
+	 * tasks can't ever be on an interrupt stack, so regardless of cpu,
+	 * the checks will always fail.

arch/arm64/kernel/irq.c int notrace unwind_frame(struct stackframe *frame) · line 60

Quoted 10 lines

arch/arm64/kernel/irq.c +7 -1

+	 */
+	irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id());
 
 	low  = frame->sp;
-	high = ALIGN(low, THREAD_SIZE);
+	/* irq stacks are not THREAD_SIZE aligned */
+	if (on_irq_stack(frame->sp, raw_smp_processor_id()))
+		high = irq_stack_ptr;
+	else
+		high = ALIGN(low, THREAD_SIZE) - 0x20;

More of a nitpick: if you rework patch 4, you can remove on_irq_stack() here, just expand it inline (and avoid IRQ_STACK_PTR twice, though that's pretty cheap).

Either way:

Reviewed-by Catalin Marinas <[email protected]>

James Morse

author

2015-12-04 11:02 UTC #9 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

In reply to James Morse

entry.S is modified to switch to the per_cpu irq_stack during el{0,1}_irq. irq_count is used to detect recursive interrupts on the irq_stack, it is updated late by do_softirq_own_stack(), when called on the irq_stack, before __do_softirq() re-enables interrupts to process softirqs.

do_softirq_own_stack() is added by this patch, but does not yet switch stack.

This patch adds the dummy stack frame and data needed by the previous stack tracing patches.

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/include/asm/irq.h |  2 ++
 arch/arm64/kernel/entry.S    | 42 ++++++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/irq.c      | 38 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 79 insertions(+), 3 deletions(-)

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

index e2f3f135a3bc..fa2a8d0e4792 100644
@@ -11,6 +11,8 @@
 #include <asm-generic/irq.h>
 #include <asm/thread_info.h>
 
+#define __ARCH_HAS_DO_SOFTIRQ
+
 struct pt_regs;
 
 DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);

arch/arm64/kernel/entry.S +40 -2

62 lines not on this page · raw message

arch/arm64/kernel/irq.c +37 -1

index 1e3cef578e21..ff7ebb710e51 100644
@@ -25,14 +25,24 @@
 #include <linux/irq.h>
 #include <linux/smp.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/irqchip.h>
 #include <linux/seq_file.h>
 
 unsigned long irq_err_count;
 
-/* irq stack only needs to be 16 byte aligned - not IRQ_STACK_SIZE aligned */
+/*
+ * irq stack only needs to be 16 byte aligned - not IRQ_STACK_SIZE aligned.
+ * irq_stack[0] is used as irq_count, a non-zero value indicates the stack
+ * is in use, and el?_irq() shouldn't switch to it. This is used to detect
+ * recursive use of the irq_stack, it is lazily updated by
+ * do_softirq_own_stack(), which is called on the irq_stack, before
+ * re-enabling interrupts to process softirqs.
+ */
 DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(16);
 
+#define IRQ_COUNT()	(*per_cpu(irq_stack, smp_processor_id()))
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 	show_ipi_list(p, prec);

@@ -56,3 +66,29 @@ void __init init_IRQ(void)
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
 }
+
+/*
+ * do_softirq_own_stack() is called from irq_exit() before __do_softirq()
+ * re-enables interrupts, at which point we may re-enter el?_irq(). We
+ * increase irq_count here so that el1_irq() knows that it is already on the
+ * irq stack.
+ *
+ * Called with interrupts disabled, so we don't worry about moving cpu, or
+ * being interrupted while modifying irq_count.
+ *
+ * This function doesn't actually switch stack.
+ */
+void do_softirq_own_stack(void)
+{
+	int cpu = smp_processor_id();
+
+	WARN_ON_ONCE(!irqs_disabled());
+
+	if (on_irq_stack(current_stack_pointer, cpu)) {
+		IRQ_COUNT()++;
+		__do_softirq();
+		IRQ_COUNT()--;
+	} else {
+		__do_softirq();
+	}
+}

Signature · 1 line

2.6.2

Catalin Marinas

maintainer

2015-12-04 13:46 UTC #10 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

Quoted 14 lines · James Morse

arch/arm64/kernel/irq.c +14 -0

+/*
+ * do_softirq_own_stack() is called from irq_exit() before __do_softirq()
+ * re-enables interrupts, at which point we may re-enter el?_irq(). We
+ * increase irq_count here so that el1_irq() knows that it is already on the
+ * irq stack.
+ *
+ * Called with interrupts disabled, so we don't worry about moving cpu, or
+ * being interrupted while modifying irq_count.
+ *
+ * This function doesn't actually switch stack.
+ */
+void do_softirq_own_stack(void)
+{
+	int cpu = smp_processor_id();

arch/arm64/kernel/irq.c void __init init_IRQ(void) · line 93

Quoted 10 lines

arch/arm64/kernel/irq.c +10 -0

+
+	WARN_ON_ONCE(!irqs_disabled());
+
+	if (on_irq_stack(current_stack_pointer, cpu)) {
+		IRQ_COUNT()++;
+		__do_softirq();
+		IRQ_COUNT()--;
+	} else {
+		__do_softirq();
+	}

Following your and my reply on the previous series, should we not _always_ switch to the irqstack here?

Signature · 1 line

Catalin

Catalin Marinas

maintainer

2015-12-04 13:47 UTC #11 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

Quoted 20 lines · Catalin Marinas

Quoted 19 lines · James Morse

arch/arm64/kernel/irq.c +19 -0

+/*
+ * do_softirq_own_stack() is called from irq_exit() before __do_softirq()
+ * re-enables interrupts, at which point we may re-enter el?_irq(). We
+ * increase irq_count here so that el1_irq() knows that it is already on the
+ * irq stack.
+ *
+ * Called with interrupts disabled, so we don't worry about moving cpu, or
+ * being interrupted while modifying irq_count.
+ *
+ * This function doesn't actually switch stack.
+ */
+void do_softirq_own_stack(void)
+{
+	int cpu = smp_processor_id();
+
+	WARN_ON_ONCE(!irqs_disabled());
+
+	if (on_irq_stack(current_stack_pointer, cpu)) {
+		IRQ_COUNT()++;

Quoted 8 lines

arch/arm64/kernel/irq.c void __init init_IRQ(void) · line 93
Quoted 5 lines
arch/arm64/kernel/irq.c +5 -0
+		__do_softirq();
+		IRQ_COUNT()--;
+	} else {
+		__do_softirq();
+	}
Following your and my reply on the previous series, should we not _always_ switch to the irqstack here?

Ah, sorry, I now noticed that this is done in patch 4.

Signature · 1 line

Catalin

Catalin Marinas

maintainer

2015-12-07 22:48 UTC #12 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

In reply to James Morse

Quoted 5 lines · James Morse

entry.S is modified to switch to the per_cpu irq_stack during el{0,1}_irq. irq_count is used to detect recursive interrupts on the irq_stack, it is updated late by do_softirq_own_stack(), when called on the irq_stack, before __do_softirq() re-enables interrupts to process softirqs.

Quoted 7 lines

do_softirq_own_stack() is added by this patch, but does not yet switch stack.
This patch adds the dummy stack frame and data needed by the previous stack tracing patches.
Signed-off-by James Morse <[email protected]>

In the interest of getting things moving on this series:

Reviewed-by Catalin Marinas <[email protected]>

I propose that we skip patch 4 for now and, as James suggested, keep the THREAD_SIZE to 16KB for one more release cycle. It would be good to get some statistics on stack usage.

Patches 1-3 look fine to me on their own.

Signature · 1 line

Catalin

Will Deacon

2015-12-08 11:43 UTC #13 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

Quoted 16 lines · Catalin Marinas

Quoted 5 lines · James Morse
entry.S is modified to switch to the per_cpu irq_stack during el{0,1}_irq. irq_count is used to detect recursive interrupts on the irq_stack, it is updated late by do_softirq_own_stack(), when called on the irq_stack, before __do_softirq() re-enables interrupts to process softirqs.
Quoted 7 lines
do_softirq_own_stack() is added by this patch, but does not yet switch stack.
This patch adds the dummy stack frame and data needed by the previous stack tracing patches.
Signed-off-by James Morse <[email protected]>
In the interest of getting things moving on this series:

Quoted 7 lines

Reviewed-by Catalin Marinas <[email protected]>
I propose that we skip patch 4 for now and, as James suggested, keep the THREAD_SIZE to 16KB for one more release cycle. It would be good to get some statistics on stack usage.
Patches 1-3 look fine to me on their own.

I'll pick them up and see if they survive testing.

Will

Jungseok Lee

2015-12-08 16:02 UTC #14 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

Quoted 19 lines · Will Deacon

Quoted 18 lines · Catalin Marinas
Quoted 5 lines · James Morse
entry.S is modified to switch to the per_cpu irq_stack during el{0,1}_irq. irq_count is used to detect recursive interrupts on the irq_stack, it is updated late by do_softirq_own_stack(), when called on the irq_stack, before __do_softirq() re-enables interrupts to process softirqs.
Quoted 7 lines
do_softirq_own_stack() is added by this patch, but does not yet switch stack.
This patch adds the dummy stack frame and data needed by the previous stack tracing patches.
Signed-off-by James Morse <[email protected]>
In the interest of getting things moving on this series:
Reviewed-by Catalin Marinas <[email protected]>

Quoted 7 lines

Quoted 5 lines
I propose that we skip patch 4 for now and, as James suggested, keep the THREAD_SIZE to 16KB for one more release cycle. It would be good to get some statistics on stack usage.
Patches 1-3 look fine to me on their own.
I'll pick them up and see if they survive testing.

Dear All

I've seen the following BUG log with CONFIG_DEBUG_SPINLOCK=y kernel.

 BUG: spinlock lockup suspected on CPU#1

Under that option, I cannot even complete a single kernel build successfully. I hope I'm the only person to experience it. My Android machine is running well for over 12 hours now with the below change.

----8<----

arch/arm64/include/asm/irq.h +1 -1

index fa2a8d0..db90a69 100644
@@ -27,7 +27,7 @@ DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);
  * The offset from irq_stack_ptr where entry.S will store the original
  * stack pointer. Used by unwind_frame() and dump_backtrace().
  */
-#define IRQ_STACK_TO_TASK_STACK(ptr) *((unsigned long *)(ptr - 0x10));
+#define IRQ_STACK_TO_TASK_STACK(ptr) *((unsigned long *)(ptr - 0x20));
 
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

arch/arm64/kernel/entry.S +1 -1

index 81cc538..7d54c26 100644
@@ -199,7 +199,7 @@ alternative_endif
        /* Add a dummy stack frame */
        stp     x29, \dummy_lr, [sp, #-16]!           // dummy stack frame
        mov     x29, sp
-       stp     xzr, x19, [sp, #-16]!
+       stp     x19, xzr, [sp, #-16]!
 
 9998:
        .endm

----8<----

If I read the patches correctly, the dummy stack frame looks as follows.

top ------------ <- irq_stack_ptr

      | dummy_lr |
      ------------
      |   x29    |
      ------------ <- new frame pointer (x29)
      |   x19    |
      ------------
      |   xzr    |
      ------------

So, we should refer to x19 in order to retrieve frame->sp. But, frame->sp is xzr under the current implementation. I suspect this causes spinlock lockup.

Please correct me if I'm wrong.

Best Regards
Jungseok Lee

James Morse

author

2015-12-08 17:23 UTC #15 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

Quoted 7 lines · Jungseok Lee

I've seen the following BUG log with CONFIG_DEBUG_SPINLOCK=y kernel.
 BUG: spinlock lockup suspected on CPU#1
Under that option, I cannot even complete a single kernel build successfully. I hope I'm the only person to experience it. My Android machine is running well for over 12 hours now with the below change.

I can't reproduce this, can you send me your .config file (off-list)? Do you have any other patches in your tree? What hardware are you using?

Quoted 12 lines

If I read the patches correctly, the dummy stack frame looks as follows.
top ------------ <- irq_stack_ptr
      | dummy_lr |
      ------------
      |   x29    |
      ------------ <- new frame pointer (x29)
      |   x19    |
      ------------
      |   xzr    |
      ------------

Quoted 2 lines

So, we should refer to x19 in order to retrieve frame->sp. But, frame->sp is xzr under the current implementation. I suspect this causes spinlock lockup.

This is the sort of place where it is too easy to make an off-by-one error, I will go through it all with the debugger again tomorrow.

I'm not seeing this when testing on Juno. This would only affect the tracing code, are you running perf or ftrace at the same time?

I've just re-tested this with defconfig, and the following hack:
=======%<=======

arch/arm64/kernel/stacktrace.c +3 -1

index b947eeffa5b2..686086e4d870 100644
@@ -72,8 +72,10 @@ int notrace unwind_frame(struct stackframe *frame)
         * If we reach the end of the stack - and its an interrupt stack,
         * read the original task stack pointer from the dummy frame.
         */
-       if (frame->sp == irq_stack_ptr)
+       if (frame->sp == irq_stack_ptr) {
                frame->sp = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+               BUG_ON(frame->sp == 0);
+       }
 
        return 0;
 }

=======%<=======

While running:

Quoted 1 line

sudo ./perf record -e mem:<address of __do_softirq>:x -ag -- sleep 180

and

Quoted 1 line

dd if=/dev/sda of=/dev/null bs=512 iflag=direct;

This should cause lots of interrupts from /dev/sda, and cause the tracing code to step between irq_stack and the original task stack frequently. The BUG_ON() doesn't fire, and the perf trace output looks correct.

My only theory is that there is an off by one, and its reading what was x29 instead. This wouldn't show up in these tests, but might be a problem for aarch32 user-space, as presumably x29==0 when it switches to aarch64 mode for el0_irq(). I will try this tomorrow.

Thanks,

James

Will Deacon

2015-12-08 17:27 UTC #16 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

Quoted 11 lines · James Morse

Quoted 7 lines · Jungseok Lee
I've seen the following BUG log with CONFIG_DEBUG_SPINLOCK=y kernel.
 BUG: spinlock lockup suspected on CPU#1
Under that option, I cannot even complete a single kernel build successfully. I hope I'm the only person to experience it. My Android machine is running well for over 12 hours now with the below change.
I can't reproduce this, can you send me your .config file (off-list)? Do you have any other patches in your tree? What hardware are you using?

FWIW, I tried to reproduce it and hit something that looks slightly different. Crash log below.

Quoted 12 lines

Quoted 12 lines
If I read the patches correctly, the dummy stack frame looks as follows.
top ------------ <- irq_stack_ptr
      | dummy_lr |
      ------------
      |   x29    |
      ------------ <- new frame pointer (x29)
      |   x19    |
      ------------
      |   xzr    |
      ------------

Quoted 5 lines

Quoted 2 lines
So, we should refer to x19 in order to retrieve frame->sp. But, frame->sp is xzr under the current implementation. I suspect this causes spinlock lockup.
This is the sort of place where it is too easy to make an off-by-one error, I will go through it all with the debugger again tomorrow.

Ok; I'll hold off pushing this into linux-next until we've worked out what's going wrong.

Quoted 2 lines

I'm not seeing this when testing on Juno. This would only affect the tracing code, are you running perf or ftrace at the same time?

I just set CONFIG_PROVE_LOCKING=y, but that likely turns on a bunch of the tracing infrastructure.

Will

--->8

Unable to handle kernel paging request at virtual address 7fff6dd050
pgd = ffffffc0601de000
[7fff6dd050] *pgd=00000000f905b003, *pud=00000000f905b003, *pmd=00000000f90cd003, *pte=00a00009f52aabd3
Internal error: Oops: 9600000b [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1365 Comm: networking Not tainted 4.4.0-rc3+ #1
Hardware name: ARM Juno development board (r0) (DT)
task: ffffffc0792be400 ti: ffffffc0790dc000 task.ti: ffffffc0790dc000
PC is at unwind_frame+0x74/0xa0
LR is at walk_stackframe+0x28/0x50
pc : [<ffffffc0000896bc>] lr : [<ffffffc000089710>] pstate: a00001c5

sp : ffffffc97fe5b8a0
x29: ffffffc97fe5b8a0 x28: ffffffc0792be400
x27: ffffffc000994000 x26: ffffffc000c0f000
x25: ffffffc0792beab0 x24: ffffffc0792beb00
x23: ffffffc000c18518 x22: ffffffc0016aa9a0
x21: ffffffc0000895a0 x20: ffffffc97fe5b8f8
x19: ffffffc97fe5b908 x18: 000000000000381b
x17: ffffffc0014b0130 x16: ffffffc00168f9d8
x15: 000000000000381a x14: ffffffc00136f8d8
x13: 0000000000000002 x12: 0000000000000000
x11: ffffffc975de1d00 x10: ffffffc001696000
x9 : 0000000000000000 x8 : 0000000000000001
x7 : ffffffc000bff8d8 x6 : 0000000000000000
x5 : 0000007fff6dced0 x4 : 0000007fff6dd060
x3 : 0000000000000000 x2 : 0000007fff6dd050
x1 : ffffffc97fe58060 x0 : ffffffc97fe5b908

Process networking (pid: 1365, stack limit = 0xffffffc0790dc020)

Stack: (0xffffffc97fe5b8a0 to 0xffffffc0790e0000)
Call trace:
[<ffffffc0000896bc>] unwind_frame+0x74/0xa0
[<ffffffc000089794>] save_stack_trace_tsk+0x5c/0xa8
[<ffffffc0000897f8>] save_stack_trace+0x18/0x20
[<ffffffc0000f9600>] save_trace+0x48/0x100
[<ffffffc0000fd690>] __lock_acquire+0x1bc8/0x1c48
[<ffffffc0000fdedc>] lock_acquire+0x4c/0x68
[<ffffffc00064f2b0>] _raw_spin_lock+0x40/0x58
[<ffffffc0001af9a8>] unfreeze_partials.isra.23+0x78/0x2c0
[<ffffffc0001afefc>] put_cpu_partial+0x16c/0x200
[<ffffffc0001b14c4>] __slab_free+0x2e4/0x430
[<ffffffc0001b1e1c>] kfree+0x1d4/0x1e8
[<ffffffc00013c9dc>] put_css_set_locked+0x114/0x168
[<ffffffc00013cd7c>] put_css_set+0xac/0xc0
[<ffffffc000143544>] cgroup_free+0x9c/0x108
[<ffffffc0000b41c0>] __put_task_struct+0x38/0x110
[<ffffffc0000b7b60>] delayed_put_task_struct+0x40/0x50
[<ffffffc000115d50>] rcu_process_callbacks+0x2f8/0x5f8
[<ffffffc0000bb0c4>] __do_softirq+0x13c/0x278
[<ffffffc0000860d4>] do_softirq_own_stack+0x84/0xc8
[<ffffffc0000bb3c0>] irq_exit+0xa0/0xd8
[<ffffffc000107b60>] __handle_domain_irq+0x60/0xb8
[<ffffffc0000824f0>] gic_handle_irq+0x58/0xa8
Exception stack(0x0000007fff6dc3e0 to 0x0000007fff6dc500)

c3e0: 0000007fff6dc420 000000558c8d6df8 000000558c8ed058 000000558c8ec000
c400: 0000000000000000 00000055c65b3105 00000055c65bed30 0000000000000000
c420: 0000007fff6dc440 000000558c8caef8 0000000000000000 0000007fff6dc47f
c440: 0000007fff6dc5f0 000000558c8cb3c4 0000000000000001 0000000000000000
c460: 0000000000000000 00000055c65b3105 00000055c65bed30 000000558c8d1b00
c480: 0000000000000000 0000000100000000 000000558c8ec548 00000055c65bed30
c4a0: 0000007fff6dc4b0 0000007fff6ddf82 0000007fff6dc7a8 0000000000000001
c4c0: 0000000000000000 0000000000000000 00000055c65b3105 00000055c65bed30
c4e0: 0000000000000000 00000055c65b77f8 000000558c8ec000 0000007fff6dc6c8 [<ffffffc000085b0c>] el0_irq_naked+0x4c/0x60 [<000000558c8d6d88>] 0x558c8d6d88
[<000000558c8d6df8>] 0x558c8d6df8
[<000000558c8caef8>] 0x558c8caef8
[<000000558c8cb3c4>] 0x558c8cb3c4
[<000000558c8ca2e4>] 0x558c8ca2e4
[<000000558c8ca2ac>] 0x558c8ca2ac
[<000000558c8ca924>] 0x558c8ca924
[<000000558c8cb5a0>] 0x558c8cb5a0
[<000000558c8ca2e4>] 0x558c8ca2e4
[<000000558c8cb738>] 0x558c8cb738
[<000000558c8ce92c>] 0x558c8ce92c
[<000000558c8ceb80>] 0x558c8ceb80
[<000000558c8cb1c0>] 0x558c8cb1c0
[<000000558c8ca2e4>] 0x558c8ca2e4
[<000000558c8ca2ac>] 0x558c8ca2ac
[<000000558c8ca3f0>] 0x558c8ca3f0
[<000000558c8ca3f0>] 0x558c8ca3f0
[<000000558c8ca52c>] 0x558c8ca52c
[<000000558c8ca2ac>] 0x558c8ca2ac
[<000000558c8d18b0>] 0x558c8d18b0
[<000000558c8c8604>] 0x558c8c8604
[<0000007f968fe9bc>] 0x7f968fe9bc

Jungseok Lee

2015-12-08 23:13 UTC #17 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

In reply to James Morse

On Dec 9, 2015, at 2:23 AM, James Morse wrote:

Hi James,

Quoted 11 lines · Jungseok Lee

Quoted 7 lines
I've seen the following BUG log with CONFIG_DEBUG_SPINLOCK=y kernel.
BUG: spinlock lockup suspected on CPU#1
Under that option, I cannot even complete a single kernel build successfully. I hope I'm the only person to experience it. My Android machine is running well for over 12 hours now with the below change.
I can't reproduce this, can you send me your .config file (off-list)? Do you have any other patches in your tree? What hardware are you using?

No additional patches with dragonboard410c [1]. I don't have Juno unfortunately. Please refer to 'Lock Debugging' and 'RCU Debugging' of .config.

#
# Lock Debugging (spinlocks, mutexes, etc...)
#
# CONFIG_DEBUG_RT_MUTEXES is not set
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
# CONFIG_DEBUG_WW_MUTEX_SLOWPATH is not set
CONFIG_DEBUG_LOCK_ALLOC=y
CONFIG_PROVE_LOCKING=y
CONFIG_LOCKDEP=y
# CONFIG_LOCK_STAT is not set
# CONFIG_DEBUG_LOCKDEP is not set
# CONFIG_DEBUG_ATOMIC_SLEEP is not set
# CONFIG_DEBUG_LOCKING_API_SELFTESTS is not set
# CONFIG_LOCK_TORTURE_TEST is not set
CONFIG_TRACE_IRQFLAGS=y
CONFIG_STACKTRACE=y
# CONFIG_DEBUG_KOBJECT is not set
CONFIG_HAVE_DEBUG_BUGVERBOSE=y
CONFIG_DEBUG_BUGVERBOSE=y
# CONFIG_DEBUG_LIST is not set
# CONFIG_DEBUG_PI_LIST is not set
# CONFIG_DEBUG_SG is not set
# CONFIG_DEBUG_NOTIFIERS is not set
# CONFIG_DEBUG_CREDENTIALS is not set

#
# RCU Debugging
#
CONFIG_PROVE_RCU=y
# CONFIG_PROVE_RCU_REPEATEDLY is not set
# CONFIG_SPARSE_RCU_POINTER is not set
# CONFIG_TORTURE_TEST is not set
# CONFIG_RCU_TORTURE_TEST is not set
CONFIG_RCU_CPU_STALL_TIMEOUT=21
# CONFIG_RCU_TRACE is not set
# CONFIG_RCU_EQS_DEBUG is not set
# CONFIG_DEBUG_BLOCK_EXT_DEVT is not set
# CONFIG_NOTIFIER_ERROR_INJECTION is not set
# CONFIG_FAULT_INJECTION is not set
CONFIG_HAVE_FUNCTION_TRACER=y
CONFIG_HAVE_FUNCTION_GRAPH_TRACER=y
CONFIG_HAVE_DYNAMIC_FTRACE=y
CONFIG_HAVE_FTRACE_MCOUNT_RECORD=y
CONFIG_HAVE_SYSCALL_TRACEPOINTS=y
CONFIG_HAVE_C_RECORDMCOUNT=y
CONFIG_TRACING_SUPPORT=y
# CONFIG_FTRACE is not set

Quoted 15 lines

Quoted 12 lines
If I read the patches correctly, the dummy stack frame looks as follows.
top ------------ <- irq_stack_ptr
     | dummy_lr |
     ------------
     |   x29    |
     ------------ <- new frame pointer (x29)
     |   x19    |
     ------------
     |   xzr    |
     ------------
Quoted 2 lines
So, we should refer to x19 in order to retrieve frame->sp. But, frame->sp is xzr under the current implementation. I suspect this causes spinlock lockup.

Quoted 6 lines

This is the sort of place where it is too easy to make an off-by-one error, I will go through it all with the debugger again tomorrow.
I'm not seeing this when testing on Juno. This would only affect the tracing code, are you running perf or ftrace at the same time?

No. I've got the same call trace Will reported when only trying kernel build.

Quoted 33 lines

I've just re-tested this with defconfig, and the following hack:
=======%<=======
arch/arm64/kernel/stacktrace.c +3 -1
index b947eeffa5b2..686086e4d870 100644
@@ -72,8 +72,10 @@ int notrace unwind_frame(struct stackframe *frame)
        * If we reach the end of the stack - and its an interrupt stack,
        * read the original task stack pointer from the dummy frame.
        */
-       if (frame->sp == irq_stack_ptr)
+       if (frame->sp == irq_stack_ptr) {
               frame->sp = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+               BUG_ON(frame->sp == 0);
+       }
 
       return 0;
 }
=======%<=======
While running:
Quoted 1 line
sudo ./perf record -e mem:<address of __do_softirq>:x -ag -- sleep 180
and
Quoted 1 line
dd if=/dev/sda of=/dev/null bs=512 iflag=direct;
This should cause lots of interrupts from /dev/sda, and cause the tracing code to step between irq_stack and the original task stack frequently. The BUG_ON() doesn't fire, and the perf trace output looks correct.

Quoted 4 lines

My only theory is that there is an off by one, and its reading what was x29 instead. This wouldn't show up in these tests, but might be a problem for aarch32 user-space, as presumably x29==0 when it switches to aarch64 mode for el0_irq(). I will try this tomorrow.

My description on dummy stack frame was incorrect. frame->sp is retrieved from x29, not xzr currently. That is why 0x20 is needed instead of 0x10.

Best Regards
Jungseok Lee

James Morse

author

2015-12-09 09:47 UTC #18 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

In reply to James Morse

Quoted 4 lines · James Morse

My only theory is that there is an off by one, and its reading what was x29 instead. This wouldn't show up in these tests, but might be a problem for aarch32 user-space, as presumably x29==0 when it switches to aarch64 mode for el0_irq(). I will try this tomorrow.

Yup, this is what is happening. Its an off-by-one due to broken thinking about how the stack works. My broken thinking was:

Quoted 9 lines

top ------------

      | dummy_lr | <- irq_stack_ptr
      ------------
      |   x29    |
      ------------
      |   x19    | <- irq_stack_ptr - 0x10
      ------------
      |   xzr    |
      ------------

But the stack-pointer is decreased before use. So it actually looks like this:

Quoted 11 lines

------------
| | <- irq_stack_ptr
top ------------

      | dummy_lr |
      ------------
      |   x29    | <- irq_stack_ptr - 0x10
      ------------
      |   x19    |
      ------------
      |   xzr    | <- irq_stack_ptr - 0x20
      ------------

The value being used as the original stack is x29, which in all the tests is sp but without the current frames data, hence there are no missing frames in the output.

Jungseok Lee picked it up with a 32bit user space because aarch32 can't use x29, so it remains 0 forever. The fix he posted is correct.

Will: do you want to take Jungseok Lee's patch as a 'Fixes:', or is it easier if I repost the series?

Thanks,

James

Will Deacon

2015-12-09 11:38 UTC #19 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

Quoted 38 lines · James Morse

Quoted 4 lines · James Morse
My only theory is that there is an off by one, and its reading what was x29 instead. This wouldn't show up in these tests, but might be a problem for aarch32 user-space, as presumably x29==0 when it switches to aarch64 mode for el0_irq(). I will try this tomorrow.
Yup, this is what is happening. Its an off-by-one due to broken thinking about how the stack works. My broken thinking was:
Quoted 9 lines
top ------------
      | dummy_lr | <- irq_stack_ptr
      ------------
      |   x29    |
      ------------
      |   x19    | <- irq_stack_ptr - 0x10
      ------------
      |   xzr    |
      ------------
But the stack-pointer is decreased before use. So it actually looks like this:
Quoted 11 lines
------------
| | <- irq_stack_ptr
top ------------
      | dummy_lr |
      ------------
      |   x29    | <- irq_stack_ptr - 0x10
      ------------
      |   x19    |
      ------------
      |   xzr    | <- irq_stack_ptr - 0x20
      ------------
The value being used as the original stack is x29, which in all the tests is sp but without the current frames data, hence there are no missing frames in the output.

Quoted 5 lines

Jungseok Lee picked it up with a 32bit user space because aarch32 can't use x29, so it remains 0 forever. The fix he posted is correct.
Will: do you want to take Jungseok Lee's patch as a 'Fixes:', or is it easier if I repost the series?

I'll take it as a fix on top, but I still want to get to the bottom of why unwind_frame appeared to be exploding. We really shouldn't be relying on the frame layout to provide us with safe addresses in there.

Will

Will Deacon

2015-12-09 13:45 UTC #20 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

In reply to James Morse

Hi James,

Quoted 44 lines · James Morse

entry.S is modified to switch to the per_cpu irq_stack during el{0,1}_irq. irq_count is used to detect recursive interrupts on the irq_stack, it is updated late by do_softirq_own_stack(), when called on the irq_stack, before __do_softirq() re-enables interrupts to process softirqs.
do_softirq_own_stack() is added by this patch, but does not yet switch stack.
This patch adds the dummy stack frame and data needed by the previous stack tracing patches.
Signed-off-by James Morse <[email protected]>
---
 arch/arm64/include/asm/irq.h |  2 ++
 arch/arm64/kernel/entry.S    | 42 ++++++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/irq.c      | 38 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 79 insertions(+), 3 deletions(-)
arch/arm64/include/asm/irq.h +2 -0
index e2f3f135a3bc..fa2a8d0e4792 100644
@@ -11,6 +11,8 @@
 #include <asm-generic/irq.h>
 #include <asm/thread_info.h>
 
+#define __ARCH_HAS_DO_SOFTIRQ
+
 struct pt_regs;
 
 DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);
arch/arm64/kernel/entry.S +1 -0
index fc87373d3f88..81cc5380977d 100644
@@ -27,6 +27,7 @@
 #include <asm/cpufeature.h>
 #include <asm/errno.h>
 #include <asm/esr.h>
+#include <asm/irq.h>
 #include <asm/thread_info.h>
 #include <asm/unistd.h>
 

arch/arm64/kernel/entry.S alternative_endif · line 184

Quoted 9 lines

arch/arm64/kernel/entry.S +6 -0

@@ -175,6 +176,42 @@ alternative_endif
 	mrs	\rd, sp_el0
 	.endm
 
+	.macro	irq_stack_entry, dummy_lr
+	mov	x19, sp			// preserve the original sp
+
+	adr_l	x25, irq_stack
+	mrs	x26, tpidr_el1
+	add	x25, x25, x26

Perhaps we could add a macro to assembler.h to correspond to __my_cpu_offset in percpu.h?

Quoted 8 lines

arch/arm64/kernel/entry.S +8 -0

+
+	/*
+	 * Check the lowest address on irq_stack for the irq_count value,
+	 * incremented by do_softirq_own_stack if we have re-enabled irqs
+	 * while on the irq_stack.
+	 */
+	ldr	x26, [x25]
+	cbnz	x26, 9998f		// recursive use?

arch/arm64/kernel/entry.S alternative_endif · line 202

Quoted 10 lines

arch/arm64/kernel/entry.S +10 -0

+
+	/* switch to the irq stack */
+	mov	x26, #IRQ_STACK_START_SP
+	add	x26, x25, x26
+	mov	sp, x26
+
+	/* Add a dummy stack frame */
+	stp     x29, \dummy_lr, [sp, #-16]!           // dummy stack frame
+	mov	x29, sp
+	stp     xzr, x19, [sp, #-16]!

Hmm. I'm not sure we necessarily want to push a frame when the interrupt was taken from userspace. The unwinder will either explode (which should be fixed separately) or truncate the walk anyway.

If we changed this so that we only push a frame when taking an interrupt from EL1, could we then avoid pushing x19 as well and get the unwinder to walk back through the pushed fp like it usually would?

For the case where we've come from EL0, we want to zero fp. I don't *think* we need to push anything at all.

Thoughts?

Will

James Morse

author

2015-12-09 14:36 UTC #21 lore raw

PATCH v8 3/4 arm64: Add do_softirq_own_stack() and enable irq_stacks

Hi Will,

Quoted 16 lines · Will Deacon

Quoted 16 lines

arch/arm64/kernel/entry.S +1 -0

index fc87373d3f88..81cc5380977d 100644
@@ -27,6 +27,7 @@
 #include <asm/cpufeature.h>
 #include <asm/errno.h>
 #include <asm/esr.h>
+#include <asm/irq.h>
 #include <asm/thread_info.h>
 #include <asm/unistd.h>

@@ -175,6 +176,42 @@ alternative_endif
 	mrs	\rd, sp_el0
 	.endm

Quoted 9 lines

arch/arm64/kernel/entry.S alternative_endif · line 184
Quoted 6 lines
arch/arm64/kernel/entry.S +6 -0
+	.macro	irq_stack_entry, dummy_lr
+	mov	x19, sp			// preserve the original sp
+
+	adr_l	x25, irq_stack
+	mrs	x26, tpidr_el1
+	add	x25, x25, x26
Perhaps we could add a macro to assembler.h to correspond to __my_cpu_offset in percpu.h?

Is it worth going all the way and having a this_cpu_ptr() macro? I can't think of any other use for reading tpidr_el1 from asm.

Quoted 14 lines

arch/arm64/kernel/entry.S +14 -0

+
+	/*
+	 * Check the lowest address on irq_stack for the irq_count value,
+	 * incremented by do_softirq_own_stack if we have re-enabled irqs
+	 * while on the irq_stack.
+	 */
+	ldr	x26, [x25]
+	cbnz	x26, 9998f		// recursive use?
+
+	/* switch to the irq stack */
+	mov	x26, #IRQ_STACK_START_SP
+	add	x26, x25, x26
+	mov	sp, x26
+

Quoted 8 lines

arch/arm64/kernel/entry.S alternative_endif · line 202
Quoted 4 lines
arch/arm64/kernel/entry.S +4 -0
+	/* Add a dummy stack frame */
+	stp     x29, \dummy_lr, [sp, #-16]!           // dummy stack frame
+	mov	x29, sp
+	stp     xzr, x19, [sp, #-16]!
Hmm. I'm not sure we necessarily want to push a frame when the interrupt was taken from userspace. The unwinder will either explode (which should be fixed separately) or truncate the walk anyway.

I think the exploding is due to insufficient sanity checks round the:

Quoted 2 lines

???????if (frame->sp == irq_stack_ptr) ??????????????frame->sp = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);

This is effectively allowing the maybe-bogus-fp value at the bottom of the irq_stack to pass the bounds checking, without doing any checking of the IRQ_STACK_TO_TASK_STACK(irq_stack_ptr) value.

An extra thing we can do here is check that both this new frame->sp and frame->fp point into current->stack.

Quoted 3 lines

If we changed this so that we only push a frame when taking an interrupt from EL1, could we then avoid pushing x19 as well and get the unwinder to walk back through the pushed fp like it usually would?

x19 is also struct pt_regs, which dump_backtrace() wants to print the registers that were passed into gic_handle_irq(). I think this still needs to be found at a known location on the stack.

Quoted 1 line

For the case where we've come from EL0, we want to zero fp.

That sounds better - I will see how it behaves.

Thanks!

James

James Morse

author

2015-12-04 11:02 UTC #22 lore raw

PATCH v8 4/4 arm64: switch to irq_stack during softirq

In reply to James Morse

do_softirq_own_stack() was added in a previous patch, but was only used to increase the irq_count value before __do_softirq() re-enables interrupts.

This patch adds a helper to call __do_softirq() on the irq stack. This means do_softirq() can be called by a task that is running out of stack space, as the work will be done on the irq_stack.

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/include/asm/irq.h |  2 ++
 arch/arm64/kernel/entry.S    | 25 +++++++++++++++++++++++++
 arch/arm64/kernel/irq.c      |  4 +---
 3 files changed, 28 insertions(+), 3 deletions(-)

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

index fa2a8d0e4792..6fd25ab534ca 100644
@@ -31,6 +31,8 @@ DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);
 
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
 
+void __do_softirq_on_irqstack(void);
+
 static inline int nr_legacy_irqs(void)
 {
 	return 0;

arch/arm64/kernel/entry.S +25 -0

index 81cc5380977d..2963050e5e9e 100644
@@ -779,3 +779,28 @@ ENTRY(sys_rt_sigreturn_wrapper)
 	mov	x0, sp
 	b	sys_rt_sigreturn
 ENDPROC(sys_rt_sigreturn_wrapper)
+
+ENTRY(__do_softirq_on_irqstack)
+	push	x19, lr
+	push	x25, x26
+
+	irq_stack_entry lr
+
+	/* irq_stack_entry leaves irq_count in x25 */
+	ldr	x1, [x25]
+	add	x1, x1, #1
+	str	x1, [x25]
+
+	bl	__do_softirq
+
+	ldr	x1, [x25]
+	sub	x1, x1, #1
+	str	x1, [x25]
+
+	irq_stack_exit
+
+	pop x25, x26
+	pop x19, lr
+	ret
+ENDPROC(__do_softirq_on_irqstack)
+

arch/arm64/kernel/irq.c +1 -3

index ff7ebb710e51..cbf1f00993ef 100644
@@ -75,8 +75,6 @@ void __init init_IRQ(void)
  *
  * Called with interrupts disabled, so we don't worry about moving cpu, or
  * being interrupted while modifying irq_count.
- *
- * This function doesn't actually switch stack.
  */
 void do_softirq_own_stack(void)
 {

@@ -89,6 +87,6 @@ void do_softirq_own_stack(void)
 		__do_softirq();
 		IRQ_COUNT()--;
 	} else {
-		__do_softirq();
+		__do_softirq_on_irqstack();
 	}
 }

Signature · 1 line

2.6.2

Catalin Marinas

maintainer

2015-12-04 14:01 UTC #23 lore raw

PATCH v8 4/4 arm64: switch to irq_stack during softirq

Quoted 13 lines · James Morse

arch/arm64/kernel/entry.S +13 -0

+ENTRY(__do_softirq_on_irqstack)
+	push	x19, lr
+	push	x25, x26
+
+	irq_stack_entry lr
+
+	/* irq_stack_entry leaves irq_count in x25 */
+	ldr	x1, [x25]
+	add	x1, x1, #1
+	str	x1, [x25]
+
+	bl	__do_softirq
+

arch/arm64/kernel/entry.S ENTRY(sys_rt_sigreturn_wrapper) · line 805

Quoted 10 lines

arch/arm64/kernel/entry.S +10 -0

+	ldr	x1, [x25]
+	sub	x1, x1, #1
+	str	x1, [x25]
+
+	irq_stack_exit
+
+	pop x25, x26
+	pop x19, lr
+	ret
+ENDPROC(__do_softirq_on_irqstack)

I was thinking of doing do_softirq_own_stack() entirely in assembly without the need to check for on_irq_stack() check in C, just a test of the irq_count value.

But this means collapsing patches 3 and 4 ;).

Signature · 1 line

Catalin

James Morse

author

2015-12-04 14:39 UTC #24 lore raw

PATCH v8 4/4 arm64: switch to irq_stack during softirq

Hi Catalin,

Quoted 20 lines · Catalin Marinas

Quoted 19 lines · James Morse

arch/arm64/kernel/entry.S +19 -0

+ENTRY(__do_softirq_on_irqstack)
+	push	x19, lr
+	push	x25, x26
+
+	irq_stack_entry lr
+
+	/* irq_stack_entry leaves irq_count in x25 */
+	ldr	x1, [x25]
+	add	x1, x1, #1
+	str	x1, [x25]
+
+	bl	__do_softirq
+
+	ldr	x1, [x25]
+	sub	x1, x1, #1
+	str	x1, [x25]
+
+	irq_stack_exit
+

Quoted 8 lines

arch/arm64/kernel/entry.S ENTRY(sys_rt_sigreturn_wrapper) · line 805
Quoted 4 lines
arch/arm64/kernel/entry.S +4 -0
+	pop x25, x26
+	pop x19, lr
+	ret
+ENDPROC(__do_softirq_on_irqstack)
I was thinking of doing do_softirq_own_stack() entirely in assembly without the need to check for on_irq_stack() check in C, just a test of the irq_count value.

I tried that, but couldn't get it to work - maybe I'm missing a trick:

There are at least two ways into do_softirq_own_stack():
el1_irq() -> __irq_exit() -> do_softirq_own_stack(), as well as:
cpu_switch_to(ksoftirqd) -> do_softirq_own_stack()

In both cases irq_count == 0 because do_softirq_own_stack() is where we update irq_count [0]. I can only see two ways to tell them apart, increment irq_count in el1_irq(), (which we don't like), or have that bounds checking.

(We could increase hardirq_count() in el?_irq(), which would mean in_irq()/in_interrupt() always returns true when on the irq_stack, which would stop softirqs being processed here... but that is a fairly significant change in behaviour.)

Thanks,

James

[0] http://article.gmane.org/gmane.linux.kernel/2041877

Catalin Marinas

maintainer

2015-12-04 18:40 UTC #25 lore raw

PATCH v8 4/4 arm64: switch to irq_stack during softirq

Quoted 30 lines · James Morse

Quoted 20 lines · Catalin Marinas
Quoted 19 lines · James Morse
arch/arm64/kernel/entry.S +19 -0
+ENTRY(__do_softirq_on_irqstack)
+	push	x19, lr
+	push	x25, x26
+
+	irq_stack_entry lr
+
+	/* irq_stack_entry leaves irq_count in x25 */
+	ldr	x1, [x25]
+	add	x1, x1, #1
+	str	x1, [x25]
+
+	bl	__do_softirq
+
+	ldr	x1, [x25]
+	sub	x1, x1, #1
+	str	x1, [x25]
+
+	irq_stack_exit
+
Quoted 8 lines
arch/arm64/kernel/entry.S ENTRY(sys_rt_sigreturn_wrapper) · line 805
Quoted 4 lines
arch/arm64/kernel/entry.S +4 -0
+	pop x25, x26
+	pop x19, lr
+	ret
+ENDPROC(__do_softirq_on_irqstack)
I was thinking of doing do_softirq_own_stack() entirely in assembly without the need to check for on_irq_stack() check in C, just a test of the irq_count value.

Quoted 6 lines

I tried that, but couldn't get it to work - maybe I'm missing a trick:
There are at least two ways into do_softirq_own_stack():
el1_irq() -> __irq_exit() -> do_softirq_own_stack(), as well as:
cpu_switch_to(ksoftirqd) -> do_softirq_own_stack()

If it was only these two cases, I think it would have been easier as in both scenarios the stack is nearly empty. But there is another call to do_softirq() from netif_rx_ni() and I'm not sure how the stack looks like at this point. That's why I suggested that we always switch to a different stack for softirqs.

Quoted 4 lines

In both cases irq_count == 0 because do_softirq_own_stack() is where we update irq_count [0]. I can only see two ways to tell them apart, increment irq_count in el1_irq(), (which we don't like), or have that bounds checking.

I now see the problem. Looking at x86, they increment irq_count for every interrupt (as per Jungseok's early patch).

Quoted 4 lines

(We could increase hardirq_count() in el?_irq(), which would mean in_irq()/in_interrupt() always returns true when on the irq_stack, which would stop softirqs being processed here... but that is a fairly significant change in behaviour.)

So I think the options are (a) irq_count increment in el?_irq or (b) check the current stack before switching. I'll think about it until Monday ;).

BTW, at which point does in_irq() return true for an interrupt (and when it ceases to do so)?

Signature · 1 line

Catalin

James Morse

author

2015-12-08 10:29 UTC #26 lore raw

PATCH v8 4/4 arm64: switch to irq_stack during softirq

Quoted 2 lines · Catalin Marinas

BTW, at which point does in_irq() return true for an interrupt (and when it ceases to do so)?

It uses hardirq_count(), so its true between irq_enter() and irq_exit().

The calls out from handle_IPI() are wrapped by these (except scheduler_ipi(), which calls them itself).

The other route looks like:
gic_handle_irq() -> __handle_domain_irq()
which wraps generic_handle_irq() in irq_enter()/irq_exit().

I don't know how chained_irq_enter() changes/affects this.

Thanks,

James

Jungseok Lee

2015-12-06 13:51 UTC #27 lore raw

PATCH v8 4/4 arm64: switch to irq_stack during softirq

In reply to James Morse

On Dec 4, 2015, at 11:39 PM, James Morse wrote:

Hi James,

Quoted 45 lines

Hi Catalin,
Quoted 20 lines · Catalin Marinas
Quoted 19 lines · James Morse
arch/arm64/kernel/entry.S +19 -0
+ENTRY(__do_softirq_on_irqstack)
+	push	x19, lr
+	push	x25, x26
+
+	irq_stack_entry lr
+
+	/* irq_stack_entry leaves irq_count in x25 */
+	ldr	x1, [x25]
+	add	x1, x1, #1
+	str	x1, [x25]
+
+	bl	__do_softirq
+
+	ldr	x1, [x25]
+	sub	x1, x1, #1
+	str	x1, [x25]
+
+	irq_stack_exit
+
Quoted 8 lines
arch/arm64/kernel/entry.S ENTRY(sys_rt_sigreturn_wrapper) · line 805
Quoted 4 lines
arch/arm64/kernel/entry.S +4 -0
+	pop x25, x26
+	pop x19, lr
+	ret
+ENDPROC(__do_softirq_on_irqstack)
I was thinking of doing do_softirq_own_stack() entirely in assembly without the need to check for on_irq_stack() check in C, just a test of the irq_count value.
I tried that, but couldn't get it to work - maybe I'm missing a trick:
There are at least two ways into do_softirq_own_stack():
el1_irq() -> __irq_exit() -> do_softirq_own_stack(), as well as:
cpu_switch_to(ksoftirqd) -> do_softirq_own_stack()
In both cases irq_count == 0 because do_softirq_own_stack() is where we update irq_count [0]. I can only see two ways to tell them apart, increment irq_count in el1_irq(), (which we don't like), or have that bounds checking.

Quoted 4 lines

(We could increase hardirq_count() in el?_irq(), which would mean in_irq()/in_interrupt() always returns true when on the irq_stack, which would stop softirqs being processed here... but that is a fairly significant change in behavior.)

It is not simple to utilize preempt_count which is based on task for stack switching based on core. Unluckily, I cannot give a specific challenge I've experienced.

Quoted 1 line

From implementation perspective, I think it is an advantage to use tpidr_el1

for percpu variables. But, preempt_count based approach might not utilize it.

Best Regards
Jungseok Lee

Jungseok Lee

2015-12-04 12:17 UTC #28 lore raw

v8 cover arm64: Add support for IRQ stack

In reply to James Morse

On Dec 4, 2015, at 8:02 PM, James Morse wrote:

Hi James,

Quoted 23 lines

This consolidated series adds a per-cpu irq_stack, which will eventually allow us to reduce the size of task stacks.
Code in entry.S switches to the per-cpu irq_stack when irq_count is zero. This counter is updated in do_softirq_own_stack(), which is called before __do_softirq() re-enables interrupts, which could cause recursive use of the irq stack.
sp_el0 is used as a scratch register to store struct thread_info during el1 execution, this means code to find it by masking the stack pointer no longer needs to be inlined. This also lets us remove the alignment requirements for the irq stack, (task stacks still need to be naturally aligned).
Patch 2 is a combination of Akashi Takahiro's 'arm64: unwind_frame for interrupt stack' and 'arm64: fix dump_backtrace() to show correct pt_regs at interrupt', both of which need to be present before irq_stack is enabled in Patch 3.
Patch 4 is new, following Catalin's comments, but I don't think it is necessary unless we also decrease the stack size, which I don't think we should do immediatly - it would be good to collect some max_stack_size values for various workloads first.

Quoted 4 lines

This series is based on the v4.4-rc3
The series can be pulled from git://linux-arm.org/linux-jm.git -b irq_stack/v8
Comments welcome,

At first glance, this series looks mature ;) I will leave feedbacks after playing with this.

Quoted 22 lines

James
Changes since v7 [0]:
* Removed independent irq_stack_ptr and irq_count values. The lowest irq_stack value is used as irq_count, irq_stack_entry adds IRQ_STACK_START_SP to this when switching stack.
* Moved irq_stack definition from patch 1 to patch 2.
* Added patch 4
Changes since v6 [1]:
* Add irq_count and do_softirq_own_stack().
* Removed requirement that the irq_stack is naturally aligned.
* Made irq_stack per_cpu regardless of page size.
[0] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/385337.html
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/382238.html
AKASHI Takahiro (1):
arm64: Modify stack trace and dump for use with irq_stack

Quoted 6 lines

James Morse (2):
arm64: Add do_softirq_own_stack() and enable irq_stacks
arm64: switch to irq_stack during softirq
Jungseok Lee (1):
arm64: Store struct task_info in sp_el0

s/task_info/thread_info would be clearer.

Best Regards
Jungseok Lee

Jungseok Lee

2015-12-06 13:56 UTC #29 lore raw

v8 cover arm64: Add support for IRQ stack

Quoted 27 lines · Jungseok Lee

On Dec 4, 2015, at 8:02 PM, James Morse wrote:
Hi James,
Quoted 23 lines
This consolidated series adds a per-cpu irq_stack, which will eventually allow us to reduce the size of task stacks.
Code in entry.S switches to the per-cpu irq_stack when irq_count is zero. This counter is updated in do_softirq_own_stack(), which is called before __do_softirq() re-enables interrupts, which could cause recursive use of the irq stack.
sp_el0 is used as a scratch register to store struct thread_info during el1 execution, this means code to find it by masking the stack pointer no longer needs to be inlined. This also lets us remove the alignment requirements for the irq stack, (task stacks still need to be naturally aligned).
Patch 2 is a combination of Akashi Takahiro's 'arm64: unwind_frame for interrupt stack' and 'arm64: fix dump_backtrace() to show correct pt_regs at interrupt', both of which need to be present before irq_stack is enabled in Patch 3.
Patch 4 is new, following Catalin's comments, but I don't think it is necessary unless we also decrease the stack size, which I don't think we should do immediatly - it would be good to collect some max_stack_size values for various workloads first.

Quoted 7 lines

Quoted 4 lines
This series is based on the v4.4-rc3
The series can be pulled from git://linux-arm.org/linux-jm.git -b irq_stack/v8
Comments welcome,
At first glance, this series looks mature ;) I will leave feedbacks after playing with this.

I will hold off until the IRQ count management scheme is determined, which is an important design point.

Best Regards
Jungseok Lee

Catalin Marinas

maintainer

2015-12-04 13:57 UTC #30 lore raw

v8 cover arm64: Add support for IRQ stack

In reply to James Morse

Quoted 2 lines · James Morse

Patch 4 is new, following Catalin's comments, but I don't think it is necessary unless we also decrease the stack size,

That's the whole aim of this patchset, to try to to get back to 8KB stacks.

Quoted 2 lines

which I don't think we should do immediatly - it would be good to collect some max_stack_size values for various workloads first.

We could make it configurable though, maybe defaulting to 16KB for a while.

IIRC, the stack overflows we had in the past were triggered when running specweb but I don't remember whether it was hard or soft IRQs that caused the issues (could have been both). We could run some tests again and see how close we get to the 8KB limit.

BTW, what's the disadvantage of always switching to the IRQ stack in do_softirq?

Signature · 1 line

Catalin

Jungseok Lee

2015-12-06 13:33 UTC #31 lore raw

v8 cover arm64: Add support for IRQ stack

Quoted 13 lines · Catalin Marinas

Quoted 2 lines · James Morse
Patch 4 is new, following Catalin's comments, but I don't think it is necessary unless we also decrease the stack size,
That's the whole aim of this patchset, to try to to get back to 8KB stacks.
Quoted 2 lines
which I don't think we should do immediatly - it would be good to collect some max_stack_size values for various workloads first.
We could make it configurable though, maybe defaulting to 16KB for a while.

Quoted 7 lines

IIRC, the stack overflows we had in the past were triggered when running specweb but I don't remember whether it was hard or soft IRQs that caused the issues (could have been both). We could run some tests again and see how close we get to the 8KB limit.
BTW, what's the disadvantage of always switching to the IRQ stack in do_softirq?

As we know well, additional operations, such as IRQ count management and stack pointer update, are the only items I'm aware of.

Best Regards
Jungseok Lee

James Morse

author

2015-12-10 10:22 UTC #32 lore raw

[PATCH v8 5/4] arm64: Fix off-by-one in stack tracing when stepping off irq stack

In reply to James Morse

From: Jungseok Lee <[email protected]>

Signed-off-by Jungseok Lee <[email protected]>

Signed-off-by James Morse <[email protected]>

---
I believe Will already has this patch, its included here for completeness.

 arch/arm64/include/asm/irq.h | 2 +-
 arch/arm64/kernel/entry.S    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/arm64/include/asm/irq.h +1 -1

index 6fd25ab534ca..cdc2916a00f0 100644
@@ -27,7 +27,7 @@ DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);
  * The offset from irq_stack_ptr where entry.S will store the original
  * stack pointer. Used by unwind_frame() and dump_backtrace().
  */
-#define IRQ_STACK_TO_TASK_STACK(ptr) *((unsigned long *)(ptr - 0x10));
+#define IRQ_STACK_TO_TASK_STACK(ptr) *((unsigned long *)(ptr - 0x20));
 
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

arch/arm64/kernel/entry.S +1 -1

index 2963050e5e9e..76e803118f57 100644
@@ -199,7 +199,7 @@ alternative_endif
 	/* Add a dummy stack frame */
 	stp     x29, \dummy_lr, [sp, #-16]!           // dummy stack frame
 	mov	x29, sp
-	stp     xzr, x19, [sp, #-16]!
+	stp     x19, xzr, [sp, #-16]!
 
 9998:
 	.endm

Signature · 1 line

2.6.2

James Morse

author

2015-12-10 10:22 UTC #33 lore raw

[PATCH v8 6/4] arm64: Add this_cpu_ptr() assembler macro for use in entry.S

irq_stack is a per_cpu variable, that needs to be access from entry.S. Use an assembler macro instead of the unreadable details.

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/include/asm/assembler.h | 11 +++++++++++
 arch/arm64/kernel/entry.S          |  4 +---
 2 files changed, 12 insertions(+), 3 deletions(-)

arch/arm64/include/asm/assembler.h +11 -0

index 12eff928ef8b..bb7b72734c24 100644
@@ -193,6 +193,17 @@ lr	.req	x30		// link register
 	str	\src, [\tmp, :lo12:\sym]
 	.endm
 
+	/*
+	 * @sym: The name of the per-cpu variable
+	 * @reg: Result of per_cpu(sym, smp_processor_id())
+	 * @tmp: scratch register
+	 */
+	.macro this_cpu_ptr, sym, reg, tmp
+	adr_l	\reg, \sym
+	mrs	\tmp, tpidr_el1
+	add	\reg, \reg, \tmp
+	.endm
+
 /*
  * Annotate a function as position independent, i.e., safe to be called before
  * the kernel virtual mapping is activated.

arch/arm64/kernel/entry.S +1 -3

index 76e803118f57..9a0450344335 100644
@@ -179,9 +179,7 @@ alternative_endif
 	.macro	irq_stack_entry, dummy_lr
 	mov	x19, sp			// preserve the original sp
 
-	adr_l	x25, irq_stack
-	mrs	x26, tpidr_el1
-	add	x25, x25, x26
+	this_cpu_ptr irq_stack, x25, x26
 
 	/*
 	 * Check the lowest address on irq_stack for the irq_count value,

Signature · 1 line

2.6.2

James Morse

author

2015-12-10 10:22 UTC #34 lore raw

[PATCH v8 7/4] arm64: when walking onto the task stack, check sp & fp are in current->stack

In reply to James Morse

When unwind_frame() reaches the bottom of the irq_stack, the last fp points to the original task stack. unwind_frame() uses IRQ_STACK_TO_TASK_STACK() to find the sp value. If either values is wrong, we may end up walking a corrupt stack.

Check these values are sane by testing if they are both on the stack pointed to by current->stack.

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/kernel/stacktrace.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

arch/arm64/kernel/stacktrace.c +10 -2

index b947eeffa5b2..d916d5b6aef6 100644
@@ -71,9 +71,17 @@ int notrace unwind_frame(struct stackframe *frame)
 	 * to task stack.
 	 * If we reach the end of the stack - and its an interrupt stack,
 	 * read the original task stack pointer from the dummy frame.
+	 *
+	 * Check the frame->fp we read from the bottom of the irq_stack,
+	 * and the original task stack pointer are both in current->stack.
 	 */
-	if (frame->sp == irq_stack_ptr)
-		frame->sp = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+	if (frame->sp == irq_stack_ptr) {
+		unsigned long orig_sp = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
+
+		if(object_is_on_stack((void *)orig_sp) &&
+		   object_is_on_stack((void *)frame->fp))
+			frame->sp = orig_sp;
+	}
 
 	return 0;
 }

Signature · 1 line

2.6.2

James Morse

author

2015-12-10 10:22 UTC #35 lore raw

[PATCH v8 8/4] arm64: don't call C code with el0's fp register

In reply to James Morse

On entry from el0, we save all the registers on the kernel stack, and restore them before returning. x29 remains unchanged when we call out to C code, which will store x29 as the frame-pointer on the stack.

Instead, write 0 into x29 after entry from el0, to avoid any risk of tracing into user space.

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/kernel/entry.S | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/kernel/entry.S +2 -0

index 9a0450344335..8dd009b4cab2 100644
@@ -93,6 +93,8 @@
 	and	tsk, tsk, #~(THREAD_SIZE - 1)	// Ensure MDSCR_EL1.SS is clear,
 	ldr	x19, [tsk, #TI_FLAGS]		// since we can unmask debug
 	disable_step_tsk x19, x20		// exceptions when scheduling.
+
+	mov	x29, xzr			// fp pointed to user-space
 	.else
 	add	x21, sp, #S_FRAME_SIZE
 	.endif

Signature · 1 line

2.6.2

Jungseok Lee

2015-12-10 14:03 UTC #36 lore raw

[PATCH v8 5/4] arm64: Fix off-by-one in stack tracing when stepping off irq stack

In reply to James Morse

On Dec 10, 2015, at 7:22 PM, James Morse wrote:

Hi James,

Quoted 1 line

From: Jungseok Lee <[email protected]>

I'm satisfied with spotting the frame->sp issue ;) According to for-next/core branch, Will have already worked on this change with an informative commit msg.

Thanks for your care!

Best Regards
Jungseok Lee

James Morse

author

2015-12-15 11:21 UTC #37 lore raw

[PATCH v8 9/4] arm64: reduce stack use in irq_handler

In reply to James Morse

The code for switching to irq_stack stores three pieces of information on the stack, fp+lr, as a fake stack frame (that lets us walk back onto the interrupted tasks stack frame), and the address of the struct pt_regs that contains the register values from kernel entry. (which dump_backtrace() will print in any stack trace).

To reduce this, we store fp, and the pointer to the struct pt_regs. unwind_frame() can recognise this as the irq_stack dummy frame, (as it only appears at the top of the irq_stack), and use the struct pt_regs values to find the missing interrupted link-register.

Signed-off-by James Morse <[email protected]>

---
This patch also silently removes the other stupid mistakes I made in IRQ_STACK_TO_TASK_STACK().

Based on for-next/core

 arch/arm64/include/asm/irq.h   | 11 ++++-------
 arch/arm64/kernel/entry.S      | 12 +++++++-----
 arch/arm64/kernel/stacktrace.c | 17 +++++++++++++++--
 3 files changed, 26 insertions(+), 14 deletions(-)

arch/arm64/include/asm/irq.h +4 -7

index 877c7e358384..3bece4379bd9 100644
@@ -25,16 +25,13 @@ DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);
  *       ------------
  *       |          |  <- irq_stack_ptr
  *   top ------------
- *       |  elr_el1 |
+ *       |   x19    | <- irq_stack_ptr - 0x08
  *       ------------
  *       |   x29    | <- irq_stack_ptr - 0x10
  *       ------------
- *       |   xzr    |
- *       ------------
- *       |   x19    | <- irq_stack_ptr - 0x20
- *       ------------
  *
- * where x19 holds a copy of the task stack pointer.
+ * where x19 holds a copy of the task stack pointer where the struct pt_regs
+ * from kernel_entry can be found.
  *
  */
 #define IRQ_STACK_PTR(cpu) ((unsigned long)per_cpu(irq_stack, cpu) + IRQ_STACK_START_SP)

@@ -43,7 +40,7 @@ DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);
  * The offset from irq_stack_ptr where entry.S will store the original
  * stack pointer. Used by unwind_frame() and dump_backtrace().
  */
-#define IRQ_STACK_TO_TASK_STACK(ptr) *((unsigned long *)(ptr - 0x20));
+#define IRQ_STACK_TO_TASK_STACK(ptr) (*((unsigned long *)((ptr) - 0x08)))
 
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

arch/arm64/kernel/entry.S +7 -5

index 2284c296e3f7..0667fb7d8bb1 100644
@@ -178,7 +178,7 @@ alternative_endif
 	mrs	\rd, sp_el0
 	.endm
 
-	.macro	irq_stack_entry, dummy_lr
+	.macro	irq_stack_entry
 	mov	x19, sp			// preserve the original sp
 
 	this_cpu_ptr irq_stack, x25, x26

@@ -196,10 +196,12 @@ alternative_endif
 	add	x26, x25, x26
 	mov	sp, x26
 
-	/* Add a dummy stack frame */
-	stp     x29, \dummy_lr, [sp, #-16]!           // dummy stack frame
+	/*
+	 * Add a dummy stack frame, this non-standard format is fixed up
+	 * by unwind_frame()
+	 */
+	stp     x29, x19, [sp, #-16]!
 	mov	x29, sp
-	stp     x19, xzr, [sp, #-16]!
 
 9998:
 	.endm

@@ -229,7 +231,7 @@ tsk	.req	x28		// current thread_info
 	.macro	irq_handler
 	ldr_l	x1, handle_arch_irq
 	mov	x0, sp
-	irq_stack_entry x22
+	irq_stack_entry
 	blr	x1
 	irq_stack_exit
 	.endm

arch/arm64/kernel/stacktrace.c +15 -2

index d916d5b6aef6..82659e1df761 100644
@@ -70,17 +70,30 @@ int notrace unwind_frame(struct stackframe *frame)
 	 * Check whether we are going to walk through from interrupt stack
 	 * to task stack.
 	 * If we reach the end of the stack - and its an interrupt stack,
-	 * read the original task stack pointer from the dummy frame.
+	 * unpack the dummy frame to find the original elr.
 	 *
 	 * Check the frame->fp we read from the bottom of the irq_stack,
 	 * and the original task stack pointer are both in current->stack.
 	 */
 	if (frame->sp == irq_stack_ptr) {
+		struct pt_regs *irq_args;
 		unsigned long orig_sp = IRQ_STACK_TO_TASK_STACK(irq_stack_ptr);
 
 		if(object_is_on_stack((void *)orig_sp) &&
-		   object_is_on_stack((void *)frame->fp))
+		   object_is_on_stack((void *)frame->fp)) {
 			frame->sp = orig_sp;
+
+			/* orig_sp is the saved pt_regs, find the elr */
+			irq_args = (struct pt_regs *)orig_sp;
+			frame->pc = irq_args->pc;
+		} else {
+			/*
+			 * This frame has a non-standard format, and we
+			 * didn't fix it, because the data looked wrong.
+			 * Refuse to output this frame.
+			 */
+			return -EINVAL;
+		}
 	}
 
 	return 0;

Signature · 1 line

2.6.2

James Morse

author

2015-12-18 16:01 UTC #38 lore raw

[PATCH v8 9/4] arm64: remove irq_count and do_softirq_own_stack()

In reply to James Morse

sysrq_handle_reboot() re-enables interrupts while on the irq stack. The irq_stack implementation wrongly assumed this would only ever happen via the softirq path, allowing it to update irq_count late, in do_softirq_own_stack().

This means if an irq occurs in sysrq_handle_reboot(), during emergency_restart() the stack will be corrupted, as irq_count wasn't updated.

Lose the optimisation, and instead of moving the adding/subtracting of irq_count into irq_stack_entry/irq_stack_exit, remove it, and compare sp_el0 (struct task_info) with sp & ~(THREAD_SIZE - 1). This tells us if we are on a task stack, if so, we can safely switch to the irq stack. Finally, remove do_softirq_own_stack(), we don't need it anymore.

Reported-by Will Deacon <[email protected]>

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/include/asm/irq.h |  2 --
 arch/arm64/kernel/entry.S    | 17 +++++++++--------
 arch/arm64/kernel/irq.c      | 38 +-------------------------------------
 3 files changed, 10 insertions(+), 47 deletions(-)

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

index 3bece4379bd9..b77197d941fc 100644
@@ -11,8 +11,6 @@
 #include <asm-generic/irq.h>
 #include <asm/thread_info.h>
 
-#define __ARCH_HAS_DO_SOFTIRQ
-
 struct pt_regs;
 
 DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);

arch/arm64/kernel/entry.S +9 -8

index 0667fb7d8bb1..6745a9041f99 100644
@@ -181,19 +181,20 @@ alternative_endif
 	.macro	irq_stack_entry
 	mov	x19, sp			// preserve the original sp
 
-	this_cpu_ptr irq_stack, x25, x26
-
 	/*
-	 * Check the lowest address on irq_stack for the irq_count value,
-	 * incremented by do_softirq_own_stack if we have re-enabled irqs
-	 * while on the irq_stack.
+	 * Compare sp and sp_el0, if the top ~(THREAD_SIZE - 1) bits match,
+	 * we are on a task stack, and should switch to the irq stack.
 	 */
-	ldr	x26, [x25]
-	cbnz	x26, 9998f		// recursive use?
+	mrs	x26, sp_el0		// already masked
+	and	x25, x19, #~(THREAD_SIZE - 1)
+	cmp	x25, x26
+	b.ne	9998f
 
-	/* switch to the irq stack */
+	this_cpu_ptr irq_stack, x25, x26
 	mov	x26, #IRQ_STACK_START_SP
 	add	x26, x25, x26
+
+	/* switch to the irq stack */
 	mov	sp, x26
 
 	/*

arch/arm64/kernel/irq.c +1 ~~-37~~

index ff7ebb710e51..2386b26c0712 100644
@@ -25,24 +25,14 @@
 #include <linux/irq.h>
 #include <linux/smp.h>
 #include <linux/init.h>
-#include <linux/interrupt.h>
 #include <linux/irqchip.h>
 #include <linux/seq_file.h>
 
 unsigned long irq_err_count;
 
-/*
- * irq stack only needs to be 16 byte aligned - not IRQ_STACK_SIZE aligned.
- * irq_stack[0] is used as irq_count, a non-zero value indicates the stack
- * is in use, and el?_irq() shouldn't switch to it. This is used to detect
- * recursive use of the irq_stack, it is lazily updated by
- * do_softirq_own_stack(), which is called on the irq_stack, before
- * re-enabling interrupts to process softirqs.
- */
+/* irq stack only needs to be 16 byte aligned - not IRQ_STACK_SIZE aligned. */
 DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(16);
 
-#define IRQ_COUNT()	(*per_cpu(irq_stack, smp_processor_id()))
-
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 	show_ipi_list(p, prec);

@@ -66,29 +56,3 @@ void __init init_IRQ(void)
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
 }
-
-/*
- * do_softirq_own_stack() is called from irq_exit() before __do_softirq()
- * re-enables interrupts, at which point we may re-enter el?_irq(). We
- * increase irq_count here so that el1_irq() knows that it is already on the
- * irq stack.
- *
- * Called with interrupts disabled, so we don't worry about moving cpu, or
- * being interrupted while modifying irq_count.
- *
- * This function doesn't actually switch stack.
- */
-void do_softirq_own_stack(void)
-{
-	int cpu = smp_processor_id();
-
-	WARN_ON_ONCE(!irqs_disabled());
-
-	if (on_irq_stack(current_stack_pointer, cpu)) {
-		IRQ_COUNT()++;
-		__do_softirq();
-		IRQ_COUNT()--;
-	} else {
-		__do_softirq();
-	}
-}

Signature · 1 line

2.6.2

Jungseok Lee

2015-12-20 11:07 UTC #39 lore raw

[PATCH v8 9/4] arm64: remove irq_count and do_softirq_own_stack()

On Dec 19, 2015, at 1:01 AM, James Morse wrote:

Hi James,

Quoted 5 lines

sysrq_handle_reboot() re-enables interrupts while on the irq stack. The irq_stack implementation wrongly assumed this would only ever happen via the softirq path, allowing it to update irq_count late, in do_softirq_own_stack().

Quoted 7 lines

This means if an irq occurs in sysrq_handle_reboot(), during emergency_restart() the stack will be corrupted, as irq_count wasn't updated.
Lose the optimisation, and instead of moving the adding/subtracting of irq_count into irq_stack_entry/irq_stack_exit, remove it, and compare sp_el0 (struct task_info) with sp & ~(THREAD_SIZE - 1). This tells us if

Nit: s/task_info/thread_info

Quoted 34 lines

we are on a task stack, if so, we can safely switch to the irq stack. Finally, remove do_softirq_own_stack(), we don't need it anymore.

Reported-by Will Deacon <[email protected]>

Signed-off-by James Morse <[email protected]>

---

arch/arm64/include/asm/irq.h |  2 --
arch/arm64/kernel/entry.S    | 17 +++++++++--------
arch/arm64/kernel/irq.c      | 38 +-------------------------------------
3 files changed, 10 insertions(+), 47 deletions(-)

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

index 3bece4379bd9..b77197d941fc 100644
@@ -11,8 +11,6 @@
 #include <asm-generic/irq.h>
 #include <asm/thread_info.h>
 
-#define __ARCH_HAS_DO_SOFTIRQ
-
 struct pt_regs;
 
 DECLARE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack);

arch/arm64/kernel/entry.S +0 -2

index 0667fb7d8bb1..6745a9041f99 100644
@@ -181,19 +181,20 @@ alternative_endif
 	.macro	irq_stack_entry
 	mov	x19, sp			// preserve the original sp
 
-	this_cpu_ptr irq_stack, x25, x26
-

arch/arm64/kernel/entry.S alternative_endif · line 188

Quoted 10 lines

arch/arm64/kernel/entry.S +3 -5

 	/*
-	 * Check the lowest address on irq_stack for the irq_count value,
-	 * incremented by do_softirq_own_stack if we have re-enabled irqs
-	 * while on the irq_stack.
+	 * Compare sp and sp_el0, if the top ~(THREAD_SIZE - 1) bits match,
+	 * we are on a task stack, and should switch to the irq stack.
 	 */
-	ldr	x26, [x25]
-	cbnz	x26, 9998f		// recursive use?
+	mrs	x26, sp_el0		// already masked

Nit: How about using 'get_thread_info x26'?

Quoted 63 lines

arch/arm64/kernel/entry.S +6 -1

+	and	x25, x19, #~(THREAD_SIZE - 1)
+	cmp	x25, x26
+	b.ne	9998f
 
-	/* switch to the irq stack */
+	this_cpu_ptr irq_stack, x25, x26
 	mov	x26, #IRQ_STACK_START_SP
 	add	x26, x25, x26
+
+	/* switch to the irq stack */
 	mov	sp, x26
 
 	/*

arch/arm64/kernel/irq.c +1 ~~-27~~

index ff7ebb710e51..2386b26c0712 100644
@@ -25,24 +25,14 @@
 #include <linux/irq.h>
 #include <linux/smp.h>
 #include <linux/init.h>
-#include <linux/interrupt.h>
 #include <linux/irqchip.h>
 #include <linux/seq_file.h>
 
 unsigned long irq_err_count;
 
-/*
- * irq stack only needs to be 16 byte aligned - not IRQ_STACK_SIZE aligned.
- * irq_stack[0] is used as irq_count, a non-zero value indicates the stack
- * is in use, and el?_irq() shouldn't switch to it. This is used to detect
- * recursive use of the irq_stack, it is lazily updated by
- * do_softirq_own_stack(), which is called on the irq_stack, before
- * re-enabling interrupts to process softirqs.
- */
+/* irq stack only needs to be 16 byte aligned - not IRQ_STACK_SIZE aligned. */
 DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(16);
 
-#define IRQ_COUNT()	(*per_cpu(irq_stack, smp_processor_id()))
-
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 	show_ipi_list(p, prec);

@@ -66,29 +56,3 @@ void __init init_IRQ(void)
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
 }
-
-/*
- * do_softirq_own_stack() is called from irq_exit() before __do_softirq()
- * re-enables interrupts, at which point we may re-enter el?_irq(). We
- * increase irq_count here so that el1_irq() knows that it is already on the
- * irq stack.
- *
- * Called with interrupts disabled, so we don't worry about moving cpu, or
- * being interrupted while modifying irq_count.
- *
- * This function doesn't actually switch stack.
- */
-void do_softirq_own_stack(void)
-{
-	int cpu = smp_processor_id();
-

arch/arm64/kernel/irq.c void __init init_IRQ(void) · line 94

Quoted 10 lines

arch/arm64/kernel/irq.c +0 ~~-10~~

-	WARN_ON_ONCE(!irqs_disabled());
-
-	if (on_irq_stack(current_stack_pointer, cpu)) {
-		IRQ_COUNT()++;
-		__do_softirq();
-		IRQ_COUNT()--;
-	} else {
-		__do_softirq();
-	}
-}

The approach, removal of the counter, looks good to me from a high level point of view although I have not played with this change seriously yet.

If I spot something strange, I will report it.

Best Regards
Jungseok Lee

Will Deacon

2015-12-21 11:30 UTC #40 lore raw

[PATCH v8 9/4] arm64: remove irq_count and do_softirq_own_stack()

Quoted 9 lines · Jungseok Lee

Quoted 8 lines · James Morse

arch/arm64/kernel/entry.S +0 -0

index 0667fb7d8bb1..6745a9041f99 100644
@@ -181,19 +181,20 @@ alternative_endif
 	.macro	irq_stack_entry
 	mov	x19, sp			// preserve the original sp

Quoted 14 lines

arch/arm64/kernel/entry.S alternative_endif · line 188

Quoted 12 lines

arch/arm64/kernel/entry.S +3 -7

-	this_cpu_ptr irq_stack, x25, x26
-
 	/*
-	 * Check the lowest address on irq_stack for the irq_count value,
-	 * incremented by do_softirq_own_stack if we have re-enabled irqs
-	 * while on the irq_stack.
+	 * Compare sp and sp_el0, if the top ~(THREAD_SIZE - 1) bits match,
+	 * we are on a task stack, and should switch to the irq stack.
 	 */
-	ldr	x26, [x25]
-	cbnz	x26, 9998f		// recursive use?
+	mrs	x26, sp_el0		// already masked

Nit: How about using 'get_thread_info x26'?

Something like the following?

Will

--->8

arch/arm64/kernel/entry.S +5 -5

index 6745a9041f99..c0db321db7e1 100644
@@ -182,12 +182,12 @@ alternative_endif
 	mov	x19, sp			// preserve the original sp
 
 	/*
-	 * Compare sp and sp_el0, if the top ~(THREAD_SIZE - 1) bits match,
-	 * we are on a task stack, and should switch to the irq stack.
+	 * Compare sp with the current thread_info, if the top
+	 * ~(THREAD_SIZE - 1) bits match, we are on a task stack, and
+	 * should switch to the irq stack.
 	 */
-	mrs	x26, sp_el0		// already masked
 	and	x25, x19, #~(THREAD_SIZE - 1)
-	cmp	x25, x26
+	cmp	x25, tsk
 	b.ne	9998f
 
 	this_cpu_ptr irq_stack, x25, x26

@@ -406,10 +406,10 @@ el1_irq:
 	bl	trace_hardirqs_off
 #endif
 
+	get_thread_info tsk
 	irq_handler
 
 #ifdef CONFIG_PREEMPT
-	get_thread_info tsk
 	ldr	w24, [tsk, #TI_PREEMPT]		// get preempt count
 	cbnz	w24, 1f				// preempt count != 0
 	ldr	x0, [tsk, #TI_FLAGS]		// get flags

James Morse

author

2015-12-21 12:19 UTC #41 lore raw

[PATCH v8 9/4] arm64: remove irq_count and do_softirq_own_stack()

Hi Will,

Quoted 5 lines · Will Deacon

Quoted 3 lines
Quoted 1 line
+ mrs x26, sp_el0 // already masked
Nit: How about using 'get_thread_info x26'?
Something like the following?

Looks fine - I guess you're cleverly leaving the the el0_irq case to be covered by get_thread_info in kernel_entry?

Thanks,

James

(Sorry if this message formatting is weird - I'm briefly stuck with webmail)
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Will Deacon

2015-12-21 12:21 UTC #42 lore raw

[PATCH v8 9/4] arm64: remove irq_count and do_softirq_own_stack()

Quoted 9 lines · James Morse

Quoted 5 lines · Will Deacon
Quoted 3 lines
Quoted 1 line
+ mrs x26, sp_el0 // already masked
Nit: How about using 'get_thread_info x26'?
Something like the following?
Looks fine - I guess you're cleverly leaving the the el0_irq case to be covered by get_thread_info in kernel_entry?

Yup. tsk is always valid for exceptions taken from el0, afaict.

Will

Jungseok Lee

2015-12-21 14:06 UTC #43 lore raw

[PATCH v8 9/4] arm64: remove irq_count and do_softirq_own_stack()

Quoted 4 lines · Will Deacon

Quoted 3 lines · James Morse
Quoted 2 lines · Will Deacon
Quoted 2 lines
Quoted 1 line
+ mrs x26, sp_el0 // already masked

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Nit: How about using 'get_thread_info x26'?
Something like the following?
Looks fine - I guess you're cleverly leaving the the el0_irq case to be covered by get_thread_info in kernel_entry?
Yup. tsk is always valid for exceptions taken from el0, afaict.

Good idea! I like the change.

Best Regards
Jungseok Lee