[PATCH v7 0/3] arm64: Add support for IRQ stack

[PATCH v7 0/3] arm64: Add support for IRQ stack

[James Morse]

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.

This series is split into three for reasons of (coarse) attribution, although
I've added bugs to all the patches...

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

Comments welcome,


James

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

James Morse (1):
  arm64: Add do_softirq_own_stack() and enable irq_stacks

Jungseok Lee (1):
  arm64: Add irq_stack boiler plate, store struct task_info in sp_el0

 arch/arm64/include/asm/irq.h         | 26 +++++++++++++++++++
 arch/arm64/include/asm/thread_info.h | 10 ++++++--
 arch/arm64/kernel/entry.S            | 48 ++++++++++++++++++++++++++++++++----
 arch/arm64/kernel/head.S             |  5 ++++
 arch/arm64/kernel/irq.c              | 47 +++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/sleep.S            |  3 +++
 arch/arm64/kernel/smp.c              |  5 ++++
 arch/arm64/kernel/stacktrace.c       | 29 ++++++++++++++++++++--
 arch/arm64/kernel/traps.c            | 14 ++++++++++-
 9 files changed, 177 insertions(+), 10 deletions(-)

-- 
2.1.4

[PATCH v7 1/3] arm64: Add irq_stack boiler plate, store struct task_info in sp_el0

[James Morse]

From: Jungseok Lee <jungseoklee85@gmail.com>

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.

This patch adds per_cpu definitions and initialisation for the irq_stack,
that will be used by a later patch.

Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
[Added per_cpu definitions and initialisation for irq_stack]
Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/irq.h         |  9 +++++++++
 arch/arm64/include/asm/thread_info.h | 10 ++++++++--
 arch/arm64/kernel/entry.S            | 18 +++++++++++++-----
 arch/arm64/kernel/head.S             |  5 +++++
 arch/arm64/kernel/irq.c              | 13 +++++++++++++
 arch/arm64/kernel/sleep.S            |  3 +++
 arch/arm64/kernel/smp.c              |  5 +++++
 7 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index 23eb450b820b..00cab2e28376 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -1,10 +1,19 @@
 #ifndef __ASM_IRQ_H
 #define __ASM_IRQ_H
 
+#include <linux/percpu.h>
+
 #include <asm-generic/irq.h>
+#include <asm/thread_info.h>
 
 struct pt_regs;
 
+DECLARE_PER_CPU(unsigned long, irq_stack_ptr);
+
+#define IRQ_STACK_SIZE			THREAD_SIZE
+#define IRQ_STACK_START_SP		THREAD_START_SP
+
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
 
+void init_irq_stack(unsigned int cpu);
 #endif
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 90c7ff233735..abd64bd1f6d9 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -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)	\
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 7ed3d75f6304..1971da98dfad 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -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
 
 /*
@@ -183,8 +190,7 @@ tsk	.req	x28		// current thread_info
  * Interrupt handling.
  */
 	.macro	irq_handler
-	adrp	x1, handle_arch_irq
-	ldr	x1, [x1, #:lo12:handle_arch_irq]
+	ldr_l	x1, handle_arch_irq
 	mov	x0, sp
 	blr	x1
 	.endm
@@ -599,6 +605,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)
 
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 23cfc08fc8ba..b363f340f2c7 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -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)
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 9f17ec071ee0..da752bb18bfb 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -30,6 +30,10 @@
 
 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);
+DEFINE_PER_CPU(unsigned long, irq_stack_ptr);
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 	show_ipi_list(p, prec);
@@ -49,7 +53,16 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 
 void __init init_IRQ(void)
 {
+	init_irq_stack(smp_processor_id());
+
 	irqchip_init();
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
 }
+
+void init_irq_stack(unsigned int cpu)
+{
+	unsigned long stack = (unsigned long)per_cpu(irq_stack, cpu);
+
+	per_cpu(irq_stack_ptr, cpu) = stack + IRQ_STACK_START_SP;
+}
diff --git a/arch/arm64/kernel/sleep.S b/arch/arm64/kernel/sleep.S
index f586f7c875e2..e33fe33876ab 100644
--- a/arch/arm64/kernel/sleep.S
+++ b/arch/arm64/kernel/sleep.S
@@ -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
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index b1adc51b2c2e..65fd164a372b 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -98,6 +98,11 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
 	__flush_dcache_area(&secondary_data, sizeof(secondary_data));
 
 	/*
+	 * Initialise IRQ stack to handle interrupts.
+	 */
+	init_irq_stack(cpu);
+
+	/*
 	 * Now bring the CPU into our world.
 	 */
 	ret = boot_secondary(cpu, idle);
-- 
2.1.4

[PATCH v7 2/3] arm64: Modify stack trace and dump for use with irq_stack

[James Morse]

From: AKASHI Takahiro <takahiro.akashi@linaro.org>

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 <takahiro.akashi@linaro.org>
[merged two patches, reworked for per_cpu irq_stacks, and no alignment
 guarantees]
Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/irq.h   | 15 +++++++++++++++
 arch/arm64/kernel/stacktrace.c | 29 +++++++++++++++++++++++++++--
 arch/arm64/kernel/traps.c      | 14 +++++++++++++-
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index 00cab2e28376..bf823c5f8cbd 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -13,7 +13,22 @@ DECLARE_PER_CPU(unsigned long, irq_stack_ptr);
 #define IRQ_STACK_SIZE			THREAD_SIZE
 #define IRQ_STACK_START_SP		THREAD_START_SP
 
+/*
+ * This is 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(x)	*((unsigned long *)(x - 0x10));
+
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
 
 void init_irq_stack(unsigned int cpu);
+
+static inline bool on_irq_stack(unsigned long sp, int cpu)
+{
+	/* variable names the same as kernel/stacktrace.c */
+	unsigned long high = per_cpu(irq_stack_ptr, cpu);
+	unsigned long low = high - IRQ_STACK_START_SP;
+
+	return (low <= sp && sp <= high);
+}
 #endif
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ccb6078ed9f2..a15985137328 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -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 = per_cpu(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;
 }
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index e9b9b5364393..cdfa2f9e8d59 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -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 = per_cpu(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);
+		}
 	}
 }
 
-- 
2.1.4

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[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-enabled interrupts to process softirqs.

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

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/irq.h |  2 ++
 arch/arm64/kernel/entry.S    | 30 ++++++++++++++++++++++++++++++
 arch/arm64/kernel/irq.c      | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+)

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index bf823c5f8cbd..04aae95dee8d 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -6,6 +6,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_ptr);
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 1971da98dfad..45473838fe21 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -175,6 +175,34 @@ alternative_endif
 	mrs	\rd, sp_el0
 	.endm
 
+	.macro	irq_stack_entry
+	mov	x19, sp			// preserve the original sp
+	adr_l   x25, irq_count		// incremented by do_softirq_own_stack()
+	mrs	x26, tpidr_el1
+	add	x25, x25, x26
+	ldr	w25, [x25]
+	cbnz	w25, 1f			// recursive use?
+
+	/* switch to the irq stack */
+	adr_l	x25, irq_stack_ptr
+	add	x25, x25, x26
+	ldr	x25, [x25]
+	mov	sp, x25
+
+	/* Add a dummy stack frame */
+	stp     x29, x22, [sp, #-16]!           // dummy stack frame
+	mov	x29, sp
+	stp     xzr, x19, [sp, #-16]!
+1:
+	.endm
+
+	/*
+	 * x19 is preserved between irq_stack_entry and irq_stack_exit.
+	 */
+	.macro	irq_stack_exit
+	mov	sp, x19
+	.endm
+
 /*
  * These are the registers used in the syscall handler, and allow us to
  * have in theory up to 7 arguments to a function - x0 to x6.
@@ -192,7 +220,9 @@ tsk	.req	x28		// current thread_info
 	.macro	irq_handler
 	ldr_l	x1, handle_arch_irq
 	mov	x0, sp
+	irq_stack_entry
 	blr	x1
+	irq_stack_exit
 	.endm
 
 	.text
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index da752bb18bfb..838541cf5e5d 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -25,6 +25,7 @@
 #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>
 
@@ -34,6 +35,13 @@ unsigned long irq_err_count;
 DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(16);
 DEFINE_PER_CPU(unsigned long, irq_stack_ptr);
 
+/*
+ * irq_count is used to detect recursive use of the irq_stack, it is lazily
+ * incremented very late, by do_softirq_own_stack(), which is called on the
+ * irq_stack, before re-enabling interrupts to process softirqs.
+ */
+DEFINE_PER_CPU(unsigned int, irq_count);
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 	show_ipi_list(p, prec);
@@ -66,3 +74,29 @@ void init_irq_stack(unsigned int cpu)
 
 	per_cpu(irq_stack_ptr, cpu) = stack + IRQ_STACK_START_SP;
 }
+
+/*
+ * 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)) {
+		per_cpu(irq_count, cpu)++;
+		__do_softirq();
+		per_cpu(irq_count, cpu)--;
+	} else {
+		__do_softirq();
+	}
+}
-- 
2.1.4

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[Jungseok Lee]

On Nov 17, 2015, at 3:22 AM, James Morse wrote:

Hi James,

First of all, thanks for this work!

> 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-enabled interrupts to process softirqs.
> 
> This patch adds the dummy stack frame and data needed by the previous
> stack tracing patches.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> arch/arm64/include/asm/irq.h |  2 ++
> arch/arm64/kernel/entry.S    | 30 ++++++++++++++++++++++++++++++
> arch/arm64/kernel/irq.c      | 34 ++++++++++++++++++++++++++++++++++
> 3 files changed, 66 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index bf823c5f8cbd..04aae95dee8d 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -6,6 +6,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_ptr);
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 1971da98dfad..45473838fe21 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -175,6 +175,34 @@ alternative_endif
> 	mrs	\rd, sp_el0
> 	.endm
> 
> +	.macro	irq_stack_entry
> +	mov	x19, sp			// preserve the original sp
> +	adr_l   x25, irq_count		// incremented by do_softirq_own_stack()
> +	mrs	x26, tpidr_el1
> +	add	x25, x25, x26
> +	ldr	w25, [x25]
> +	cbnz	w25, 1f			// recursive use?
> +
> +	/* switch to the irq stack */
> +	adr_l	x25, irq_stack_ptr
> +	add	x25, x25, x26
> +	ldr	x25, [x25]
> +	mov	sp, x25
> +
> +	/* Add a dummy stack frame */
> +	stp     x29, x22, [sp, #-16]!           // dummy stack frame
> +	mov	x29, sp
> +	stp     xzr, x19, [sp, #-16]!
> +1:
> +	.endm
> +
> +	/*
> +	 * x19 is preserved between irq_stack_entry and irq_stack_exit.
> +	 */
> +	.macro	irq_stack_exit
> +	mov	sp, x19
> +	.endm
> +
> /*
>  * These are the registers used in the syscall handler, and allow us to
>  * have in theory up to 7 arguments to a function - x0 to x6.
> @@ -192,7 +220,9 @@ tsk	.req	x28		// current thread_info
> 	.macro	irq_handler
> 	ldr_l	x1, handle_arch_irq
> 	mov	x0, sp
> +	irq_stack_entry
> 	blr	x1
> +	irq_stack_exit
> 	.endm
> 
> 	.text
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index da752bb18bfb..838541cf5e5d 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -25,6 +25,7 @@
> #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>
> 
> @@ -34,6 +35,13 @@ unsigned long irq_err_count;
> DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(16);
> DEFINE_PER_CPU(unsigned long, irq_stack_ptr);
> 
> +/*
> + * irq_count is used to detect recursive use of the irq_stack, it is lazily
> + * incremented very late, by do_softirq_own_stack(), which is called on the
> + * irq_stack, before re-enabling interrupts to process softirqs.
> + */
> +DEFINE_PER_CPU(unsigned int, irq_count);
> +
> int arch_show_interrupts(struct seq_file *p, int prec)
> {
> 	show_ipi_list(p, prec);
> @@ -66,3 +74,29 @@ void init_irq_stack(unsigned int cpu)
> 
> 	per_cpu(irq_stack_ptr, cpu) = stack + IRQ_STACK_START_SP;
> }
> +
> +/*
> + * 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)) {
> +		per_cpu(irq_count, cpu)++;
> +		__do_softirq();
> +		per_cpu(irq_count, cpu)--;
> +	} else {
> +		__do_softirq();
> +	}
> +}

I'm really interested in feedbacks from other folks since, as you know
well, softirq could be handled using a process stack under this design.

Best Regards
Jungseok Lee

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[Catalin Marinas]

On Thu, Nov 19, 2015 at 11:26:08PM +0900, Jungseok Lee wrote:
> > +/*
> > + * 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)) {
> > +		per_cpu(irq_count, cpu)++;
> > +		__do_softirq();
> > +		per_cpu(irq_count, cpu)--;
> > +	} else {
> > +		__do_softirq();
> > +	}
> > +}
> 
> I'm really interested in feedbacks from other folks since, as you know
> well, softirq could be handled using a process stack under this design.

With this approach, we could run softirq either on the IRQ stack or on
the process stack. One such process is softirqd, so there shouldn't be a
problem since its goal is to run softirqs. But do_softirq() may be
invoked from other contexts (network stack), I'm not sure what the stack
looks like at that point.

We could just do like x86 and always switch to the IRQ stack before
invoking __do_softirq(). I don't think we lose much.

-- 
Catalin

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[Catalin Marinas]

On Mon, Nov 16, 2015 at 06:22:07PM +0000, James Morse wrote:
>  DECLARE_PER_CPU(unsigned long, irq_stack_ptr);
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 1971da98dfad..45473838fe21 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -175,6 +175,34 @@ alternative_endif
>  	mrs	\rd, sp_el0
>  	.endm
>  
> +	.macro	irq_stack_entry
> +	mov	x19, sp			// preserve the original sp
> +	adr_l   x25, irq_count		// incremented by do_softirq_own_stack()
> +	mrs	x26, tpidr_el1
> +	add	x25, x25, x26
> +	ldr	w25, [x25]
> +	cbnz	w25, 1f			// recursive use?
> +
> +	/* switch to the irq stack */
> +	adr_l	x25, irq_stack_ptr
> +	add	x25, x25, x26
> +	ldr	x25, [x25]
> +	mov	sp, x25

Do we need a separate pointer for irq_stack? Could we not just use the
address of the irq_stack array directly since it's statically declared
already and just add IRQ_STACK_START_SP?

> +	/* Add a dummy stack frame */
> +	stp     x29, x22, [sp, #-16]!           // dummy stack frame
> +	mov	x29, sp
> +	stp     xzr, x19, [sp, #-16]!
> +1:
> +	.endm

Just a nitpick, use some high number for the label (e.g. 9998) or use a
named one, in case we ever get similar labels around the macro use.

> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index da752bb18bfb..838541cf5e5d 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -25,6 +25,7 @@
>  #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>
>  
> @@ -34,6 +35,13 @@ unsigned long irq_err_count;
>  DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(16);
>  DEFINE_PER_CPU(unsigned long, irq_stack_ptr);
>  
> +/*
> + * irq_count is used to detect recursive use of the irq_stack, it is lazily
> + * incremented very late, by do_softirq_own_stack(), which is called on the
> + * irq_stack, before re-enabling interrupts to process softirqs.
> + */
> +DEFINE_PER_CPU(unsigned int, irq_count);

We can keep irq_count in a union in the IRQ stack (at the other end of
the array), the code may be marginally shorter (you avoid another
adrp+adr+add vs an "and" with the IRQ_STACK_SIZE

Either that or, if we still need irq_stack_ptr, put both in the same
struct to avoid two adrp+adr+add for retrieving the address.

> +void do_softirq_own_stack(void)
> +{
> +	int cpu = smp_processor_id();
> +
> +	WARN_ON_ONCE(!irqs_disabled());
> +
> +	if (on_irq_stack(current_stack_pointer, cpu)) {
> +		per_cpu(irq_count, cpu)++;
> +		__do_softirq();
> +		per_cpu(irq_count, cpu)--;
> +	} else {
> +		__do_softirq();
> +	}
> +}

If we go for softirq always on the IRQ stack, we should move this to
assembly and reuse the irq_stack_entry/exit macros above.

-- 
Catalin

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[Jungseok Lee]

On Nov 27, 2015, at 8:47 PM, Catalin Marinas wrote:

Hi Catalin,

> On Thu, Nov 19, 2015 at 11:26:08PM +0900, Jungseok Lee wrote:
>>> +/*
>>> + * 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)) {
>>> +		per_cpu(irq_count, cpu)++;
>>> +		__do_softirq();
>>> +		per_cpu(irq_count, cpu)--;
>>> +	} else {
>>> +		__do_softirq();
>>> +	}
>>> +}
>> 
>> I'm really interested in feedbacks from other folks since, as you know
>> well, softirq could be handled using a process stack under this design.
> 
> With this approach, we could run softirq either on the IRQ stack or on
> the process stack. One such process is softirqd, so there shouldn't be a
> problem since its goal is to run softirqs. But do_softirq() may be
> invoked from other contexts (network stack), I'm not sure what the stack
> looks like at that point.
> 
> We could just do like x86 and always switch to the IRQ stack before
> invoking __do_softirq(). I don't think we lose much.

The lazy update has an obvious advantage in case of softirqd as avoiding
stack switch cost. OTOH, debugging (e.g register dump from stack) would
be more difficult since we have to find out which stack was used at the
last snapshot. Another point is the network context you mentioned. I can't
get an answer on the following question: Could we cover all network
scenarios with this approach which differs from x86 one?

The below is "always switch" implementation for reference. A couple of
comments you've left have been already addressed. I will post it if
"always switch" method is picked up ;)

Best Regards
Jungseok Lee

----8<----

 arch/arm64/Kconfig              |  1 +
 arch/arm64/include/asm/irq.h    | 16 +++++++
 arch/arm64/kernel/asm-offsets.c |  2 +
 arch/arm64/kernel/entry.S       | 53 ++++++++++++++++++++-
 arch/arm64/kernel/irq.c         |  2 +
 arch/arm64/kernel/smp.c         |  4 +-
 6 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 9ac16a4..a12f015 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -70,6 +70,7 @@ config ARM64
 	select HAVE_FUNCTION_GRAPH_TRACER
 	select HAVE_GENERIC_DMA_COHERENT
 	select HAVE_HW_BREAKPOINT if PERF_EVENTS
+	select HAVE_IRQ_EXIT_ON_IRQ_STACK
 	select HAVE_MEMBLOCK
 	select HAVE_PATA_PLATFORM
 	select HAVE_PERF_EVENTS
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index 23eb450..d4cbae6 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -1,10 +1,26 @@
 #ifndef __ASM_IRQ_H
 #define __ASM_IRQ_H
 
+#define IRQ_STACK_SIZE		16384
+
+#ifndef __ASSEMBLY__
+
 #include <asm-generic/irq.h>
 
+#define __ARCH_HAS_DO_SOFTIRQ
+
+struct irq_stack {
+	unsigned int count;
+	/*
+	 * The stack must be quad-word aligned according to '5.2.2 The stack'
+	 * of 'Procedure Call Standard for the ARM 64-bit Architecture'.
+	 */
+	char stack[IRQ_STACK_SIZE] __aligned(16);
+};
+
 struct pt_regs;
 
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
 
 #endif
+#endif
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 25de8b2..f8b4df7 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -41,6 +41,8 @@ int main(void)
   BLANK();
   DEFINE(THREAD_CPU_CONTEXT,	offsetof(struct task_struct, thread.cpu_context));
   BLANK();
+  DEFINE(IRQ_STACK,		offsetof(struct irq_stack, stack));
+  BLANK();
   DEFINE(S_X0,			offsetof(struct pt_regs, regs[0]));
   DEFINE(S_X1,			offsetof(struct pt_regs, regs[1]));
   DEFINE(S_X2,			offsetof(struct pt_regs, regs[2]));
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index fc87373..bc01102 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -27,9 +27,12 @@
 #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>
 
+#define IRQ_STACK_START_SP	(IRQ_STACK + IRQ_STACK_SIZE - 16)
+
 /*
  * Context tracking subsystem.  Used to instrument transitions
  * between user and kernel mode.
@@ -175,6 +178,29 @@ alternative_endif
 	mrs	\rd, sp_el0
 	.endm
 
+	.macro	irq_stack_entry
+	adr_l	x19, irq_stacks
+	mrs	x20, tpidr_el1
+	add	x20, x19, x20
+	mov	x19, sp
+	ldr	w23, [x20]
+	cbnz	w23, 1f				// check irq re-entrance
+	mov	x24, #IRQ_STACK_START_SP
+	add	x24, x20, x24			// x24 = top of irq stack
+	mov	sp, x24
+1:	add	w23, w23, #1
+	str	w23, [x20]
+	.endm
+
+	/*
+	 * x19, x20, w23 are preserved between irq_stack_{entry|exit}.
+	 */
+	.macro	irq_stack_exit
+	sub	w23, w23, #1
+	str	w23, [x20]
+	mov	sp, x19
+	.endm
+
 /*
  * These are the registers used in the syscall handler, and allow us to
  * have in theory up to 7 arguments to a function - x0 to x6.
@@ -190,10 +216,11 @@ tsk	.req	x28		// current thread_info
  * Interrupt handling.
  */
 	.macro	irq_handler
-	adrp	x1, handle_arch_irq
-	ldr	x1, [x1, #:lo12:handle_arch_irq]
+	ldr_l	x1, handle_arch_irq
 	mov	x0, sp
+	irq_stack_entry
 	blr	x1
+	irq_stack_exit
 	.endm
 
 	.text
@@ -741,3 +768,25 @@ ENTRY(sys_rt_sigreturn_wrapper)
 	mov	x0, sp
 	b	sys_rt_sigreturn
 ENDPROC(sys_rt_sigreturn_wrapper)
+
+/*
+ * Softirq is handled on IRQ stack.
+ */
+ENTRY(do_softirq_own_stack)
+	stp	x29, lr, [sp, #-96]!
+	stp	x19, x20, [sp, #16]
+	stp	x21, x22, [sp, #32]
+	stp	x23, x24, [sp, #48]
+	stp	x25, x26, [sp, #64]
+	stp	x27, x28, [sp, #80]
+	irq_stack_entry
+	bl	__do_softirq
+	irq_stack_exit
+	ldp	x19, x20, [sp, #16]
+	ldp	x21, x22, [sp, #32]
+	ldp	x23, x24, [sp, #48]
+	ldp	x25, x26, [sp, #64]
+	ldp	x27, x28, [sp, #80]
+	ldp	x29, lr, [sp], #96
+	ret
+ENDPROC(do_softirq_own_stack)
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 9f17ec0..6094ec8 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -30,6 +30,8 @@
 
 unsigned long irq_err_count;
 
+DEFINE_PER_CPU(struct irq_stack, irq_stacks);
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 	show_ipi_list(p, prec);
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index b1adc51..532a11f 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -91,8 +91,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
 	int ret;
 
 	/*
-	 * We need to tell the secondary core where to find its stack and the
-	 * page tables.
+	 * We need to tell the secondary core where to find its process stack
+	 * and the page tables.
 	 */
 	secondary_data.stack = task_stack_page(idle) + THREAD_START_SP;
 	__flush_dcache_area(&secondary_data, sizeof(secondary_data));
-- 
2.5.0

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[Catalin Marinas]

On Sat, Nov 28, 2015 at 12:37:31AM +0900, Jungseok Lee wrote:
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 9ac16a4..a12f015 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -70,6 +70,7 @@ config ARM64
>  	select HAVE_FUNCTION_GRAPH_TRACER
>  	select HAVE_GENERIC_DMA_COHERENT
>  	select HAVE_HW_BREAKPOINT if PERF_EVENTS
> +	select HAVE_IRQ_EXIT_ON_IRQ_STACK

I'm not convinced about this. See below.

>  	select HAVE_MEMBLOCK
>  	select HAVE_PATA_PLATFORM
>  	select HAVE_PERF_EVENTS
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index 23eb450..d4cbae6 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -1,10 +1,26 @@
>  #ifndef __ASM_IRQ_H
>  #define __ASM_IRQ_H
>  
> +#define IRQ_STACK_SIZE		16384

8K should be enough.

> +
> +#ifndef __ASSEMBLY__
> +
>  #include <asm-generic/irq.h>
>  
> +#define __ARCH_HAS_DO_SOFTIRQ
> +
> +struct irq_stack {
> +	unsigned int count;
> +	/*
> +	 * The stack must be quad-word aligned according to '5.2.2 The stack'
> +	 * of 'Procedure Call Standard for the ARM 64-bit Architecture'.
> +	 */
> +	char stack[IRQ_STACK_SIZE] __aligned(16);
> +};

This looks fine.

> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
> index 25de8b2..f8b4df7 100644
> --- a/arch/arm64/kernel/asm-offsets.c
> +++ b/arch/arm64/kernel/asm-offsets.c
> @@ -41,6 +41,8 @@ int main(void)
>    BLANK();
>    DEFINE(THREAD_CPU_CONTEXT,	offsetof(struct task_struct, thread.cpu_context));
>    BLANK();
> +  DEFINE(IRQ_STACK,		offsetof(struct irq_stack, stack));
> +  BLANK();
>    DEFINE(S_X0,			offsetof(struct pt_regs, regs[0]));
>    DEFINE(S_X1,			offsetof(struct pt_regs, regs[1]));
>    DEFINE(S_X2,			offsetof(struct pt_regs, regs[2]));
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index fc87373..bc01102 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -27,9 +27,12 @@
>  #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>
>  
> +#define IRQ_STACK_START_SP	(IRQ_STACK + IRQ_STACK_SIZE - 16)
> +
>  /*
>   * Context tracking subsystem.  Used to instrument transitions
>   * between user and kernel mode.
> @@ -175,6 +178,29 @@ alternative_endif
>  	mrs	\rd, sp_el0
>  	.endm
>  
> +	.macro	irq_stack_entry
> +	adr_l	x19, irq_stacks
> +	mrs	x20, tpidr_el1
> +	add	x20, x19, x20
> +	mov	x19, sp
> +	ldr	w23, [x20]
> +	cbnz	w23, 1f				// check irq re-entrance
> +	mov	x24, #IRQ_STACK_START_SP
> +	add	x24, x20, x24			// x24 = top of irq stack
> +	mov	sp, x24
> +1:	add	w23, w23, #1
> +	str	w23, [x20]
> +	.endm
> +
> +	/*
> +	 * x19, x20, w23 are preserved between irq_stack_{entry|exit}.
> +	 */
> +	.macro	irq_stack_exit
> +	sub	w23, w23, #1
> +	str	w23, [x20]
> +	mov	sp, x19
> +	.endm

With your approach to select HAVE_IRQ_EXIT_ON_IRQ_STACK you need to
always update the irq_count every time you enter or exit the IRQ. Since
presumably softirqs are rarer than hardirqs, I suggest that you only
increment/decrement irq_count via do_softirq_own_stack() and not select
HAVE_IRQ_EXIT_ON_IRQ_STACK. The only downside is having to check whether
SP is the IRQ stack (the irq_count) in this function before invoking
__do_softirq() rather than calling __do_softirq() directly. But it
doesn't look too bad.

> @@ -190,10 +216,11 @@ tsk	.req	x28		// current thread_info
>   * Interrupt handling.
>   */
>  	.macro	irq_handler
> -	adrp	x1, handle_arch_irq
> -	ldr	x1, [x1, #:lo12:handle_arch_irq]
> +	ldr_l	x1, handle_arch_irq
>  	mov	x0, sp
> +	irq_stack_entry
>  	blr	x1
> +	irq_stack_exit
>  	.endm
>  
>  	.text
> @@ -741,3 +768,25 @@ ENTRY(sys_rt_sigreturn_wrapper)
>  	mov	x0, sp
>  	b	sys_rt_sigreturn
>  ENDPROC(sys_rt_sigreturn_wrapper)
> +
> +/*
> + * Softirq is handled on IRQ stack.
> + */
> +ENTRY(do_softirq_own_stack)
> +	stp	x29, lr, [sp, #-96]!
> +	stp	x19, x20, [sp, #16]
> +	stp	x21, x22, [sp, #32]
> +	stp	x23, x24, [sp, #48]
> +	stp	x25, x26, [sp, #64]
> +	stp	x27, x28, [sp, #80]
> +	irq_stack_entry
> +	bl	__do_softirq
> +	irq_stack_exit
> +	ldp	x19, x20, [sp, #16]
> +	ldp	x21, x22, [sp, #32]
> +	ldp	x23, x24, [sp, #48]
> +	ldp	x25, x26, [sp, #64]
> +	ldp	x27, x28, [sp, #80]
> +	ldp	x29, lr, [sp], #96
> +	ret
> +ENDPROC(do_softirq_own_stack)

Since irq_stack_entry/exit shouldn't increment irq_count as I mentioned
above, you can hand-code the stack switching (or maybe parametrise the
irq_stack_* macros) to only use caller-saved registers and avoid too
many stp/ldp, probably just one for fp/lr.

-- 
Catalin

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[Jungseok Lee]

On Nov 28, 2015, at 2:38 AM, Catalin Marinas wrote:
> On Sat, Nov 28, 2015 at 12:37:31AM +0900, Jungseok Lee wrote:
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 9ac16a4..a12f015 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -70,6 +70,7 @@ config ARM64
>> 	select HAVE_FUNCTION_GRAPH_TRACER
>> 	select HAVE_GENERIC_DMA_COHERENT
>> 	select HAVE_HW_BREAKPOINT if PERF_EVENTS
>> +	select HAVE_IRQ_EXIT_ON_IRQ_STACK
> 
> I'm not convinced about this. See below.
> 
>> 	select HAVE_MEMBLOCK
>> 	select HAVE_PATA_PLATFORM
>> 	select HAVE_PERF_EVENTS
>> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
>> index 23eb450..d4cbae6 100644
>> --- a/arch/arm64/include/asm/irq.h
>> +++ b/arch/arm64/include/asm/irq.h
>> @@ -1,10 +1,26 @@
>> #ifndef __ASM_IRQ_H
>> #define __ASM_IRQ_H
>> 
>> +#define IRQ_STACK_SIZE		16384
> 
> 8K should be enough.

Agree.

>> +
>> +#ifndef __ASSEMBLY__
>> +
>> #include <asm-generic/irq.h>
>> 
>> +#define __ARCH_HAS_DO_SOFTIRQ
>> +
>> +struct irq_stack {
>> +	unsigned int count;
>> +	/*
>> +	 * The stack must be quad-word aligned according to '5.2.2 The stack'
>> +	 * of 'Procedure Call Standard for the ARM 64-bit Architecture'.
>> +	 */
>> +	char stack[IRQ_STACK_SIZE] __aligned(16);
>> +};
> 
> This looks fine.
> 
>> diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
>> index 25de8b2..f8b4df7 100644
>> --- a/arch/arm64/kernel/asm-offsets.c
>> +++ b/arch/arm64/kernel/asm-offsets.c
>> @@ -41,6 +41,8 @@ int main(void)
>>   BLANK();
>>   DEFINE(THREAD_CPU_CONTEXT,	offsetof(struct task_struct, thread.cpu_context));
>>   BLANK();
>> +  DEFINE(IRQ_STACK,		offsetof(struct irq_stack, stack));
>> +  BLANK();
>>   DEFINE(S_X0,			offsetof(struct pt_regs, regs[0]));
>>   DEFINE(S_X1,			offsetof(struct pt_regs, regs[1]));
>>   DEFINE(S_X2,			offsetof(struct pt_regs, regs[2]));
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index fc87373..bc01102 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -27,9 +27,12 @@
>> #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>
>> 
>> +#define IRQ_STACK_START_SP	(IRQ_STACK + IRQ_STACK_SIZE - 16)
>> +
>> /*
>>  * Context tracking subsystem.  Used to instrument transitions
>>  * between user and kernel mode.
>> @@ -175,6 +178,29 @@ alternative_endif
>> 	mrs	\rd, sp_el0
>> 	.endm
>> 
>> +	.macro	irq_stack_entry
>> +	adr_l	x19, irq_stacks
>> +	mrs	x20, tpidr_el1
>> +	add	x20, x19, x20
>> +	mov	x19, sp
>> +	ldr	w23, [x20]
>> +	cbnz	w23, 1f				// check irq re-entrance
>> +	mov	x24, #IRQ_STACK_START_SP
>> +	add	x24, x20, x24			// x24 = top of irq stack
>> +	mov	sp, x24
>> +1:	add	w23, w23, #1
>> +	str	w23, [x20]
>> +	.endm
>> +
>> +	/*
>> +	 * x19, x20, w23 are preserved between irq_stack_{entry|exit}.
>> +	 */
>> +	.macro	irq_stack_exit
>> +	sub	w23, w23, #1
>> +	str	w23, [x20]
>> +	mov	sp, x19
>> +	.endm
> 
> With your approach to select HAVE_IRQ_EXIT_ON_IRQ_STACK you need to
> always update the irq_count every time you enter or exit the IRQ. Since
> presumably softirqs are rarer than hardirqs, I suggest that you only
> increment/decrement irq_count via do_softirq_own_stack() and not select
> HAVE_IRQ_EXIT_ON_IRQ_STACK. The only downside is having to check whether
> SP is the IRQ stack (the irq_count) in this function before invoking
> __do_softirq() rather than calling __do_softirq() directly. But it
> doesn't look too bad.

Aha!!

I clearly understand why irq_count update in do_softirq_own_stack() is
a good approach. It would be really great to add the clause, "Since
presumably softirqs are rarer than hardirqs", to this commit message.
It explains why irq_count is updated in do_softirq_own_stack(), not elX_irq
context. IOW, I believe that it is a critical background on the design.

Now, I vote in favor of not selecting HAVE_IRQ_EXIT_ON_IRQ_STACK and this
lazy irq_count update.

>> @@ -190,10 +216,11 @@ tsk	.req	x28		// current thread_info
>>  * Interrupt handling.
>>  */
>> 	.macro	irq_handler
>> -	adrp	x1, handle_arch_irq
>> -	ldr	x1, [x1, #:lo12:handle_arch_irq]
>> +	ldr_l	x1, handle_arch_irq
>> 	mov	x0, sp
>> +	irq_stack_entry
>> 	blr	x1
>> +	irq_stack_exit
>> 	.endm
>> 
>> 	.text
>> @@ -741,3 +768,25 @@ ENTRY(sys_rt_sigreturn_wrapper)
>> 	mov	x0, sp
>> 	b	sys_rt_sigreturn
>> ENDPROC(sys_rt_sigreturn_wrapper)
>> +
>> +/*
>> + * Softirq is handled on IRQ stack.
>> + */
>> +ENTRY(do_softirq_own_stack)
>> +	stp	x29, lr, [sp, #-96]!
>> +	stp	x19, x20, [sp, #16]
>> +	stp	x21, x22, [sp, #32]
>> +	stp	x23, x24, [sp, #48]
>> +	stp	x25, x26, [sp, #64]
>> +	stp	x27, x28, [sp, #80]
>> +	irq_stack_entry
>> +	bl	__do_softirq
>> +	irq_stack_exit
>> +	ldp	x19, x20, [sp, #16]
>> +	ldp	x21, x22, [sp, #32]
>> +	ldp	x23, x24, [sp, #48]
>> +	ldp	x25, x26, [sp, #64]
>> +	ldp	x27, x28, [sp, #80]
>> +	ldp	x29, lr, [sp], #96
>> +	ret
>> +ENDPROC(do_softirq_own_stack)
> 
> Since irq_stack_entry/exit shouldn't increment irq_count as I mentioned
> above, you can hand-code the stack switching (or maybe parametrise the
> irq_stack_* macros) to only use caller-saved registers and avoid too
> many stp/ldp, probably just one for fp/lr.

Agree. This part should be reworked under the lazy update approach.

Best Regards
Jungseok Lee

[PATCH v7 1/3] arm64: Add irq_stack boiler plate, store struct task_info in sp_el0

[Jungseok Lee]

On Nov 17, 2015, at 3:22 AM, James Morse wrote:

Hi James,

> From: Jungseok Lee <jungseoklee85@gmail.com>
> 
> 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.
> 
> This patch adds per_cpu definitions and initialisation for the irq_stack,
> that will be used by a later patch.
> 
> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
> [Added per_cpu definitions and initialisation for irq_stack]

How about moving all irq_stack items to [PATCH 2/3] or [PATCH 3/3]?

First of all, I'd like to focus on only thread_info management, which has
no objection yet, via this patch. Secondly, this hunk is now considered
a common part for IRQ stack written by both of us. If a process stack is
allowed in softirq context, I will wait for your next version. If "always
switch" method is picked up, I'd like to write down the feature based on
this patch.

Best Regards
Jungseok Lee

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[James Morse]

On 27/11/15 11:47, Catalin Marinas wrote:
> With this approach, we could run softirq either on the IRQ stack or on
> the process stack. One such process is softirqd, so there shouldn't be a
> problem since its goal is to run softirqs. But do_softirq() may be
> invoked from other contexts (network stack), I'm not sure what the stack
> looks like at that point.

As long as we don't reduce the stack sizes yet - nothing has changed. We
already run __do_softirq() on other task's stacks via __irq_exit().


> We could just do like x86 and always switch to the IRQ stack before
> invoking __do_softirq(). I don't think we lose much.

I will add an extra patch to do this - its not as straight forward as I
would like - as both ksoftirqd and softirq on irq_stack call
do_softirq_own_stack() with irq_count == 0.


Thanks,

James

[PATCH v7 1/3] arm64: Add irq_stack boiler plate, store struct task_info in sp_el0

[James Morse]

On 28/11/15 16:32, Jungseok Lee wrote:
> On Nov 17, 2015, at 3:22 AM, James Morse wrote:
>> [Added per_cpu definitions and initialisation for irq_stack]
> 
> How about moving all irq_stack items to [PATCH 2/3] or [PATCH 3/3]?

They ended up in patch 1 because this was all originally one patch, and
patch 1 already modified those files.

I have moved them into patch 2 for v8.


Thanks,

James

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[Catalin Marinas]

On Fri, Dec 04, 2015 at 11:00:19AM +0000, James Morse wrote:
> On 27/11/15 11:47, Catalin Marinas wrote:
> > With this approach, we could run softirq either on the IRQ stack or on
> > the process stack. One such process is softirqd, so there shouldn't be a
> > problem since its goal is to run softirqs. But do_softirq() may be
> > invoked from other contexts (network stack), I'm not sure what the stack
> > looks like at that point.
> 
> As long as we don't reduce the stack sizes yet - nothing has changed. We
> already run __do_softirq() on other task's stacks via __irq_exit().

Yes, the difference would be that we can run it on the IRQ stack but it
doesn't solve explicit do_softirq() invocations from other contexts.

> > We could just do like x86 and always switch to the IRQ stack before
> > invoking __do_softirq(). I don't think we lose much.
> 
> I will add an extra patch to do this - its not as straight forward as I
> would like - as both ksoftirqd and softirq on irq_stack call
> do_softirq_own_stack() with irq_count == 0.

Yes, so do_softirq_own_stack() would do:

1. Increment and test irq_count
2. If old value was 0, switch to the IRQ stack while saving the previous
   SP
3. Call __do_softirq
5. Decrement irq_count
4. Restore previous SP if irq_count is 0
5. RET

One thing I'm not clear about is whether __do_softirq can be preempted
when executed in process context, it could mess up our stack
assumptions.

-- 
Catalin

[PATCH v7 3/3] arm64: Add do_softirq_own_stack() and enable irq_stacks

[James Morse]

On 04/12/15 13:12, Catalin Marinas wrote:
> One thing I'm not clear about is whether __do_softirq can be preempted
> when executed in process context, it could mess up our stack
> assumptions.

Previously I thought not, because in __do_softirq():
> __local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);

Increases the softirq part of preempt_count, before:
> local_irq_enable()

so preempt_count is always non-zero when we have interrupts (re)enabled
on the irq_stack...

... but its a little murkier than I first thought,
__cond_resched_softirq() looks like it will re-schedule a task if it is
processing softirqs, while softirqs are disabled.

net/core/sock.c:__release_sock is the only caller, I haven't yet worked
out if this can happen while on the irq stack.


James