[RFC PATCH 0/3] Implement IRQ stack on ARM64

[RFC PATCH 0/3] Implement IRQ stack on ARM64

[Jungseok Lee]

ARM64 kernel allocates 16KB kernel stack when creating a process. In case
of low memory platforms with tough workloads on userland, this order-2
allocation request reaches to memory pressure and performance degradation
simultaenously since VM page allocator falls into slowpath frequently,
which triggers page reclaim and compaction.

I believe that one of the best solutions is to reduce kernel stack size.
According to the following data from stack tracer with some fixes, [1],
a separate IRQ stack would greatly help to decrease a kernel stack depth.

	        Depth    Size   Location    (51 entries)
	        -----    ----   --------
	  0)     5352      96   _raw_spin_unlock_irqrestore+0x1c/0x60
	  1)     5256      48   gic_raise_softirq+0xa0/0xbc
	  2)     5208      80   smp_cross_call+0x40/0xbc
	  3)     5128      48   smp_send_reschedule+0x38/0x48
	  4)     5080      32   trigger_load_balance+0x184/0x29c
	  5)     5048     112   scheduler_tick+0xac/0x104
	  6)     4936      64   update_process_times+0x5c/0x74
	  7)     4872      32   tick_sched_handle.isra.15+0x38/0x7c
	  8)     4840      48   tick_sched_timer+0x48/0x90
	  9)     4792      48   __run_hrtimer+0x60/0x258
	 10)     4744      64   hrtimer_interrupt+0xe8/0x260
	 11)     4680     128   arch_timer_handler_virt+0x38/0x48
	 12)     4552      32   handle_percpu_devid_irq+0x84/0x188
	 13)     4520      64   generic_handle_irq+0x38/0x54
	 14)     4456      32   __handle_domain_irq+0x68/0xbc
	 15)     4424      64   gic_handle_irq+0x38/0x88
	 16)     4360     280   el1_irq+0x64/0xd8
	 17)     4080     168   ftrace_ops_no_ops+0xb4/0x16c
	 18)     3912      32   ftrace_call+0x0/0x4
	 19)     3880     144   __alloc_skb+0x48/0x180
	 20)     3736      96   alloc_skb_with_frags+0x74/0x234
	 21)     3640     112   sock_alloc_send_pskb+0x1d0/0x294
	 22)     3528     160   sock_alloc_send_skb+0x44/0x54
	 23)     3368      64   __ip_append_data.isra.40+0x78c/0xb48
	 24)     3304     224   ip_append_data.part.42+0x98/0xe8
	 25)     3080     112   ip_append_data+0x68/0x7c
	 26)     2968      96   icmp_push_reply+0x7c/0x144
	 27)     2872      96   icmp_send+0x3c0/0x3c8
	 28)     2776     192   __udp4_lib_rcv+0x5b8/0x684
	 29)     2584      96   udp_rcv+0x2c/0x3c
	 30)     2488      32   ip_local_deliver+0xa0/0x224
	 31)     2456      48   ip_rcv+0x360/0x57c
	 32)     2408      64   __netif_receive_skb_core+0x4d0/0x80c
	 33)     2344     128   __netif_receive_skb+0x24/0x84
	 34)     2216      32   process_backlog+0x9c/0x15c
	 35)     2184      80   net_rx_action+0x1ec/0x32c
	 36)     2104     160   __do_softirq+0x114/0x2f0
	 37)     1944     128   do_softirq+0x60/0x68
	 38)     1816      32   __local_bh_enable_ip+0xb0/0xd4
	 39)     1784      32   ip_finish_output+0x1f4/0xabc
	 40)     1752      96   ip_output+0xf0/0x120
	 41)     1656      64   ip_local_out_sk+0x44/0x54
	 42)     1592      32   ip_send_skb+0x24/0xbc
	 43)     1560      48   udp_send_skb+0x1b4/0x2f4
	 44)     1512      80   udp_sendmsg+0x2a8/0x7a0
	 45)     1432     272   inet_sendmsg+0xa0/0xd0
	 46)     1160      48   sock_sendmsg+0x30/0x78
	 47)     1112      32   ___sys_sendmsg+0x15c/0x26c
	 48)     1080     400   __sys_sendmmsg+0x94/0x180
	 49)      680     320   SyS_sendmmsg+0x38/0x54
	 50)      360     360   el0_svc_naked+0x20/0x28

So, this patch set implements a separate percpu IRQ stack. 

AFAIK, a stack tracer on ftrace does not work well. Thus, this is a single
todo list at this moment.

This series is written on top of 4.2-rc5 with drangon410c board, and it has
been validated with two different tracks: 4.2-rc5 + Linaro Ubuntu 15.04 and
3.10 + Android.

After this merge window, I will rebase this series and resend it.

Any comments or feedbacks are always welcome.

Thanks in advance!

[1]: https://lkml.org/lkml/2015/7/13/29  

Jungseok Lee (3):
  arm64: entry: Remove unnecessary calculation for S_SP in EL1h
  arm64: Introduce IRQ stack
  arm64: Reduce kernel stack size when using IRQ stack

 arch/arm64/Kconfig.debug             | 10 ++
 arch/arm64/include/asm/irq.h         |  8 ++
 arch/arm64/include/asm/thread_info.h | 19 ++++
 arch/arm64/kernel/asm-offsets.c      |  8 ++
 arch/arm64/kernel/entry.S            | 85 +++++++++++++++-
 arch/arm64/kernel/head.S             |  7 ++
 arch/arm64/kernel/irq.c              | 18 ++++
 7 files changed, 150 insertions(+), 5 deletions(-)

-- 
1.9.1

[RFC PATCH 1/3] arm64: entry: Remove unnecessary calculation for S_SP in EL1h

[Jungseok Lee]

Under EL1h, S_SP data is not seen in kernel_exit. Thus, x21 calculation
is not needed in kernel_entry. Currently, S_SP information is vaild only
when sp_el0 is used.

Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
---
 arch/arm64/kernel/entry.S | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index e163518..d23ca0d 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -91,8 +91,6 @@
 	get_thread_info tsk			// 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
-	add	x21, sp, #S_FRAME_SIZE
 	.endif
 	mrs	x22, elr_el1
 	mrs	x23, spsr_el1
-- 
1.9.1

[RFC PATCH 1/3] arm64: entry: Remove unnecessary calculation for S_SP in EL1h

[James Morse]

On 04/09/15 15:23, Jungseok Lee wrote:
> Under EL1h, S_SP data is not seen in kernel_exit. Thus, x21 calculation
> is not needed in kernel_entry. Currently, S_SP information is vaild only
> when sp_el0 is used.
> 
> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index e163518..d23ca0d 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -91,8 +91,6 @@
>  	get_thread_info tsk			// 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
> -	add	x21, sp, #S_FRAME_SIZE
>  	.endif
>  	mrs	x22, elr_el1
>  	mrs	x23, spsr_el1
> 

This sp value gets written to the struct pt_regs that is built on the
stack, and passed to the fault handlers, see 'el1_sp_pc' in kernel/entry.S,
which goes on to call do_sp_pc_abort() which prints this value out. (Other
fault handlers may make decisions based on this value).
It should be present and correct.


Thanks,

James

[RFC PATCH 1/3] arm64: entry: Remove unnecessary calculation for S_SP in EL1h

[Mark Rutland]

On Fri, Sep 04, 2015 at 03:23:05PM +0100, Jungseok Lee wrote:
> Under EL1h, S_SP data is not seen in kernel_exit. Thus, x21 calculation
> is not needed in kernel_entry. Currently, S_SP information is vaild only
> when sp_el0 is used.

I don't think this is true. The generic BUG implementation will grab the
saved SP from the pt_regs, and with this change we'll report whatever
happened to be in x21 instead.

> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
> ---
>  arch/arm64/kernel/entry.S | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index e163518..d23ca0d 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -91,8 +91,6 @@
>  	get_thread_info tsk			// 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
> -	add	x21, sp, #S_FRAME_SIZE
>  	.endif
>  	mrs	x22, elr_el1
>  	mrs	x23, spsr_el1

Immediately after this we do:

	stp     lr, x21, [sp, #S_LR]

To store the LR and SP to the pt_regs which bug_handler would use.

Am I missing smoething?

Thanks,
Mark.

[RFC PATCH 1/3] arm64: entry: Remove unnecessary calculation for S_SP in EL1h

[Jungseok Lee]

On Sep 7, 2015, at 11:56 PM, Mark Rutland wrote:

Hi Mark,

> On Fri, Sep 04, 2015 at 03:23:05PM +0100, Jungseok Lee wrote:
>> Under EL1h, S_SP data is not seen in kernel_exit. Thus, x21 calculation
>> is not needed in kernel_entry. Currently, S_SP information is vaild only
>> when sp_el0 is used.
> 
> I don't think this is true. The generic BUG implementation will grab the
> saved SP from the pt_regs, and with this change we'll report whatever
> happened to be in x21 instead.
> 
>> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
>> ---
>> arch/arm64/kernel/entry.S | 2 --
>> 1 file changed, 2 deletions(-)
>> 
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index e163518..d23ca0d 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -91,8 +91,6 @@
>> 	get_thread_info tsk			// 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
>> -	add	x21, sp, #S_FRAME_SIZE
>> 	.endif
>> 	mrs	x22, elr_el1
>> 	mrs	x23, spsr_el1
> 
> Immediately after this we do:
> 
> 	stp     lr, x21, [sp, #S_LR]
> 
> To store the LR and SP to the pt_regs which bug_handler would use.
> 
> Am I missing smoething?

No, You're right. As James mentioned, x21 is used in do_sp_pc_abort.

Thanks for the comment.

Best Regards
Jungseok Lee

[RFC PATCH 2/3] arm64: Introduce IRQ stack

[Jungseok Lee]

Currently, kernel context and interrupts are handled using a single
kernel stack navigated by sp_el1. This forces many systems to use
16KB stack, not 8KB one. Low memory platforms naturally suffer from
both memory pressure and performance degradation simultaneously as
VM page allocator falls into slowpath frequently.

This patch, thus, solves the problem as introducing a separate percpu
IRQ stack to handle both hard and soft interrupts with two ground rules:

  - Utilize sp_el0 in EL1 context, which is not used currently
  - Do *not* complicate current_thread_info calculation

struct thread_info can be tracked easily using sp_el0, not sp_el1 when
this feature is enabled.

Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
---
 arch/arm64/Kconfig.debug             | 10 ++
 arch/arm64/include/asm/irq.h         |  8 ++
 arch/arm64/include/asm/thread_info.h | 11 ++
 arch/arm64/kernel/asm-offsets.c      |  8 ++
 arch/arm64/kernel/entry.S            | 83 +++++++++++++++-
 arch/arm64/kernel/head.S             |  7 ++
 arch/arm64/kernel/irq.c              | 18 ++++
 7 files changed, 142 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index d6285ef..e16d91f 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -18,6 +18,16 @@ config ARM64_PTDUMP
 	  kernel.
 	  If in doubt, say "N"
 
+config IRQ_STACK
+	bool "Use separate kernel stack when handling interrupts"
+	depends on ARM64_4K_PAGES
+	help
+	  Say Y here if you want to use separate kernel stack to handle both
+	  hard and soft interrupts. As reduceing memory footprint regarding
+	  kernel stack, it benefits low memory platforms.
+
+	  If in doubt, say N.
+
 config STRICT_DEVMEM
 	bool "Filter access to /dev/mem"
 	depends on MMU
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index bbb251b..3ec1fa2 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -5,6 +5,14 @@
 
 #include <asm-generic/irq.h>
 
+#ifdef CONFIG_IRQ_STACK
+struct irq_stack {
+	void *stack;
+	unsigned long thread_sp;
+	unsigned int count;
+};
+#endif
+
 struct pt_regs;
 
 extern void migrate_irqs(void);
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index dcd06d1..5345a67 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -71,11 +71,22 @@ register unsigned long current_stack_pointer asm ("sp");
  */
 static inline struct thread_info *current_thread_info(void) __attribute_const__;
 
+#ifndef CONFIG_IRQ_STACK
 static inline struct thread_info *current_thread_info(void)
 {
 	return (struct thread_info *)
 		(current_stack_pointer & ~(THREAD_SIZE - 1));
 }
+#else
+static inline struct thread_info *current_thread_info(void)
+{
+	unsigned long sp_el0;
+
+	asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
+
+	return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
+}
+#endif
 
 #define thread_saved_pc(tsk)	\
 	((unsigned long)(tsk->thread.cpu_context.pc))
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index c99701a..6feee0e 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -22,6 +22,7 @@
 #include <linux/mm.h>
 #include <linux/dma-mapping.h>
 #include <linux/kvm_host.h>
+#include <asm/irq.h>
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/smp_plat.h>
@@ -41,6 +42,13 @@ int main(void)
   BLANK();
   DEFINE(THREAD_CPU_CONTEXT,	offsetof(struct task_struct, thread.cpu_context));
   BLANK();
+#ifdef CONFIG_IRQ_STACK
+  DEFINE(IRQ_STACK,		offsetof(struct irq_stack, stack));
+  DEFINE(IRQ_THREAD_SP,		offsetof(struct irq_stack, thread_sp));
+  DEFINE(IRQ_COUNT,		offsetof(struct irq_stack, count));
+  DEFINE(IRQ_STACK_SIZE,	sizeof(struct irq_stack));
+  BLANK();
+#endif
   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 d23ca0d..f1fdfa9 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -88,7 +88,11 @@
 
 	.if	\el == 0
 	mrs	x21, sp_el0
+#ifndef CONFIG_IRQ_STACK
 	get_thread_info tsk			// Ensure MDSCR_EL1.SS is clear,
+#else
+	get_thread_info \el, tsk
+#endif
 	ldr	x19, [tsk, #TI_FLAGS]		// since we can unmask debug
 	disable_step_tsk x19, x20		// exceptions when scheduling.
 	.endif
@@ -168,11 +172,56 @@
 	eret					// return to kernel
 	.endm
 
+#ifndef CONFIG_IRQ_STACK
 	.macro	get_thread_info, rd
 	mov	\rd, sp
-	and	\rd, \rd, #~(THREAD_SIZE - 1)	// top of stack
+	and	\rd, \rd, #~(THREAD_SIZE - 1)	// bottom of stack
+	.endm
+#else
+	.macro	get_thread_info, el, rd
+	.if	\el == 0
+	mov	\rd, sp
+	.else
+	mrs	\rd, sp_el0
+	.endif
+	and	\rd, \rd, #~(THREAD_SIZE - 1)	// bottom of thread stack
+	.endm
+
+	.macro	get_irq_stack
+	get_thread_info 1, tsk
+	ldr	w22, [tsk, #TI_CPU]
+	adr_l	x21, irq_stacks
+	mov	x23, #IRQ_STACK_SIZE
+	madd	x21, x22, x23, x21
 	.endm
 
+	.macro	irq_stack_entry
+	get_irq_stack
+	ldr	w23, [x21, #IRQ_COUNT]
+	cbnz	w23, 1f
+	mov	x23, sp
+	str	x23, [x21, #IRQ_THREAD_SP]
+	ldr	x23, [x21, #IRQ_STACK]
+	mov	sp, x23
+	mov	x23, xzr
+1:	add	w23, w23, #1
+	str	w23, [x21, #IRQ_COUNT]
+	.endm
+
+	.macro	irq_stack_exit
+	get_irq_stack
+	ldr	w23, [x21, #IRQ_COUNT]
+	sub	w23, w23, #1
+	cbnz	w23, 1f
+	mov	x23, sp
+	str	x23, [x21, #IRQ_STACK]
+	ldr	x23, [x21, #IRQ_THREAD_SP]
+	mov	sp, x23
+	mov	x23, xzr
+1:	str	w23, [x21, #IRQ_COUNT]
+	.endm
+#endif
+
 /*
  * 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.
@@ -188,10 +237,15 @@ 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
+#ifdef CONFIG_IRQ_STACK
+	irq_stack_entry
+#endif
 	blr	x1
+#ifdef CONFIG_IRQ_STACK
+	irq_stack_exit
+#endif
 	.endm
 
 	.text
@@ -366,7 +420,11 @@ el1_irq:
 	irq_handler
 
 #ifdef CONFIG_PREEMPT
+#ifndef CONFIG_IRQ_STACK
 	get_thread_info tsk
+#else
+	get_thread_info 1, tsk
+#endif
 	ldr	w24, [tsk, #TI_PREEMPT]		// get preempt count
 	cbnz	w24, 1f				// preempt count != 0
 	ldr	x0, [tsk, #TI_FLAGS]		// get flags
@@ -395,6 +453,10 @@ el1_preempt:
 	.align	6
 el0_sync:
 	kernel_entry 0
+#ifdef CONFIG_IRQ_STACK
+	mov	x25, sp
+	msr	sp_el0, x25
+#endif
 	mrs	x25, esr_el1			// read the syndrome register
 	lsr	x24, x25, #ESR_ELx_EC_SHIFT	// exception class
 	cmp	x24, #ESR_ELx_EC_SVC64		// SVC in 64-bit state
@@ -423,6 +485,10 @@ el0_sync:
 	.align	6
 el0_sync_compat:
 	kernel_entry 0, 32
+#ifdef CONFIG_IRQ_STACK
+	mov	x25, sp
+	msr	sp_el0, x25
+#endif
 	mrs	x25, esr_el1			// read the syndrome register
 	lsr	x24, x25, #ESR_ELx_EC_SHIFT	// exception class
 	cmp	x24, #ESR_ELx_EC_SVC32		// SVC in 32-bit state
@@ -560,6 +626,10 @@ ENDPROC(el0_sync)
 el0_irq:
 	kernel_entry 0
 el0_irq_naked:
+#ifdef CONFIG_IRQ_STACK
+	mov	x22, sp
+	msr	sp_el0, x22
+#endif
 	enable_dbg
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
@@ -602,6 +672,9 @@ ENTRY(cpu_switch_to)
 	ldp	x29, x9, [x8], #16
 	ldr	lr, [x8]
 	mov	sp, x9
+#ifdef CONFIG_IRQ_STACK
+	msr	sp_el0, x9
+#endif
 	ret
 ENDPROC(cpu_switch_to)
 
@@ -661,7 +734,11 @@ ENTRY(ret_from_fork)
 	cbz	x19, 1f				// not a kernel thread
 	mov	x0, x20
 	blr	x19
+#ifndef CONFIG_IRQ_STACK
 1:	get_thread_info tsk
+#else
+1:	get_thread_info 1, tsk
+#endif
 	b	ret_to_user
 ENDPROC(ret_from_fork)
 
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index c0ff3ce..3142766 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -446,6 +446,10 @@ __mmap_switched:
 	b	1b
 2:
 	adr_l	sp, initial_sp, x4
+#ifdef CONFIG_IRQ_STACK
+	mov	x4, sp
+	msr	sp_el0, x4
+#endif
 	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
 	str_l	x24, memstart_addr, x6		// Save PHYS_OFFSET
 	mov	x29, #0
@@ -619,6 +623,9 @@ ENDPROC(secondary_startup)
 ENTRY(__secondary_switched)
 	ldr	x0, [x21]			// get secondary_data.stack
 	mov	sp, x0
+#ifdef CONFIG_IRQ_STACK
+	msr	sp_el0, x0
+#endif
 	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 463fa2e..fb0f522 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -31,6 +31,10 @@
 
 unsigned long irq_err_count;
 
+#ifdef CONFIG_IRQ_STACK
+struct irq_stack irq_stacks[NR_CPUS];
+#endif
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 #ifdef CONFIG_SMP
@@ -52,6 +56,20 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 
 void __init init_IRQ(void)
 {
+#ifdef CONFIG_IRQ_STACK
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu) {
+		void *stack = (void *)__get_free_pages(THREADINFO_GFP,
+							THREAD_SIZE_ORDER);
+		if (!stack)
+			panic("CPU%u: No IRQ stack", cpu);
+
+		irq_stacks[cpu].stack = stack + THREAD_START_SP;
+	}
+	pr_info("IRQ: Allocated IRQ stack successfully\n");
+#endif
+
 	irqchip_init();
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
-- 
1.9.1

[RFC PATCH 2/3] arm64: Introduce IRQ stack

[Alexnader Kuleshov]

Hello Jungseok,

On 09-04-15, Jungseok Lee wrote:
> +config IRQ_STACK
> +	bool "Use separate kernel stack when handling interrupts"
> +	depends on ARM64_4K_PAGES
> +	help
> +	  Say Y here if you want to use separate kernel stack to handle both
> +	  hard and soft interrupts. As reduceing memory footprint regarding
> +	  kernel stack, it benefits low memory platforms.

s/reduceing/reducing

[RFC PATCH 2/3] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 5, 2015, at 2:12 AM, Alexnader Kuleshov wrote:

> Hello Jungseok,

Hello Alexnader,

> On 09-04-15, Jungseok Lee wrote:
>> +config IRQ_STACK
>> +	bool "Use separate kernel stack when handling interrupts"
>> +	depends on ARM64_4K_PAGES
>> +	help
>> +	  Say Y here if you want to use separate kernel stack to handle both
>> +	  hard and soft interrupts. As reduceing memory footprint regarding
>> +	  kernel stack, it benefits low memory platforms.
> 
> s/reduceing/reducing

Thanks for pointing out the typo.

I will update the help message in the next version.

Best Regards
Jungseok Lee

[RFC PATCH 2/3] arm64: Introduce IRQ stack

[James Morse]

On 04/09/15 15:23, Jungseok Lee wrote:
> Currently, kernel context and interrupts are handled using a single
> kernel stack navigated by sp_el1. This forces many systems to use
> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
> both memory pressure and performance degradation simultaneously as
> VM page allocator falls into slowpath frequently.
> 
> This patch, thus, solves the problem as introducing a separate percpu
> IRQ stack to handle both hard and soft interrupts with two ground rules:
> 
>   - Utilize sp_el0 in EL1 context, which is not used currently
>   - Do *not* complicate current_thread_info calculation
> 
> struct thread_info can be tracked easily using sp_el0, not sp_el1 when
> this feature is enabled.
> 
> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
> ---
>  arch/arm64/Kconfig.debug             | 10 ++
>  arch/arm64/include/asm/irq.h         |  8 ++
>  arch/arm64/include/asm/thread_info.h | 11 ++
>  arch/arm64/kernel/asm-offsets.c      |  8 ++
>  arch/arm64/kernel/entry.S            | 83 +++++++++++++++-
>  arch/arm64/kernel/head.S             |  7 ++
>  arch/arm64/kernel/irq.c              | 18 ++++
>  7 files changed, 142 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index d6285ef..e16d91f 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -18,6 +18,16 @@ config ARM64_PTDUMP
>  	  kernel.
>  	  If in doubt, say "N"
>  
> +config IRQ_STACK
> +	bool "Use separate kernel stack when handling interrupts"
> +	depends on ARM64_4K_PAGES
> +	help
> +	  Say Y here if you want to use separate kernel stack to handle both
> +	  hard and soft interrupts. As reduceing memory footprint regarding
> +	  kernel stack, it benefits low memory platforms.
> +
> +	  If in doubt, say N.
> +

I don't think it is necessary to have a debug-only Kconfig option for this.
Reducing memory use is good for everyone!

This would let you get rid of all the #ifdefs


>  config STRICT_DEVMEM
>  	bool "Filter access to /dev/mem"
>  	depends on MMU
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index dcd06d1..5345a67 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -71,11 +71,22 @@ register unsigned long current_stack_pointer asm ("sp");
>   */
>  static inline struct thread_info *current_thread_info(void) __attribute_const__;
>  
> +#ifndef CONFIG_IRQ_STACK
>  static inline struct thread_info *current_thread_info(void)
>  {
>  	return (struct thread_info *)
>  		(current_stack_pointer & ~(THREAD_SIZE - 1));
>  }
> +#else
> +static inline struct thread_info *current_thread_info(void)
> +{
> +	unsigned long sp_el0;
> +
> +	asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
> +
> +	return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
> +}
> +#endif

Because sp_el0 is only used as a stack value to find struct thread_info,
you could just store the struct thread_info pointer in sp_el0, and save the
masking on each read of the value.


>  
>  #define thread_saved_pc(tsk)	\
>  	((unsigned long)(tsk->thread.cpu_context.pc))
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index d23ca0d..f1fdfa9 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -88,7 +88,11 @@
>  
>  	.if	\el == 0
>  	mrs	x21, sp_el0
> +#ifndef CONFIG_IRQ_STACK
>  	get_thread_info tsk			// Ensure MDSCR_EL1.SS is clear,
> +#else
> +	get_thread_info \el, tsk
> +#endif
>  	ldr	x19, [tsk, #TI_FLAGS]		// since we can unmask debug
>  	disable_step_tsk x19, x20		// exceptions when scheduling.
>  	.endif
> @@ -168,11 +172,56 @@
>  	eret					// return to kernel
>  	.endm
>  
> +#ifndef CONFIG_IRQ_STACK
>  	.macro	get_thread_info, rd
>  	mov	\rd, sp
> -	and	\rd, \rd, #~(THREAD_SIZE - 1)	// top of stack
> +	and	\rd, \rd, #~(THREAD_SIZE - 1)	// bottom of stack
> +	.endm
> +#else
> +	.macro	get_thread_info, el, rd
> +	.if	\el == 0
> +	mov	\rd, sp
> +	.else
> +	mrs	\rd, sp_el0
> +	.endif
> +	and	\rd, \rd, #~(THREAD_SIZE - 1)	// bottom of thread stack
> +	.endm
> +
> +	.macro	get_irq_stack
> +	get_thread_info 1, tsk
> +	ldr	w22, [tsk, #TI_CPU]
> +	adr_l	x21, irq_stacks
> +	mov	x23, #IRQ_STACK_SIZE
> +	madd	x21, x22, x23, x21
>  	.endm

Using per_cpu variables would save the multiply here.
You then wouldn't need IRQ_STACK_SIZE.


>  
> +	.macro	irq_stack_entry
> +	get_irq_stack
> +	ldr	w23, [x21, #IRQ_COUNT]
> +	cbnz	w23, 1f
> +	mov	x23, sp
> +	str	x23, [x21, #IRQ_THREAD_SP]
> +	ldr	x23, [x21, #IRQ_STACK]
> +	mov	sp, x23
> +	mov	x23, xzr
> +1:	add	w23, w23, #1
> +	str	w23, [x21, #IRQ_COUNT]

Why do you need to count the number of irqs?
struct thread_info:preempt_count already does something similar (but its
not usable this early...)

An alternative way would be to compare the top bits of the two stack
pointers - all we care about is irq recursion right?

This would save storing 'int count' for each cpu, and the logic to
inc/decrement it.


> +	.endm
> +
> +	.macro	irq_stack_exit
> +	get_irq_stack
> +	ldr	w23, [x21, #IRQ_COUNT]
> +	sub	w23, w23, #1
> +	cbnz	w23, 1f
> +	mov	x23, sp
> +	str	x23, [x21, #IRQ_STACK]
> +	ldr	x23, [x21, #IRQ_THREAD_SP]
> +	mov	sp, x23
> +	mov	x23, xzr
> +1:	str	w23, [x21, #IRQ_COUNT]
> +	.endm
> +#endif
> +
>  /*
>   * 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.
> @@ -188,10 +237,15 @@ 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
> +#ifdef CONFIG_IRQ_STACK
> +	irq_stack_entry
> +#endif
>  	blr	x1
> +#ifdef CONFIG_IRQ_STACK
> +	irq_stack_exit
> +#endif
>  	.endm
>  
>  	.text
> @@ -366,7 +420,11 @@ el1_irq:
>  	irq_handler
>  
>  #ifdef CONFIG_PREEMPT
> +#ifndef CONFIG_IRQ_STACK
>  	get_thread_info tsk
> +#else
> +	get_thread_info 1, tsk
> +#endif
>  	ldr	w24, [tsk, #TI_PREEMPT]		// get preempt count
>  	cbnz	w24, 1f				// preempt count != 0
>  	ldr	x0, [tsk, #TI_FLAGS]		// get flags
> @@ -395,6 +453,10 @@ el1_preempt:
>  	.align	6
>  el0_sync:
>  	kernel_entry 0
> +#ifdef CONFIG_IRQ_STACK
> +	mov	x25, sp
> +	msr	sp_el0, x25
> +#endif

Could you do this in kernel_entry?


>  	mrs	x25, esr_el1			// read the syndrome register
>  	lsr	x24, x25, #ESR_ELx_EC_SHIFT	// exception class
>  	cmp	x24, #ESR_ELx_EC_SVC64		// SVC in 64-bit state
> @@ -423,6 +485,10 @@ el0_sync:
>  	.align	6
>  el0_sync_compat:
>  	kernel_entry 0, 32
> +#ifdef CONFIG_IRQ_STACK
> +	mov	x25, sp
> +	msr	sp_el0, x25
> +#endif
>  	mrs	x25, esr_el1			// read the syndrome register
>  	lsr	x24, x25, #ESR_ELx_EC_SHIFT	// exception class
>  	cmp	x24, #ESR_ELx_EC_SVC32		// SVC in 32-bit state
> @@ -560,6 +626,10 @@ ENDPROC(el0_sync)
>  el0_irq:
>  	kernel_entry 0
>  el0_irq_naked:
> +#ifdef CONFIG_IRQ_STACK
> +	mov	x22, sp
> +	msr	sp_el0, x22
> +#endif
>  	enable_dbg
>  #ifdef CONFIG_TRACE_IRQFLAGS
>  	bl	trace_hardirqs_off
> @@ -602,6 +672,9 @@ ENTRY(cpu_switch_to)
>  	ldp	x29, x9, [x8], #16
>  	ldr	lr, [x8]
>  	mov	sp, x9
> +#ifdef CONFIG_IRQ_STACK
> +	msr	sp_el0, x9
> +#endif
>  	ret
>  ENDPROC(cpu_switch_to)
>  
> @@ -661,7 +734,11 @@ ENTRY(ret_from_fork)
>  	cbz	x19, 1f				// not a kernel thread
>  	mov	x0, x20
>  	blr	x19
> +#ifndef CONFIG_IRQ_STACK
>  1:	get_thread_info tsk
> +#else
> +1:	get_thread_info 1, tsk
> +#endif
>  	b	ret_to_user
>  ENDPROC(ret_from_fork)
>  
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index c0ff3ce..3142766 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -446,6 +446,10 @@ __mmap_switched:
>  	b	1b
>  2:
>  	adr_l	sp, initial_sp, x4
> +#ifdef CONFIG_IRQ_STACK
> +	mov	x4, sp
> +	msr	sp_el0, x4
> +#endif
>  	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
>  	str_l	x24, memstart_addr, x6		// Save PHYS_OFFSET
>  	mov	x29, #0
> @@ -619,6 +623,9 @@ ENDPROC(secondary_startup)
>  ENTRY(__secondary_switched)
>  	ldr	x0, [x21]			// get secondary_data.stack
>  	mov	sp, x0
> +#ifdef CONFIG_IRQ_STACK
> +	msr	sp_el0, x0
> +#endif
>  	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 463fa2e..fb0f522 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -31,6 +31,10 @@
>  
>  unsigned long irq_err_count;
>  
> +#ifdef CONFIG_IRQ_STACK
> +struct irq_stack irq_stacks[NR_CPUS];
> +#endif
> +

DEFINE_PER_CPU()?


>  int arch_show_interrupts(struct seq_file *p, int prec)
>  {
>  #ifdef CONFIG_SMP
> @@ -52,6 +56,20 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>  
>  void __init init_IRQ(void)
>  {
> +#ifdef CONFIG_IRQ_STACK
> +	unsigned int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		void *stack = (void *)__get_free_pages(THREADINFO_GFP,
> +							THREAD_SIZE_ORDER);
> +		if (!stack)
> +			panic("CPU%u: No IRQ stack", cpu);
> +
> +		irq_stacks[cpu].stack = stack + THREAD_START_SP;
> +	}
> +	pr_info("IRQ: Allocated IRQ stack successfully\n");
> +#endif
> +

Wouldn't it be better to only allocate the stack when the CPU is about to
be brought up? (put the alloc code in __cpu_up()).


Thanks,

James

>  	irqchip_init();
>  	if (!handle_arch_irq)
>  		panic("No interrupt controller found.");
> 

[RFC PATCH 2/3] arm64: Introduce IRQ stack

[Jungseok Lee]

On Sep 7, 2015, at 11:48 PM, James Morse wrote:

Hi James,

> On 04/09/15 15:23, Jungseok Lee wrote:
>> Currently, kernel context and interrupts are handled using a single
>> kernel stack navigated by sp_el1. This forces many systems to use
>> 16KB stack, not 8KB one. Low memory platforms naturally suffer from
>> both memory pressure and performance degradation simultaneously as
>> VM page allocator falls into slowpath frequently.
>> 
>> This patch, thus, solves the problem as introducing a separate percpu
>> IRQ stack to handle both hard and soft interrupts with two ground rules:
>> 
>>  - Utilize sp_el0 in EL1 context, which is not used currently
>>  - Do *not* complicate current_thread_info calculation
>> 
>> struct thread_info can be tracked easily using sp_el0, not sp_el1 when
>> this feature is enabled.
>> 
>> Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
>> ---
>> arch/arm64/Kconfig.debug             | 10 ++
>> arch/arm64/include/asm/irq.h         |  8 ++
>> arch/arm64/include/asm/thread_info.h | 11 ++
>> arch/arm64/kernel/asm-offsets.c      |  8 ++
>> arch/arm64/kernel/entry.S            | 83 +++++++++++++++-
>> arch/arm64/kernel/head.S             |  7 ++
>> arch/arm64/kernel/irq.c              | 18 ++++
>> 7 files changed, 142 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>> index d6285ef..e16d91f 100644
>> --- a/arch/arm64/Kconfig.debug
>> +++ b/arch/arm64/Kconfig.debug
>> @@ -18,6 +18,16 @@ config ARM64_PTDUMP
>> 	  kernel.
>> 	  If in doubt, say "N"
>> 
>> +config IRQ_STACK
>> +	bool "Use separate kernel stack when handling interrupts"
>> +	depends on ARM64_4K_PAGES
>> +	help
>> +	  Say Y here if you want to use separate kernel stack to handle both
>> +	  hard and soft interrupts. As reduceing memory footprint regarding
>> +	  kernel stack, it benefits low memory platforms.
>> +
>> +	  If in doubt, say N.
>> +
> 
> I don't think it is necessary to have a debug-only Kconfig option for this.
> Reducing memory use is good for everyone!
> 
> This would let you get rid of all the #ifdefs

Hmm.. A single concern is stability. However, I agree that this is not an optional
feature. Especially, it definitely benefits low memory platforms. I will remove
this snippet in the next version. 

>> config STRICT_DEVMEM
>> 	bool "Filter access to /dev/mem"
>> 	depends on MMU
>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
>> index dcd06d1..5345a67 100644
>> --- a/arch/arm64/include/asm/thread_info.h
>> +++ b/arch/arm64/include/asm/thread_info.h
>> @@ -71,11 +71,22 @@ register unsigned long current_stack_pointer asm ("sp");
>>  */
>> static inline struct thread_info *current_thread_info(void) __attribute_const__;
>> 
>> +#ifndef CONFIG_IRQ_STACK
>> static inline struct thread_info *current_thread_info(void)
>> {
>> 	return (struct thread_info *)
>> 		(current_stack_pointer & ~(THREAD_SIZE - 1));
>> }
>> +#else
>> +static inline struct thread_info *current_thread_info(void)
>> +{
>> +	unsigned long sp_el0;
>> +
>> +	asm volatile("mrs %0, sp_el0" : "=r" (sp_el0));
>> +
>> +	return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1));
>> +}
>> +#endif
> 
> Because sp_el0 is only used as a stack value to find struct thread_info,
> you could just store the struct thread_info pointer in sp_el0, and save the
> masking on each read of the value.

IMHO, this logic, masking SP with ~(THREAD_SIZE - 1), is a well-known idiom.
So, I don't want to change the expression. In addition, the same process is
needed before storing the address of struct thread_info. 

>> 
>> #define thread_saved_pc(tsk)	\
>> 	((unsigned long)(tsk->thread.cpu_context.pc))
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index d23ca0d..f1fdfa9 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -88,7 +88,11 @@
>> 
>> 	.if	\el == 0
>> 	mrs	x21, sp_el0
>> +#ifndef CONFIG_IRQ_STACK
>> 	get_thread_info tsk			// Ensure MDSCR_EL1.SS is clear,
>> +#else
>> +	get_thread_info \el, tsk
>> +#endif
>> 	ldr	x19, [tsk, #TI_FLAGS]		// since we can unmask debug
>> 	disable_step_tsk x19, x20		// exceptions when scheduling.
>> 	.endif
>> @@ -168,11 +172,56 @@
>> 	eret					// return to kernel
>> 	.endm
>> 
>> +#ifndef CONFIG_IRQ_STACK
>> 	.macro	get_thread_info, rd
>> 	mov	\rd, sp
>> -	and	\rd, \rd, #~(THREAD_SIZE - 1)	// top of stack
>> +	and	\rd, \rd, #~(THREAD_SIZE - 1)	// bottom of stack
>> +	.endm
>> +#else
>> +	.macro	get_thread_info, el, rd
>> +	.if	\el == 0
>> +	mov	\rd, sp
>> +	.else
>> +	mrs	\rd, sp_el0
>> +	.endif
>> +	and	\rd, \rd, #~(THREAD_SIZE - 1)	// bottom of thread stack
>> +	.endm
>> +
>> +	.macro	get_irq_stack
>> +	get_thread_info 1, tsk
>> +	ldr	w22, [tsk, #TI_CPU]
>> +	adr_l	x21, irq_stacks
>> +	mov	x23, #IRQ_STACK_SIZE
>> +	madd	x21, x22, x23, x21
>> 	.endm
> 
> Using per_cpu variables would save the multiply here.
> You then wouldn't need IRQ_STACK_SIZE.

Agree.

>> 
>> +	.macro	irq_stack_entry
>> +	get_irq_stack
>> +	ldr	w23, [x21, #IRQ_COUNT]
>> +	cbnz	w23, 1f
>> +	mov	x23, sp
>> +	str	x23, [x21, #IRQ_THREAD_SP]
>> +	ldr	x23, [x21, #IRQ_STACK]
>> +	mov	sp, x23
>> +	mov	x23, xzr
>> +1:	add	w23, w23, #1
>> +	str	w23, [x21, #IRQ_COUNT]
> 
> Why do you need to count the number of irqs?
> struct thread_info:preempt_count already does something similar (but its
> not usable this early...)
> 
> An alternative way would be to compare the top bits of the two stack
> pointers - all we care about is irq recursion right?

Exactly. A point is a recursion check, not an actual number in this context.
I'd like to address the recursion issue with minimal load and store operation.
The suggestion sounds good. I will figure out a better and simpler way and
update it with comments in the code.

> This would save storing 'int count' for each cpu, and the logic to
> inc/decrement it.
> 
> 
>> +	.endm
>> +
>> +	.macro	irq_stack_exit
>> +	get_irq_stack
>> +	ldr	w23, [x21, #IRQ_COUNT]
>> +	sub	w23, w23, #1
>> +	cbnz	w23, 1f
>> +	mov	x23, sp
>> +	str	x23, [x21, #IRQ_STACK]
>> +	ldr	x23, [x21, #IRQ_THREAD_SP]
>> +	mov	sp, x23
>> +	mov	x23, xzr
>> +1:	str	w23, [x21, #IRQ_COUNT]
>> +	.endm
>> +#endif
>> +
>> /*
>>  * 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.
>> @@ -188,10 +237,15 @@ 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
>> +#ifdef CONFIG_IRQ_STACK
>> +	irq_stack_entry
>> +#endif
>> 	blr	x1
>> +#ifdef CONFIG_IRQ_STACK
>> +	irq_stack_exit
>> +#endif
>> 	.endm
>> 
>> 	.text
>> @@ -366,7 +420,11 @@ el1_irq:
>> 	irq_handler
>> 
>> #ifdef CONFIG_PREEMPT
>> +#ifndef CONFIG_IRQ_STACK
>> 	get_thread_info tsk
>> +#else
>> +	get_thread_info 1, tsk
>> +#endif
>> 	ldr	w24, [tsk, #TI_PREEMPT]		// get preempt count
>> 	cbnz	w24, 1f				// preempt count != 0
>> 	ldr	x0, [tsk, #TI_FLAGS]		// get flags
>> @@ -395,6 +453,10 @@ el1_preempt:
>> 	.align	6
>> el0_sync:
>> 	kernel_entry 0
>> +#ifdef CONFIG_IRQ_STACK
>> +	mov	x25, sp
>> +	msr	sp_el0, x25
>> +#endif
> 
> Could you do this in kernel_entry?

Yes, I could. It would make the code neater.

>> 	mrs	x25, esr_el1			// read the syndrome register
>> 	lsr	x24, x25, #ESR_ELx_EC_SHIFT	// exception class
>> 	cmp	x24, #ESR_ELx_EC_SVC64		// SVC in 64-bit state
>> @@ -423,6 +485,10 @@ el0_sync:
>> 	.align	6
>> el0_sync_compat:
>> 	kernel_entry 0, 32
>> +#ifdef CONFIG_IRQ_STACK
>> +	mov	x25, sp
>> +	msr	sp_el0, x25
>> +#endif
>> 	mrs	x25, esr_el1			// read the syndrome register
>> 	lsr	x24, x25, #ESR_ELx_EC_SHIFT	// exception class
>> 	cmp	x24, #ESR_ELx_EC_SVC32		// SVC in 32-bit state
>> @@ -560,6 +626,10 @@ ENDPROC(el0_sync)
>> el0_irq:
>> 	kernel_entry 0
>> el0_irq_naked:
>> +#ifdef CONFIG_IRQ_STACK
>> +	mov	x22, sp
>> +	msr	sp_el0, x22
>> +#endif
>> 	enable_dbg
>> #ifdef CONFIG_TRACE_IRQFLAGS
>> 	bl	trace_hardirqs_off
>> @@ -602,6 +672,9 @@ ENTRY(cpu_switch_to)
>> 	ldp	x29, x9, [x8], #16
>> 	ldr	lr, [x8]
>> 	mov	sp, x9
>> +#ifdef CONFIG_IRQ_STACK
>> +	msr	sp_el0, x9
>> +#endif
>> 	ret
>> ENDPROC(cpu_switch_to)
>> 
>> @@ -661,7 +734,11 @@ ENTRY(ret_from_fork)
>> 	cbz	x19, 1f				// not a kernel thread
>> 	mov	x0, x20
>> 	blr	x19
>> +#ifndef CONFIG_IRQ_STACK
>> 1:	get_thread_info tsk
>> +#else
>> +1:	get_thread_info 1, tsk
>> +#endif
>> 	b	ret_to_user
>> ENDPROC(ret_from_fork)
>> 
>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>> index c0ff3ce..3142766 100644
>> --- a/arch/arm64/kernel/head.S
>> +++ b/arch/arm64/kernel/head.S
>> @@ -446,6 +446,10 @@ __mmap_switched:
>> 	b	1b
>> 2:
>> 	adr_l	sp, initial_sp, x4
>> +#ifdef CONFIG_IRQ_STACK
>> +	mov	x4, sp
>> +	msr	sp_el0, x4
>> +#endif
>> 	str_l	x21, __fdt_pointer, x5		// Save FDT pointer
>> 	str_l	x24, memstart_addr, x6		// Save PHYS_OFFSET
>> 	mov	x29, #0
>> @@ -619,6 +623,9 @@ ENDPROC(secondary_startup)
>> ENTRY(__secondary_switched)
>> 	ldr	x0, [x21]			// get secondary_data.stack
>> 	mov	sp, x0
>> +#ifdef CONFIG_IRQ_STACK
>> +	msr	sp_el0, x0
>> +#endif
>> 	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 463fa2e..fb0f522 100644
>> --- a/arch/arm64/kernel/irq.c
>> +++ b/arch/arm64/kernel/irq.c
>> @@ -31,6 +31,10 @@
>> 
>> unsigned long irq_err_count;
>> 
>> +#ifdef CONFIG_IRQ_STACK
>> +struct irq_stack irq_stacks[NR_CPUS];
>> +#endif
>> +
> 
> DEFINE_PER_CPU()?

Yeah, I will update it with per_cpu in the next version.

>> int arch_show_interrupts(struct seq_file *p, int prec)
>> {
>> #ifdef CONFIG_SMP
>> @@ -52,6 +56,20 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>> 
>> void __init init_IRQ(void)
>> {
>> +#ifdef CONFIG_IRQ_STACK
>> +	unsigned int cpu;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		void *stack = (void *)__get_free_pages(THREADINFO_GFP,
>> +							THREAD_SIZE_ORDER);
>> +		if (!stack)
>> +			panic("CPU%u: No IRQ stack", cpu);
>> +
>> +		irq_stacks[cpu].stack = stack + THREAD_START_SP;
>> +	}
>> +	pr_info("IRQ: Allocated IRQ stack successfully\n");
>> +#endif
>> +
> 
> Wouldn't it be better to only allocate the stack when the CPU is about to
> be brought up? (put the alloc code in __cpu_up()).

__cpu_up could be called very frequently if a power management scheme based
on CPU hotplug is actively used. So, I'd like to avoid even a simple logic
which checks the previously allocated IRQ stack. 

Thanks for the valuable feedbacks.

Best Regards
Jungseok Lee

[RFC PATCH 3/3] arm64: Reduce kernel stack size when using IRQ stack

[Jungseok Lee]

It is a principal objective of IRQ stack feature to reduce kernel
stack size. Therefore, the size is set to 8KB when a separate IRQ
stack is active.

Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
---
 arch/arm64/include/asm/thread_info.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 5345a67..e79210d 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -24,10 +24,18 @@
 #include <linux/compiler.h>
 
 #ifndef CONFIG_ARM64_64K_PAGES
+#ifdef CONFIG_IRQ_STACK
+#define THREAD_SIZE_ORDER	1
+#else
 #define THREAD_SIZE_ORDER	2
 #endif
+#endif
 
+#ifdef CONFIG_IRQ_STACK
+#define THREAD_SIZE		8192
+#else
 #define THREAD_SIZE		16384
+#endif
 #define THREAD_START_SP		(THREAD_SIZE - 16)
 
 #ifndef __ASSEMBLY__
-- 
1.9.1

[RFC PATCH 0/3] Implement IRQ stack on ARM64

[James Morse]

On 04/09/15 15:23, Jungseok Lee wrote:
> ARM64 kernel allocates 16KB kernel stack when creating a process. In case
> of low memory platforms with tough workloads on userland, this order-2
> allocation request reaches to memory pressure and performance degradation
> simultaenously since VM page allocator falls into slowpath frequently,
> which triggers page reclaim and compaction.
> 
> I believe that one of the best solutions is to reduce kernel stack size.
> According to the following data from stack tracer with some fixes, [1],
> a separate IRQ stack would greatly help to decrease a kernel stack depth.
>

Hi Jungseok Lee,

I was working on a similar patch for irq stack, (patch as a follow up email).

I suggest we work together on a single implementation. I think the only
major difference is that you're using sp_el0 as a temporary register to
store a copy of the stack-pointer to find struct thread_info, whereas I was
copying it between stacks (ends up as 2x ldp/stps), which keeps the change
restricted to irq_stack setup code.

We should get some feedback as to which approach is preferred.


Thanks,

James Morse

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[James Morse]

Having to handle interrupts on top of an existing kernel stack means the
kernel stack must be large enough to accomodate both the maximum kernel
usage, and the maximum irq handler usage. Switching to a different stack
when processing irqs allows us to make the stack size smaller.

Maximum kernel stack usage (running ltp and generating usb+ethernet
interrupts) was 7256 bytes. With this patch, the same workload gives
a maximum stack usage of 5816 bytes.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/irq.h         | 12 +++++++++
 arch/arm64/include/asm/thread_info.h |  8 ++++--
 arch/arm64/kernel/entry.S            | 33 ++++++++++++++++++++---
 arch/arm64/kernel/irq.c              | 52 ++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/smp.c              |  4 +++
 arch/arm64/kernel/stacktrace.c       |  4 ++-
 6 files changed, 107 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index bbb251b14746..050d4196c736 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -2,14 +2,20 @@
 #define __ASM_IRQ_H
 
 #include <linux/irqchip/arm-gic-acpi.h>
+#include <linux/percpu.h>
 
 #include <asm-generic/irq.h>
+#include <asm/thread_info.h>
+
+DECLARE_PER_CPU(unsigned long, irq_sp);
 
 struct pt_regs;
 
 extern void migrate_irqs(void);
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
 
+extern int alloc_irq_stack(unsigned int cpu);
+
 static inline void acpi_irq_init(void)
 {
 	/*
@@ -21,4 +27,10 @@ static inline void acpi_irq_init(void)
 }
 #define acpi_irq_init acpi_irq_init
 
+static inline bool is_irq_stack(unsigned long sp)
+{
+	struct thread_info *ti = get_thread_info(sp);
+	return (get_thread_info(per_cpu(irq_sp, ti->cpu)) == ti);
+}
+
 #endif
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index dcd06d18a42a..b906254fc400 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -69,12 +69,16 @@ register unsigned long current_stack_pointer asm ("sp");
 /*
  * how to get the thread information struct from C
  */
+static inline struct thread_info *get_thread_info(unsigned long sp)
+{
+	return (struct thread_info *)(sp & ~(THREAD_SIZE - 1));
+}
+
 static inline struct thread_info *current_thread_info(void) __attribute_const__;
 
 static inline struct thread_info *current_thread_info(void)
 {
-	return (struct thread_info *)
-		(current_stack_pointer & ~(THREAD_SIZE - 1));
+	return get_thread_info(current_stack_pointer);
 }
 
 #define thread_saved_pc(tsk)	\
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index e16351819fed..d42371f3f5a1 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -190,10 +190,37 @@ tsk	.req	x28		// current thread_info
  * Interrupt handling.
  */
 	.macro	irq_handler
-	adrp	x1, handle_arch_irq
-	ldr	x1, [x1, #:lo12:handle_arch_irq]
-	mov	x0, sp
+	mrs	x21, tpidr_el1
+	adr_l	x20, irq_sp
+	add	x20, x20, x21
+
+	ldr	x21, [x20]
+	mov	x20, sp
+
+	mov	x0, x21
+	mov	x1, x20
+	bl	irq_copy_thread_info
+
+	/* test for recursive use of irq_sp */
+	cbz	w0, 1f
+	mrs	x30, elr_el1
+	mov	sp, x21
+
+	/*
+	 * Create a fake stack frame to bump unwind_frame() onto the original
+	 * stack. This relies on x29 not being clobbered by kernel_entry().
+	 */
+	push	x29, x30
+
+1:	ldr_l	x1, handle_arch_irq
+	mov     x0, x20
 	blr	x1
+
+	mov	x0, x20
+	mov	x1, x21
+	bl	irq_copy_thread_info
+	mov	sp, x20
+
 	.endm
 
 	.text
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 463fa2e7e34c..10b57a006da8 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -26,11 +26,14 @@
 #include <linux/smp.h>
 #include <linux/init.h>
 #include <linux/irqchip.h>
+#include <linux/percpu.h>
 #include <linux/seq_file.h>
 #include <linux/ratelimit.h>
 
 unsigned long irq_err_count;
 
+DEFINE_PER_CPU(unsigned long, irq_sp) = 0;
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 #ifdef CONFIG_SMP
@@ -55,6 +58,10 @@ void __init init_IRQ(void)
 	irqchip_init();
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
+
+	/* Allocate an irq stack for the boot cpu */
+	if (alloc_irq_stack(smp_processor_id()))
+		panic("Failed to allocate irq stack for boot cpu.");
 }
 
 #ifdef CONFIG_HOTPLUG_CPU
@@ -117,3 +124,48 @@ void migrate_irqs(void)
 	local_irq_restore(flags);
 }
 #endif /* CONFIG_HOTPLUG_CPU */
+
+/* Allocate an irq_stack for a cpu that is about to be brought up. */
+int alloc_irq_stack(unsigned int cpu)
+{
+	struct page *irq_stack_page;
+	union thread_union *irq_stack;
+
+	/* reuse stack allocated previously */
+	if (per_cpu(irq_sp, cpu))
+		return 0;
+
+	irq_stack_page = alloc_kmem_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
+	if (!irq_stack_page) {
+		pr_crit("CPU%u: failed to allocate irq stack for cpu %u\n",
+			smp_processor_id(), cpu);
+		return -ENOMEM;
+	}
+	irq_stack = page_address(irq_stack_page);
+
+	per_cpu(irq_sp, cpu) = (unsigned long)irq_stack->stack
+			       + THREAD_START_SP;
+
+	return 0;
+}
+
+/*
+ * Copy struct thread_info between two stacks, and update current->stack.
+ * This is used when moving to the irq exception stack.
+ * Changing current->stack is necessary so that non-arch thread_info writers
+ * don't use the new thread_info->task->stack to find the old thread_info when
+ * setting flags like TIF_NEED_RESCHED...
+ */
+asmlinkage int irq_copy_thread_info(unsigned long dst_sp, unsigned long src_sp)
+{
+	struct thread_info *src = get_thread_info(src_sp);
+	struct thread_info *dst = get_thread_info(dst_sp);
+
+	if (dst == src)
+		return 0;
+
+	*dst = *src;
+	current->stack = dst;
+
+	return 1;
+}
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 50fb4696654e..5283dc5629e4 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -89,6 +89,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
 {
 	int ret;
 
+	ret = alloc_irq_stack(cpu);
+	if (ret)
+		return ret;
+
 	/*
 	 * We need to tell the secondary core where to find its stack and the
 	 * page tables.
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 407991bf79f5..3d6d5b45aa4b 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>
 
 /*
@@ -43,7 +44,8 @@ int notrace unwind_frame(struct stackframe *frame)
 	low  = frame->sp;
 	high = ALIGN(low, THREAD_SIZE);
 
-	if (fp < low || fp > high - 0x18 || fp & 0xf)
+	if ((fp < low || fp > high - 0x18 || fp & 0xf) &&
+	    !is_irq_stack(frame->sp))
 		return -EINVAL;
 
 	frame->sp = fp + 0x10;
-- 
2.1.4

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[Jungseok Lee]

On Sep 7, 2015, at 11:36 PM, James Morse wrote:

Hi James,

> Having to handle interrupts on top of an existing kernel stack means the
> kernel stack must be large enough to accomodate both the maximum kernel
> usage, and the maximum irq handler usage. Switching to a different stack
> when processing irqs allows us to make the stack size smaller.
> 
> Maximum kernel stack usage (running ltp and generating usb+ethernet
> interrupts) was 7256 bytes. With this patch, the same workload gives
> a maximum stack usage of 5816 bytes.

I'd like to know how to measure the max stack depth.
AFAIK, a stack tracer on ftrace does not work well. Did you dump a stack
region and find or track down an untouched region? 

I will leave comments after reading and playing with this change carefully.

Best Regards
Jungseok Lee

> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> arch/arm64/include/asm/irq.h         | 12 +++++++++
> arch/arm64/include/asm/thread_info.h |  8 ++++--
> arch/arm64/kernel/entry.S            | 33 ++++++++++++++++++++---
> arch/arm64/kernel/irq.c              | 52 ++++++++++++++++++++++++++++++++++++
> arch/arm64/kernel/smp.c              |  4 +++
> arch/arm64/kernel/stacktrace.c       |  4 ++-
> 6 files changed, 107 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index bbb251b14746..050d4196c736 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -2,14 +2,20 @@
> #define __ASM_IRQ_H
> 
> #include <linux/irqchip/arm-gic-acpi.h>
> +#include <linux/percpu.h>
> 
> #include <asm-generic/irq.h>
> +#include <asm/thread_info.h>
> +
> +DECLARE_PER_CPU(unsigned long, irq_sp);
> 
> struct pt_regs;
> 
> extern void migrate_irqs(void);
> extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
> 
> +extern int alloc_irq_stack(unsigned int cpu);
> +
> static inline void acpi_irq_init(void)
> {
> 	/*
> @@ -21,4 +27,10 @@ static inline void acpi_irq_init(void)
> }
> #define acpi_irq_init acpi_irq_init
> 
> +static inline bool is_irq_stack(unsigned long sp)
> +{
> +	struct thread_info *ti = get_thread_info(sp);
> +	return (get_thread_info(per_cpu(irq_sp, ti->cpu)) == ti);
> +}
> +
> #endif
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index dcd06d18a42a..b906254fc400 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -69,12 +69,16 @@ register unsigned long current_stack_pointer asm ("sp");
> /*
>  * how to get the thread information struct from C
>  */
> +static inline struct thread_info *get_thread_info(unsigned long sp)
> +{
> +	return (struct thread_info *)(sp & ~(THREAD_SIZE - 1));
> +}
> +
> static inline struct thread_info *current_thread_info(void) __attribute_const__;
> 
> static inline struct thread_info *current_thread_info(void)
> {
> -	return (struct thread_info *)
> -		(current_stack_pointer & ~(THREAD_SIZE - 1));
> +	return get_thread_info(current_stack_pointer);
> }
> 
> #define thread_saved_pc(tsk)	\
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index e16351819fed..d42371f3f5a1 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -190,10 +190,37 @@ tsk	.req	x28		// current thread_info
>  * Interrupt handling.
>  */
> 	.macro	irq_handler
> -	adrp	x1, handle_arch_irq
> -	ldr	x1, [x1, #:lo12:handle_arch_irq]
> -	mov	x0, sp
> +	mrs	x21, tpidr_el1
> +	adr_l	x20, irq_sp
> +	add	x20, x20, x21
> +
> +	ldr	x21, [x20]
> +	mov	x20, sp
> +
> +	mov	x0, x21
> +	mov	x1, x20
> +	bl	irq_copy_thread_info
> +
> +	/* test for recursive use of irq_sp */
> +	cbz	w0, 1f
> +	mrs	x30, elr_el1
> +	mov	sp, x21
> +
> +	/*
> +	 * Create a fake stack frame to bump unwind_frame() onto the original
> +	 * stack. This relies on x29 not being clobbered by kernel_entry().
> +	 */
> +	push	x29, x30
> +
> +1:	ldr_l	x1, handle_arch_irq
> +	mov     x0, x20
> 	blr	x1
> +
> +	mov	x0, x20
> +	mov	x1, x21
> +	bl	irq_copy_thread_info
> +	mov	sp, x20
> +
> 	.endm
> 
> 	.text
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index 463fa2e7e34c..10b57a006da8 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -26,11 +26,14 @@
> #include <linux/smp.h>
> #include <linux/init.h>
> #include <linux/irqchip.h>
> +#include <linux/percpu.h>
> #include <linux/seq_file.h>
> #include <linux/ratelimit.h>
> 
> unsigned long irq_err_count;
> 
> +DEFINE_PER_CPU(unsigned long, irq_sp) = 0;
> +
> int arch_show_interrupts(struct seq_file *p, int prec)
> {
> #ifdef CONFIG_SMP
> @@ -55,6 +58,10 @@ void __init init_IRQ(void)
> 	irqchip_init();
> 	if (!handle_arch_irq)
> 		panic("No interrupt controller found.");
> +
> +	/* Allocate an irq stack for the boot cpu */
> +	if (alloc_irq_stack(smp_processor_id()))
> +		panic("Failed to allocate irq stack for boot cpu.");
> }
> 
> #ifdef CONFIG_HOTPLUG_CPU
> @@ -117,3 +124,48 @@ void migrate_irqs(void)
> 	local_irq_restore(flags);
> }
> #endif /* CONFIG_HOTPLUG_CPU */
> +
> +/* Allocate an irq_stack for a cpu that is about to be brought up. */
> +int alloc_irq_stack(unsigned int cpu)
> +{
> +	struct page *irq_stack_page;
> +	union thread_union *irq_stack;
> +
> +	/* reuse stack allocated previously */
> +	if (per_cpu(irq_sp, cpu))
> +		return 0;
> +
> +	irq_stack_page = alloc_kmem_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
> +	if (!irq_stack_page) {
> +		pr_crit("CPU%u: failed to allocate irq stack for cpu %u\n",
> +			smp_processor_id(), cpu);
> +		return -ENOMEM;
> +	}
> +	irq_stack = page_address(irq_stack_page);
> +
> +	per_cpu(irq_sp, cpu) = (unsigned long)irq_stack->stack
> +			       + THREAD_START_SP;
> +
> +	return 0;
> +}
> +
> +/*
> + * Copy struct thread_info between two stacks, and update current->stack.
> + * This is used when moving to the irq exception stack.
> + * Changing current->stack is necessary so that non-arch thread_info writers
> + * don't use the new thread_info->task->stack to find the old thread_info when
> + * setting flags like TIF_NEED_RESCHED...
> + */
> +asmlinkage int irq_copy_thread_info(unsigned long dst_sp, unsigned long src_sp)
> +{
> +	struct thread_info *src = get_thread_info(src_sp);
> +	struct thread_info *dst = get_thread_info(dst_sp);
> +
> +	if (dst == src)
> +		return 0;
> +
> +	*dst = *src;
> +	current->stack = dst;
> +
> +	return 1;
> +}
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 50fb4696654e..5283dc5629e4 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -89,6 +89,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
> {
> 	int ret;
> 
> +	ret = alloc_irq_stack(cpu);
> +	if (ret)
> +		return ret;
> +
> 	/*
> 	 * We need to tell the secondary core where to find its stack and the
> 	 * page tables.
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 407991bf79f5..3d6d5b45aa4b 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>
> 
> /*
> @@ -43,7 +44,8 @@ int notrace unwind_frame(struct stackframe *frame)
> 	low  = frame->sp;
> 	high = ALIGN(low, THREAD_SIZE);
> 
> -	if (fp < low || fp > high - 0x18 || fp & 0xf)
> +	if ((fp < low || fp > high - 0x18 || fp & 0xf) &&
> +	    !is_irq_stack(frame->sp))
> 		return -EINVAL;
> 
> 	frame->sp = fp + 0x10;
> -- 
> 2.1.4
> 

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[James Morse]

On 07/09/15 16:48, Jungseok Lee wrote:
> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
> 
> Hi James,
> 
>> Having to handle interrupts on top of an existing kernel stack means the
>> kernel stack must be large enough to accomodate both the maximum kernel
>> usage, and the maximum irq handler usage. Switching to a different stack
>> when processing irqs allows us to make the stack size smaller.
>>
>> Maximum kernel stack usage (running ltp and generating usb+ethernet
>> interrupts) was 7256 bytes. With this patch, the same workload gives
>> a maximum stack usage of 5816 bytes.
> 
> I'd like to know how to measure the max stack depth.
> AFAIK, a stack tracer on ftrace does not work well. Did you dump a stack
> region and find or track down an untouched region? 

I enabled the 'Trace max stack' option under menuconfig 'Kernel Hacking' ->
'Tracers', then looked in debugfs:/tracing/stack_max_size.

What problems did you encounter?
(I may be missing something...)


Thanks,

James

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[Jungseok Lee]

On Sep 8, 2015, at 1:06 AM, James Morse wrote:
> On 07/09/15 16:48, Jungseok Lee wrote:
>> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
>> 
>> Hi James,
>> 
>>> Having to handle interrupts on top of an existing kernel stack means the
>>> kernel stack must be large enough to accomodate both the maximum kernel
>>> usage, and the maximum irq handler usage. Switching to a different stack
>>> when processing irqs allows us to make the stack size smaller.
>>> 
>>> Maximum kernel stack usage (running ltp and generating usb+ethernet
>>> interrupts) was 7256 bytes. With this patch, the same workload gives
>>> a maximum stack usage of 5816 bytes.
>> 
>> I'd like to know how to measure the max stack depth.
>> AFAIK, a stack tracer on ftrace does not work well. Did you dump a stack
>> region and find or track down an untouched region? 
> 
> I enabled the 'Trace max stack' option under menuconfig 'Kernel Hacking' ->
> 'Tracers', then looked in debugfs:/tracing/stack_max_size.
> 
> What problems did you encounter?
> (I may be missing something?)

When I enabled the feature, all entries had *0* size except the last entry.
It can be reproduced easily as looking in debugs:/tracing/stack_trace.

You can track down my report and Akashi's changes with the following links:
- http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
- https://lkml.org/lkml/2015/7/13/29

Although it is impossible to measure an exact depth at this moment, the feature
could be utilized to check improvement.

Cc'ing Akashi for additional comments if needed.

Best Regards
Jungseok Lee

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[AKASHI Takahiro]

Jungseok,

On 09/08/2015 01:34 AM, Jungseok Lee wrote:
> On Sep 8, 2015, at 1:06 AM, James Morse wrote:
>> On 07/09/15 16:48, Jungseok Lee wrote:
>>> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
>>>
>>> Hi James,
>>>
>>>> Having to handle interrupts on top of an existing kernel stack means the
>>>> kernel stack must be large enough to accomodate both the maximum kernel
>>>> usage, and the maximum irq handler usage. Switching to a different stack
>>>> when processing irqs allows us to make the stack size smaller.
>>>>
>>>> Maximum kernel stack usage (running ltp and generating usb+ethernet
>>>> interrupts) was 7256 bytes. With this patch, the same workload gives
>>>> a maximum stack usage of 5816 bytes.
>>>
>>> I'd like to know how to measure the max stack depth.
>>> AFAIK, a stack tracer on ftrace does not work well. Did you dump a stack
>>> region and find or track down an untouched region?
>>
>> I enabled the 'Trace max stack' option under menuconfig 'Kernel Hacking' ->
>> 'Tracers', then looked in debugfs:/tracing/stack_max_size.
>>
>> What problems did you encounter?
>> (I may be missing something?)
>
> When I enabled the feature, all entries had *0* size except the last entry.
> It can be reproduced easily as looking in debugs:/tracing/stack_trace.

I'm afraid that you have not applied one of patches in my RFC:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355919.html

I have not looked into James' patch in details, but hope that it will help
fix one of issues that are annoying me: Stack tracer (actually save_stack_trace())
will miss a function (and its parent function in some case) that is being executed
when an interrupt is taken.


-Takahiro AKASHI

> You can track down my report and Akashi's changes with the following links:
> - http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
> - https://lkml.org/lkml/2015/7/13/29
>
> Although it is impossible to measure an exact depth at this moment, the feature
> could be utilized to check improvement.
>
> Cc'ing Akashi for additional comments if needed.
>
> Best Regards
> Jungseok Lee
>

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[AKASHI Takahiro]

On 09/08/2015 10:45 AM, AKASHI Takahiro wrote:
> Jungseok,
>
> On 09/08/2015 01:34 AM, Jungseok Lee wrote:
>> On Sep 8, 2015, at 1:06 AM, James Morse wrote:
>>> On 07/09/15 16:48, Jungseok Lee wrote:
>>>> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
>>>>
>>>> Hi James,
>>>>
>>>>> Having to handle interrupts on top of an existing kernel stack means the
>>>>> kernel stack must be large enough to accomodate both the maximum kernel
>>>>> usage, and the maximum irq handler usage. Switching to a different stack
>>>>> when processing irqs allows us to make the stack size smaller.
>>>>>
>>>>> Maximum kernel stack usage (running ltp and generating usb+ethernet
>>>>> interrupts) was 7256 bytes. With this patch, the same workload gives
>>>>> a maximum stack usage of 5816 bytes.
>>>>
>>>> I'd like to know how to measure the max stack depth.
>>>> AFAIK, a stack tracer on ftrace does not work well. Did you dump a stack
>>>> region and find or track down an untouched region?
>>>
>>> I enabled the 'Trace max stack' option under menuconfig 'Kernel Hacking' ->
>>> 'Tracers', then looked in debugfs:/tracing/stack_max_size.
>>>
>>> What problems did you encounter?
>>> (I may be missing something?)
>>
>> When I enabled the feature, all entries had *0* size except the last entry.
>> It can be reproduced easily as looking in debugs:/tracing/stack_trace.
>
> I'm afraid that you have not applied one of patches in my RFC:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355919.html
>
> I have not looked into James' patch in details, but hope that it will help
> fix one of issues that are annoying me: Stack tracer (actually save_stack_trace())
> will miss a function (and its parent function in some case) that is being executed
> when an interrupt is taken.

Well, it didn't fix the issue:
# cat /sys/kernel/debug/tracing/stack_trace
         Depth    Size   Location    (54 entries)
         -----    ----   --------
   0)     5096      16   irq_copy_thread_info+0x18/0x70
   1)     5080     336   el1_irq+0x78/0x11c

                                  <<<= _raw_spin_unlock_irqrestore

   2)     4744      48   __skb_recv_datagram+0x148/0x49c
   3)     4696     208   skb_recv_datagram+0x50/0x60
   4)     4488      64   xs_udp_data_ready+0x48/0x170
   5)     4424      96   sock_queue_rcv_skb+0x1fc/0x270
  ...
  53)      344     344   el0_svc_naked+0x20/0x28

__skb_recv_datagram+0x148 is "bl _raw_spin_unlock_irqrestore."

And the frames, #0 and #1, appear here because this patch replaces stack pointers
*after* kernel_entry.

-Takahiro AKASHI

>
> -Takahiro AKASHI
>
>> You can track down my report and Akashi's changes with the following links:
>> - http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/354126.html
>> - https://lkml.org/lkml/2015/7/13/29
>>
>> Although it is impossible to measure an exact depth at this moment, the feature
>> could be utilized to check improvement.
>>
>> Cc'ing Akashi for additional comments if needed.
>>
>> Best Regards
>> Jungseok Lee
>>

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[Jungseok Lee]

On Sep 8, 2015, at 10:45 AM, AKASHI Takahiro wrote:
> Jungseok,

Hi Akashi,

> On 09/08/2015 01:34 AM, Jungseok Lee wrote:
>> On Sep 8, 2015, at 1:06 AM, James Morse wrote:
>>> On 07/09/15 16:48, Jungseok Lee wrote:
>>>> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
>>>> 
>>>> Hi James,
>>>> 
>>>>> Having to handle interrupts on top of an existing kernel stack means the
>>>>> kernel stack must be large enough to accomodate both the maximum kernel
>>>>> usage, and the maximum irq handler usage. Switching to a different stack
>>>>> when processing irqs allows us to make the stack size smaller.
>>>>> 
>>>>> Maximum kernel stack usage (running ltp and generating usb+ethernet
>>>>> interrupts) was 7256 bytes. With this patch, the same workload gives
>>>>> a maximum stack usage of 5816 bytes.
>>>> 
>>>> I'd like to know how to measure the max stack depth.
>>>> AFAIK, a stack tracer on ftrace does not work well. Did you dump a stack
>>>> region and find or track down an untouched region?
>>> 
>>> I enabled the 'Trace max stack' option under menuconfig 'Kernel Hacking' ->
>>> 'Tracers', then looked in debugfs:/tracing/stack_max_size.
>>> 
>>> What problems did you encounter?
>>> (I may be missing something?)
>> 
>> When I enabled the feature, all entries had *0* size except the last entry.
>> It can be reproduced easily as looking in debugs:/tracing/stack_trace.
> 
> I'm afraid that you have not applied one of patches in my RFC:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355919.html

My description is not clear. Whenever I use a stack tracer, I always put your
RFC and Steve's stack tracer fix on top of my tree.

Best Regards
Jungseok Lee

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[AKASHI Takahiro]

On 09/07/2015 11:36 PM, James Morse wrote:
> Having to handle interrupts on top of an existing kernel stack means the
> kernel stack must be large enough to accomodate both the maximum kernel
> usage, and the maximum irq handler usage. Switching to a different stack
> when processing irqs allows us to make the stack size smaller.
>
> Maximum kernel stack usage (running ltp and generating usb+ethernet
> interrupts) was 7256 bytes. With this patch, the same workload gives
> a maximum stack usage of 5816 bytes.

With stack tracer on, the kernel with this patch ran into BUG() in check_stack():
     if (task_stack_end_corrupted(current))
         BUG();

This is because a check for an interrupt stack with "object_is_on_stack(stack)"
has passed, while your irq stack doesn't have a STACK_END_MAGIC.

> +-+
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>   arch/arm64/include/asm/irq.h         | 12 +++++++++
>   arch/arm64/include/asm/thread_info.h |  8 ++++--
>   arch/arm64/kernel/entry.S            | 33 ++++++++++++++++++++---
>   arch/arm64/kernel/irq.c              | 52 ++++++++++++++++++++++++++++++++++++
>   arch/arm64/kernel/smp.c              |  4 +++
>   arch/arm64/kernel/stacktrace.c       |  4 ++-
>   6 files changed, 107 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index bbb251b14746..050d4196c736 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -2,14 +2,20 @@
>   #define __ASM_IRQ_H
>
>   #include <linux/irqchip/arm-gic-acpi.h>
> +#include <linux/percpu.h>
>
>   #include <asm-generic/irq.h>
> +#include <asm/thread_info.h>
> +
> +DECLARE_PER_CPU(unsigned long, irq_sp);
>
>   struct pt_regs;
>
>   extern void migrate_irqs(void);
>   extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
>
> +extern int alloc_irq_stack(unsigned int cpu);
> +
>   static inline void acpi_irq_init(void)
>   {
>   	/*
> @@ -21,4 +27,10 @@ static inline void acpi_irq_init(void)
>   }
>   #define acpi_irq_init acpi_irq_init
>
> +static inline bool is_irq_stack(unsigned long sp)
> +{
> +	struct thread_info *ti = get_thread_info(sp);
> +	return (get_thread_info(per_cpu(irq_sp, ti->cpu)) == ti);
> +}
> +
>   #endif
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index dcd06d18a42a..b906254fc400 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -69,12 +69,16 @@ register unsigned long current_stack_pointer asm ("sp");
>   /*
>    * how to get the thread information struct from C
>    */
> +static inline struct thread_info *get_thread_info(unsigned long sp)
> +{
> +	return (struct thread_info *)(sp & ~(THREAD_SIZE - 1));
> +}
> +
>   static inline struct thread_info *current_thread_info(void) __attribute_const__;
>
>   static inline struct thread_info *current_thread_info(void)
>   {
> -	return (struct thread_info *)
> -		(current_stack_pointer & ~(THREAD_SIZE - 1));
> +	return get_thread_info(current_stack_pointer);
>   }
>
>   #define thread_saved_pc(tsk)	\
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index e16351819fed..d42371f3f5a1 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -190,10 +190,37 @@ tsk	.req	x28		// current thread_info
>    * Interrupt handling.
>    */
>   	.macro	irq_handler
> -	adrp	x1, handle_arch_irq
> -	ldr	x1, [x1, #:lo12:handle_arch_irq]
> -	mov	x0, sp
> +	mrs	x21, tpidr_el1
> +	adr_l	x20, irq_sp
> +	add	x20, x20, x21
> +
> +	ldr	x21, [x20]
> +	mov	x20, sp
> +
> +	mov	x0, x21
> +	mov	x1, x20
> +	bl	irq_copy_thread_info
> +
> +	/* test for recursive use of irq_sp */
> +	cbz	w0, 1f
> +	mrs	x30, elr_el1
> +	mov	sp, x21
> +
> +	/*
> +	 * Create a fake stack frame to bump unwind_frame() onto the original
> +	 * stack. This relies on x29 not being clobbered by kernel_entry().
> +	 */
> +	push	x29, x30

I don't think that it works. Since unwind_frame() doesn't care about sp, but
only does fp, a pushed stack frame will never be dereferenced.

-Takahiro AKASHI


> +
> +1:	ldr_l	x1, handle_arch_irq
> +	mov     x0, x20
>   	blr	x1
> +
> +	mov	x0, x20
> +	mov	x1, x21
> +	bl	irq_copy_thread_info
> +	mov	sp, x20
> +
>   	.endm
>
>   	.text
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index 463fa2e7e34c..10b57a006da8 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -26,11 +26,14 @@
>   #include <linux/smp.h>
>   #include <linux/init.h>
>   #include <linux/irqchip.h>
> +#include <linux/percpu.h>
>   #include <linux/seq_file.h>
>   #include <linux/ratelimit.h>
>
>   unsigned long irq_err_count;
>
> +DEFINE_PER_CPU(unsigned long, irq_sp) = 0;
> +
>   int arch_show_interrupts(struct seq_file *p, int prec)
>   {
>   #ifdef CONFIG_SMP
> @@ -55,6 +58,10 @@ void __init init_IRQ(void)
>   	irqchip_init();
>   	if (!handle_arch_irq)
>   		panic("No interrupt controller found.");
> +
> +	/* Allocate an irq stack for the boot cpu */
> +	if (alloc_irq_stack(smp_processor_id()))
> +		panic("Failed to allocate irq stack for boot cpu.");
>   }
>
>   #ifdef CONFIG_HOTPLUG_CPU
> @@ -117,3 +124,48 @@ void migrate_irqs(void)
>   	local_irq_restore(flags);
>   }
>   #endif /* CONFIG_HOTPLUG_CPU */
> +
> +/* Allocate an irq_stack for a cpu that is about to be brought up. */
> +int alloc_irq_stack(unsigned int cpu)
> +{
> +	struct page *irq_stack_page;
> +	union thread_union *irq_stack;
> +
> +	/* reuse stack allocated previously */
> +	if (per_cpu(irq_sp, cpu))
> +		return 0;
> +
> +	irq_stack_page = alloc_kmem_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
> +	if (!irq_stack_page) {
> +		pr_crit("CPU%u: failed to allocate irq stack for cpu %u\n",
> +			smp_processor_id(), cpu);
> +		return -ENOMEM;
> +	}
> +	irq_stack = page_address(irq_stack_page);
> +
> +	per_cpu(irq_sp, cpu) = (unsigned long)irq_stack->stack
> +			       + THREAD_START_SP;
> +
> +	return 0;
> +}
> +
> +/*
> + * Copy struct thread_info between two stacks, and update current->stack.
> + * This is used when moving to the irq exception stack.
> + * Changing current->stack is necessary so that non-arch thread_info writers
> + * don't use the new thread_info->task->stack to find the old thread_info when
> + * setting flags like TIF_NEED_RESCHED...
> + */
> +asmlinkage int irq_copy_thread_info(unsigned long dst_sp, unsigned long src_sp)
> +{
> +	struct thread_info *src = get_thread_info(src_sp);
> +	struct thread_info *dst = get_thread_info(dst_sp);
> +
> +	if (dst == src)
> +		return 0;
> +
> +	*dst = *src;
> +	current->stack = dst;
> +
> +	return 1;
> +}
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 50fb4696654e..5283dc5629e4 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -89,6 +89,10 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>   {
>   	int ret;
>
> +	ret = alloc_irq_stack(cpu);
> +	if (ret)
> +		return ret;
> +
>   	/*
>   	 * We need to tell the secondary core where to find its stack and the
>   	 * page tables.
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 407991bf79f5..3d6d5b45aa4b 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>
>
>   /*
> @@ -43,7 +44,8 @@ int notrace unwind_frame(struct stackframe *frame)
>   	low  = frame->sp;
>   	high = ALIGN(low, THREAD_SIZE);
>
> -	if (fp < low || fp > high - 0x18 || fp & 0xf)
> +	if ((fp < low || fp > high - 0x18 || fp & 0xf) &&
> +	    !is_irq_stack(frame->sp))
>   		return -EINVAL;
>
>   	frame->sp = fp + 0x10;
>

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[Jungseok Lee]

On Sep 7, 2015, at 11:36 PM, James Morse wrote:

Hi James,

> Having to handle interrupts on top of an existing kernel stack means the
> kernel stack must be large enough to accomodate both the maximum kernel
> usage, and the maximum irq handler usage. Switching to a different stack
> when processing irqs allows us to make the stack size smaller.
> 
> Maximum kernel stack usage (running ltp and generating usb+ethernet
> interrupts) was 7256 bytes. With this patch, the same workload gives
> a maximum stack usage of 5816 bytes.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> arch/arm64/include/asm/irq.h         | 12 +++++++++
> arch/arm64/include/asm/thread_info.h |  8 ++++--
> arch/arm64/kernel/entry.S            | 33 ++++++++++++++++++++---
> arch/arm64/kernel/irq.c              | 52 ++++++++++++++++++++++++++++++++++++
> arch/arm64/kernel/smp.c              |  4 +++
> arch/arm64/kernel/stacktrace.c       |  4 ++-
> 6 files changed, 107 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index bbb251b14746..050d4196c736 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -2,14 +2,20 @@
> #define __ASM_IRQ_H
> 
> #include <linux/irqchip/arm-gic-acpi.h>
> +#include <linux/percpu.h>
> 
> #include <asm-generic/irq.h>
> +#include <asm/thread_info.h>
> +
> +DECLARE_PER_CPU(unsigned long, irq_sp);
> 
> struct pt_regs;
> 
> extern void migrate_irqs(void);
> extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
> 
> +extern int alloc_irq_stack(unsigned int cpu);
> +
> static inline void acpi_irq_init(void)
> {
> 	/*
> @@ -21,4 +27,10 @@ static inline void acpi_irq_init(void)
> }
> #define acpi_irq_init acpi_irq_init
> 
> +static inline bool is_irq_stack(unsigned long sp)
> +{
> +	struct thread_info *ti = get_thread_info(sp);
> +	return (get_thread_info(per_cpu(irq_sp, ti->cpu)) == ti);
> +}
> +
> #endif
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index dcd06d18a42a..b906254fc400 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -69,12 +69,16 @@ register unsigned long current_stack_pointer asm ("sp");
> /*
>  * how to get the thread information struct from C
>  */
> +static inline struct thread_info *get_thread_info(unsigned long sp)
> +{
> +	return (struct thread_info *)(sp & ~(THREAD_SIZE - 1));
> +}
> +
> static inline struct thread_info *current_thread_info(void) __attribute_const__;
> 
> static inline struct thread_info *current_thread_info(void)
> {
> -	return (struct thread_info *)
> -		(current_stack_pointer & ~(THREAD_SIZE - 1));
> +	return get_thread_info(current_stack_pointer);
> }
> 
> #define thread_saved_pc(tsk)	\
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index e16351819fed..d42371f3f5a1 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -190,10 +190,37 @@ tsk	.req	x28		// current thread_info
>  * Interrupt handling.
>  */
> 	.macro	irq_handler
> -	adrp	x1, handle_arch_irq
> -	ldr	x1, [x1, #:lo12:handle_arch_irq]
> -	mov	x0, sp
> +	mrs	x21, tpidr_el1
> +	adr_l	x20, irq_sp
> +	add	x20, x20, x21
> +
> +	ldr	x21, [x20]
> +	mov	x20, sp
> +
> +	mov	x0, x21
> +	mov	x1, x20
> +	bl	irq_copy_thread_info
> +
> +	/* test for recursive use of irq_sp */
> +	cbz	w0, 1f
> +	mrs	x30, elr_el1
> +	mov	sp, x21
> +
> +	/*
> +	 * Create a fake stack frame to bump unwind_frame() onto the original
> +	 * stack. This relies on x29 not being clobbered by kernel_entry().
> +	 */
> +	push	x29, x30

It is the most challenging item to handle a frame pointer in this context.

As I mentioned previously, a stack tracer on ftrace is not supported yet.
How about decoupling "IRQ Stack feature" from "Stack Tracer Fix"?

> +
> +1:	ldr_l	x1, handle_arch_irq
> +	mov     x0, x20
> 	blr	x1
> +
> +	mov	x0, x20
> +	mov	x1, x21
> +	bl	irq_copy_thread_info
> +	mov	sp, x20
> +
> 	.endm
> 
> 	.text
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index 463fa2e7e34c..10b57a006da8 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -26,11 +26,14 @@
> #include <linux/smp.h>
> #include <linux/init.h>
> #include <linux/irqchip.h>
> +#include <linux/percpu.h>
> #include <linux/seq_file.h>
> #include <linux/ratelimit.h>
> 
> unsigned long irq_err_count;
> 
> +DEFINE_PER_CPU(unsigned long, irq_sp) = 0;
> +
> int arch_show_interrupts(struct seq_file *p, int prec)
> {
> #ifdef CONFIG_SMP
> @@ -55,6 +58,10 @@ void __init init_IRQ(void)
> 	irqchip_init();
> 	if (!handle_arch_irq)
> 		panic("No interrupt controller found.");
> +
> +	/* Allocate an irq stack for the boot cpu */
> +	if (alloc_irq_stack(smp_processor_id()))
> +		panic("Failed to allocate irq stack for boot cpu.");
> }
> 
> #ifdef CONFIG_HOTPLUG_CPU
> @@ -117,3 +124,48 @@ void migrate_irqs(void)
> 	local_irq_restore(flags);
> }
> #endif /* CONFIG_HOTPLUG_CPU */
> +
> +/* Allocate an irq_stack for a cpu that is about to be brought up. */
> +int alloc_irq_stack(unsigned int cpu)
> +{
> +	struct page *irq_stack_page;
> +	union thread_union *irq_stack;
> +
> +	/* reuse stack allocated previously */
> +	if (per_cpu(irq_sp, cpu))
> +		return 0;

I'd like to avoid even this simple check since CPU hotplug could be heavily
used for power management.

> +
> +	irq_stack_page = alloc_kmem_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
> +	if (!irq_stack_page) {
> +		pr_crit("CPU%u: failed to allocate irq stack for cpu %u\n",
> +			smp_processor_id(), cpu);
> +		return -ENOMEM;
> +	}
> +	irq_stack = page_address(irq_stack_page);
> +
> +	per_cpu(irq_sp, cpu) = (unsigned long)irq_stack->stack
> +			       + THREAD_START_SP;
> +
> +	return 0;
> +}
> +
> +/*
> + * Copy struct thread_info between two stacks, and update current->stack.
> + * This is used when moving to the irq exception stack.
> + * Changing current->stack is necessary so that non-arch thread_info writers
> + * don't use the new thread_info->task->stack to find the old thread_info when
> + * setting flags like TIF_NEED_RESCHED...
> + */
> +asmlinkage int irq_copy_thread_info(unsigned long dst_sp, unsigned long src_sp)
> +{
> +	struct thread_info *src = get_thread_info(src_sp);
> +	struct thread_info *dst = get_thread_info(dst_sp);
> +
> +	if (dst == src)
> +		return 0;
> +
> +	*dst = *src;
> +	current->stack = dst;
> +
> +	return 1;
> +}

Although this is 32-byte memcpy, sizeof(struct thread_info), this function is called
twice to handle a single interrupt. Isn's it too expensive?

This is a major difference between my approach and this patch. I think we should get
some feedbacks on struct thread_info information management for v2 preparation.

Best Regards
Jungseok Lee

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[James Morse]

On 08/09/15 15:54, Jungseok Lee wrote:
> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
> 
> Hi James,
> 
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index e16351819fed..d42371f3f5a1 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -190,10 +190,37 @@ tsk	.req	x28		// current thread_info
>>  * Interrupt handling.
>>  */
>> 	.macro	irq_handler
>> -	adrp	x1, handle_arch_irq
>> -	ldr	x1, [x1, #:lo12:handle_arch_irq]
>> -	mov	x0, sp
>> +	mrs	x21, tpidr_el1
>> +	adr_l	x20, irq_sp
>> +	add	x20, x20, x21
>> +
>> +	ldr	x21, [x20]
>> +	mov	x20, sp
>> +
>> +	mov	x0, x21
>> +	mov	x1, x20
>> +	bl	irq_copy_thread_info
>> +
>> +	/* test for recursive use of irq_sp */
>> +	cbz	w0, 1f
>> +	mrs	x30, elr_el1
>> +	mov	sp, x21
>> +
>> +	/*
>> +	 * Create a fake stack frame to bump unwind_frame() onto the original
>> +	 * stack. This relies on x29 not being clobbered by kernel_entry().
>> +	 */
>> +	push	x29, x30
> 
> It is the most challenging item to handle a frame pointer in this context.
> 
> As I mentioned previously, a stack tracer on ftrace is not supported yet.
> How about decoupling "IRQ Stack feature" from "Stack Tracer Fix"?

Yes - I discovered today this was more complicated than I thought. I will
need to do some more reading.


>> +
>> +1:	ldr_l	x1, handle_arch_irq
>> +	mov     x0, x20
>> 	blr	x1
>> +
>> +	mov	x0, x20
>> +	mov	x1, x21
>> +	bl	irq_copy_thread_info
>> +	mov	sp, x20
>> +
>> 	.endm
>>
>> 	.text
>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
>> index 463fa2e7e34c..10b57a006da8 100644
>> --- a/arch/arm64/kernel/irq.c
>> +++ b/arch/arm64/kernel/irq.c
>> @@ -26,11 +26,14 @@
>> #include <linux/smp.h>
>> #include <linux/init.h>
>> #include <linux/irqchip.h>
>> +#include <linux/percpu.h>
>> #include <linux/seq_file.h>
>> #include <linux/ratelimit.h>
>>
>> unsigned long irq_err_count;
>>
>> +DEFINE_PER_CPU(unsigned long, irq_sp) = 0;
>> +
>> int arch_show_interrupts(struct seq_file *p, int prec)
>> {
>> #ifdef CONFIG_SMP
>> @@ -55,6 +58,10 @@ void __init init_IRQ(void)
>> 	irqchip_init();
>> 	if (!handle_arch_irq)
>> 		panic("No interrupt controller found.");
>> +
>> +	/* Allocate an irq stack for the boot cpu */
>> +	if (alloc_irq_stack(smp_processor_id()))
>> +		panic("Failed to allocate irq stack for boot cpu.");
>> }
>>
>> #ifdef CONFIG_HOTPLUG_CPU
>> @@ -117,3 +124,48 @@ void migrate_irqs(void)
>> 	local_irq_restore(flags);
>> }
>> #endif /* CONFIG_HOTPLUG_CPU */
>> +
>> +/* Allocate an irq_stack for a cpu that is about to be brought up. */
>> +int alloc_irq_stack(unsigned int cpu)
>> +{
>> +	struct page *irq_stack_page;
>> +	union thread_union *irq_stack;
>> +
>> +	/* reuse stack allocated previously */
>> +	if (per_cpu(irq_sp, cpu))
>> +		return 0;
> 
> I'd like to avoid even this simple check since CPU hotplug could be heavily
> used for power management.

I don't think its a problem:
__cpu_up() contains a call to wait_for_completion_timeout() (which could
eventually end up in the scheduler), so I don't think it could ever be on a
'really hot' path.

For really frequent hotplug-like power management, cpu_suspend() makes use
of firmware support to power-off cores - from what I can see it doesn't use
__cpu_up().


>> +
>> +	irq_stack_page = alloc_kmem_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
>> +	if (!irq_stack_page) {
>> +		pr_crit("CPU%u: failed to allocate irq stack for cpu %u\n",
>> +			smp_processor_id(), cpu);
>> +		return -ENOMEM;
>> +	}
>> +	irq_stack = page_address(irq_stack_page);
>> +
>> +	per_cpu(irq_sp, cpu) = (unsigned long)irq_stack->stack
>> +			       + THREAD_START_SP;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Copy struct thread_info between two stacks, and update current->stack.
>> + * This is used when moving to the irq exception stack.
>> + * Changing current->stack is necessary so that non-arch thread_info writers
>> + * don't use the new thread_info->task->stack to find the old thread_info when
>> + * setting flags like TIF_NEED_RESCHED...
>> + */
>> +asmlinkage int irq_copy_thread_info(unsigned long dst_sp, unsigned long src_sp)
>> +{
>> +	struct thread_info *src = get_thread_info(src_sp);
>> +	struct thread_info *dst = get_thread_info(dst_sp);
>> +
>> +	if (dst == src)
>> +		return 0;
>> +
>> +	*dst = *src;
>> +	current->stack = dst;
>> +
>> +	return 1;
>> +}
> 
> Although this is 32-byte memcpy, sizeof(struct thread_info), this function is called
> twice to handle a single interrupt. Isn's it too expensive?
> 
> This is a major difference between my approach and this patch. I think we should get
> some feedbacks on struct thread_info information management for v2 preparation.

Agreed.

The other difference, as Akashi Takahiro pointed out, is the behaviour of
object_is_on_stack(). What should this return for an object on an irq stack?


Thanks,

James

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[Jungseok Lee]

On Sep 9, 2015, at 1:47 AM, James Morse wrote:
> On 08/09/15 15:54, Jungseok Lee wrote:
>> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
>> 
>> Hi James,
>> 
>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>> index e16351819fed..d42371f3f5a1 100644
>>> --- a/arch/arm64/kernel/entry.S
>>> +++ b/arch/arm64/kernel/entry.S
>>> @@ -190,10 +190,37 @@ tsk	.req	x28		// current thread_info
>>> * Interrupt handling.
>>> */
>>> 	.macro	irq_handler
>>> -	adrp	x1, handle_arch_irq
>>> -	ldr	x1, [x1, #:lo12:handle_arch_irq]
>>> -	mov	x0, sp
>>> +	mrs	x21, tpidr_el1
>>> +	adr_l	x20, irq_sp
>>> +	add	x20, x20, x21
>>> +
>>> +	ldr	x21, [x20]
>>> +	mov	x20, sp
>>> +
>>> +	mov	x0, x21
>>> +	mov	x1, x20
>>> +	bl	irq_copy_thread_info
>>> +
>>> +	/* test for recursive use of irq_sp */
>>> +	cbz	w0, 1f
>>> +	mrs	x30, elr_el1
>>> +	mov	sp, x21
>>> +
>>> +	/*
>>> +	 * Create a fake stack frame to bump unwind_frame() onto the original
>>> +	 * stack. This relies on x29 not being clobbered by kernel_entry().
>>> +	 */
>>> +	push	x29, x30
>> 
>> It is the most challenging item to handle a frame pointer in this context.
>> 
>> As I mentioned previously, a stack tracer on ftrace is not supported yet.
>> How about decoupling "IRQ Stack feature" from "Stack Tracer Fix"?
> 
> Yes - I discovered today this was more complicated than I thought. I will
> need to do some more reading.
> 
> 
>>> +
>>> +1:	ldr_l	x1, handle_arch_irq
>>> +	mov     x0, x20
>>> 	blr	x1
>>> +
>>> +	mov	x0, x20
>>> +	mov	x1, x21
>>> +	bl	irq_copy_thread_info
>>> +	mov	sp, x20
>>> +
>>> 	.endm
>>> 
>>> 	.text
>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
>>> index 463fa2e7e34c..10b57a006da8 100644
>>> --- a/arch/arm64/kernel/irq.c
>>> +++ b/arch/arm64/kernel/irq.c
>>> @@ -26,11 +26,14 @@
>>> #include <linux/smp.h>
>>> #include <linux/init.h>
>>> #include <linux/irqchip.h>
>>> +#include <linux/percpu.h>
>>> #include <linux/seq_file.h>
>>> #include <linux/ratelimit.h>
>>> 
>>> unsigned long irq_err_count;
>>> 
>>> +DEFINE_PER_CPU(unsigned long, irq_sp) = 0;
>>> +
>>> int arch_show_interrupts(struct seq_file *p, int prec)
>>> {
>>> #ifdef CONFIG_SMP
>>> @@ -55,6 +58,10 @@ void __init init_IRQ(void)
>>> 	irqchip_init();
>>> 	if (!handle_arch_irq)
>>> 		panic("No interrupt controller found.");
>>> +
>>> +	/* Allocate an irq stack for the boot cpu */
>>> +	if (alloc_irq_stack(smp_processor_id()))
>>> +		panic("Failed to allocate irq stack for boot cpu.");
>>> }
>>> 
>>> #ifdef CONFIG_HOTPLUG_CPU
>>> @@ -117,3 +124,48 @@ void migrate_irqs(void)
>>> 	local_irq_restore(flags);
>>> }
>>> #endif /* CONFIG_HOTPLUG_CPU */
>>> +
>>> +/* Allocate an irq_stack for a cpu that is about to be brought up. */
>>> +int alloc_irq_stack(unsigned int cpu)
>>> +{
>>> +	struct page *irq_stack_page;
>>> +	union thread_union *irq_stack;
>>> +
>>> +	/* reuse stack allocated previously */
>>> +	if (per_cpu(irq_sp, cpu))
>>> +		return 0;
>> 
>> I'd like to avoid even this simple check since CPU hotplug could be heavily
>> used for power management.
> 
> I don't think its a problem:
> __cpu_up() contains a call to wait_for_completion_timeout() (which could
> eventually end up in the scheduler), so I don't think it could ever be on a
> 'really hot' path.
> 
> For really frequent hotplug-like power management, cpu_suspend() makes use
> of firmware support to power-off cores - from what I can see it doesn't use
> __cpu_up().

In case of some platforms, CPU hotplug is triggered via sysfs for power management
based on user data. What is advantage of putting stack allocation into this path?

IRQ stack allocation is an critical one-shot operation. So, there would be no issue
to give this work to a booting core. 

>>> +
>>> +	irq_stack_page = alloc_kmem_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
>>> +	if (!irq_stack_page) {
>>> +		pr_crit("CPU%u: failed to allocate irq stack for cpu %u\n",
>>> +			smp_processor_id(), cpu);
>>> +		return -ENOMEM;
>>> +	}
>>> +	irq_stack = page_address(irq_stack_page);

I forgot leaving a comment here. How about using __get_free_pages? It returns an
address instead of page. 

>>> +
>>> +	per_cpu(irq_sp, cpu) = (unsigned long)irq_stack->stack
>>> +			       + THREAD_START_SP;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/*
>>> + * Copy struct thread_info between two stacks, and update current->stack.
>>> + * This is used when moving to the irq exception stack.
>>> + * Changing current->stack is necessary so that non-arch thread_info writers
>>> + * don't use the new thread_info->task->stack to find the old thread_info when
>>> + * setting flags like TIF_NEED_RESCHED...
>>> + */
>>> +asmlinkage int irq_copy_thread_info(unsigned long dst_sp, unsigned long src_sp)
>>> +{
>>> +	struct thread_info *src = get_thread_info(src_sp);
>>> +	struct thread_info *dst = get_thread_info(dst_sp);
>>> +
>>> +	if (dst == src)
>>> +		return 0;
>>> +
>>> +	*dst = *src;
>>> +	current->stack = dst;
>>> +
>>> +	return 1;
>>> +}
>> 
>> Although this is 32-byte memcpy, sizeof(struct thread_info), this function is called
>> twice to handle a single interrupt. Isn's it too expensive?
>> 
>> This is a major difference between my approach and this patch. I think we should get
>> some feedbacks on struct thread_info information management for v2 preparation.
> 
> Agreed.
> 
> The other difference, as Akashi Takahiro pointed out, is the behaviour of
> object_is_on_stack(). What should this return for an object on an irq stack?

0 should be returned in that case to align with x86 behavior according to check_stack()
context and the commit, 81520a1b0649d0.

Best Regards
Jungseok Lee

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[James Morse]

On 09/09/15 14:22, Jungseok Lee wrote:
> On Sep 9, 2015, at 1:47 AM, James Morse wrote:
>> On 08/09/15 15:54, Jungseok Lee wrote:
>>> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
>>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
>>>> index 463fa2e7e34c..10b57a006da8 100644
>>>> --- a/arch/arm64/kernel/irq.c
>>>> +++ b/arch/arm64/kernel/irq.c
>>>> @@ -26,11 +26,14 @@
>>>> #include <linux/smp.h>
>>>> #include <linux/init.h>
>>>> #include <linux/irqchip.h>
>>>> +#include <linux/percpu.h>
>>>> #include <linux/seq_file.h>
>>>> #include <linux/ratelimit.h>
>>>>
>>>> unsigned long irq_err_count;
>>>>
>>>> +DEFINE_PER_CPU(unsigned long, irq_sp) = 0;
>>>> +
>>>> int arch_show_interrupts(struct seq_file *p, int prec)
>>>> {
>>>> #ifdef CONFIG_SMP
>>>> @@ -55,6 +58,10 @@ void __init init_IRQ(void)
>>>> 	irqchip_init();
>>>> 	if (!handle_arch_irq)
>>>> 		panic("No interrupt controller found.");
>>>> +
>>>> +	/* Allocate an irq stack for the boot cpu */
>>>> +	if (alloc_irq_stack(smp_processor_id()))
>>>> +		panic("Failed to allocate irq stack for boot cpu.");
>>>> }
>>>>
>>>> #ifdef CONFIG_HOTPLUG_CPU
>>>> @@ -117,3 +124,48 @@ void migrate_irqs(void)
>>>> 	local_irq_restore(flags);
>>>> }
>>>> #endif /* CONFIG_HOTPLUG_CPU */
>>>> +
>>>> +/* Allocate an irq_stack for a cpu that is about to be brought up. */
>>>> +int alloc_irq_stack(unsigned int cpu)
>>>> +{
>>>> +	struct page *irq_stack_page;
>>>> +	union thread_union *irq_stack;
>>>> +
>>>> +	/* reuse stack allocated previously */
>>>> +	if (per_cpu(irq_sp, cpu))
>>>> +		return 0;
>>>
>>> I'd like to avoid even this simple check since CPU hotplug could be heavily
>>> used for power management.
>>
>> I don't think its a problem:
>> __cpu_up() contains a call to wait_for_completion_timeout() (which could
>> eventually end up in the scheduler), so I don't think it could ever be on a
>> 'really hot' path.
>>
>> For really frequent hotplug-like power management, cpu_suspend() makes use
>> of firmware support to power-off cores - from what I can see it doesn't use
>> __cpu_up().
> 
> In case of some platforms, CPU hotplug is triggered via sysfs for power management
> based on user data. What is advantage of putting stack allocation into this path?

It will only happen for CPUs that are brought up.


> IRQ stack allocation is an critical one-shot operation. So, there would be no issue
> to give this work to a booting core. 

I agree, but:

>From include/linux/cpumask.h:
> *  If HOTPLUG is enabled, then cpu_possible_mask is forced to have
> *  all NR_CPUS bits set, otherwise it is just the set of CPUs that
> *  ACPI reports present at boot.

(This doesn't seem to happen with DT - but might with ACPI.)

NR_CPUs could be much bigger than the number of cpus the system ever has.
Allocating a stack for every possible cpu would waste memory. It is better
to do it just-in-time, when we know the memory will be used.

This already happens for the per-cpu idle task, (please check I traced
these through correctly!)
_cpu_up()
  idle_thread_get()
    init_idle()
      fork_idle()
        copy_process()
          dup_task_struct()
            alloc_task_struct_node()
            alloc_thread_info_node()
            arch_dup_task_struct()

So plenty of memory-allocation occurs during _cpu_up(), idle_init() checks
whether the idle task has already been created.


>>>> +
>>>> +	irq_stack_page = alloc_kmem_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
>>>> +	if (!irq_stack_page) {
>>>> +		pr_crit("CPU%u: failed to allocate irq stack for cpu %u\n",
>>>> +			smp_processor_id(), cpu);
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +	irq_stack = page_address(irq_stack_page);
> 
> I forgot leaving a comment here. How about using __get_free_pages? It returns an
> address instead of page. 

Good idea, I was paranoid about the stack not being correctly aligned (as
current_thread_info() relies on this). I lifted that alloc_kmem_pages line
from kernel/fork.c.


>>>> +
>>>> +	per_cpu(irq_sp, cpu) = (unsigned long)irq_stack->stack
>>>> +			       + THREAD_START_SP;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Copy struct thread_info between two stacks, and update current->stack.
>>>> + * This is used when moving to the irq exception stack.
>>>> + * Changing current->stack is necessary so that non-arch thread_info writers
>>>> + * don't use the new thread_info->task->stack to find the old thread_info when
>>>> + * setting flags like TIF_NEED_RESCHED...
>>>> + */
>>>> +asmlinkage int irq_copy_thread_info(unsigned long dst_sp, unsigned long src_sp)
>>>> +{
>>>> +	struct thread_info *src = get_thread_info(src_sp);
>>>> +	struct thread_info *dst = get_thread_info(dst_sp);
>>>> +
>>>> +	if (dst == src)
>>>> +		return 0;
>>>> +
>>>> +	*dst = *src;
>>>> +	current->stack = dst;
>>>> +
>>>> +	return 1;
>>>> +}
>>>
>>> Although this is 32-byte memcpy, sizeof(struct thread_info), this function is called
>>> twice to handle a single interrupt. Isn's it too expensive?
>>>
>>> This is a major difference between my approach and this patch. I think we should get
>>> some feedbacks on struct thread_info information management for v2 preparation.
>>
>> Agreed.
>>
>> The other difference, as Akashi Takahiro pointed out, is the behaviour of
>> object_is_on_stack(). What should this return for an object on an irq stack?
> 
> 0 should be returned in that case to align with x86 behavior according to check_stack()
> context and the commit, 81520a1b0649d0.

Unless we can update ftrace on x86 too! :)


Thanks for your comments,


James

[PATCH] arm64: kernel: Use a separate stack for irq interrupts.

[Jungseok Lee]

On Sep 10, 2015, at 3:13 AM, James Morse wrote:
> On 09/09/15 14:22, Jungseok Lee wrote:
>> On Sep 9, 2015, at 1:47 AM, James Morse wrote:
>>> On 08/09/15 15:54, Jungseok Lee wrote:
>>>> On Sep 7, 2015, at 11:36 PM, James Morse wrote:
>>>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
>>>>> index 463fa2e7e34c..10b57a006da8 100644
>>>>> --- a/arch/arm64/kernel/irq.c
>>>>> +++ b/arch/arm64/kernel/irq.c
>>>>> @@ -26,11 +26,14 @@
>>>>> #include <linux/smp.h>
>>>>> #include <linux/init.h>
>>>>> #include <linux/irqchip.h>
>>>>> +#include <linux/percpu.h>
>>>>> #include <linux/seq_file.h>
>>>>> #include <linux/ratelimit.h>
>>>>> 
>>>>> unsigned long irq_err_count;
>>>>> 
>>>>> +DEFINE_PER_CPU(unsigned long, irq_sp) = 0;
>>>>> +
>>>>> int arch_show_interrupts(struct seq_file *p, int prec)
>>>>> {
>>>>> #ifdef CONFIG_SMP
>>>>> @@ -55,6 +58,10 @@ void __init init_IRQ(void)
>>>>> 	irqchip_init();
>>>>> 	if (!handle_arch_irq)
>>>>> 		panic("No interrupt controller found.");
>>>>> +
>>>>> +	/* Allocate an irq stack for the boot cpu */
>>>>> +	if (alloc_irq_stack(smp_processor_id()))
>>>>> +		panic("Failed to allocate irq stack for boot cpu.");
>>>>> }
>>>>> 
>>>>> #ifdef CONFIG_HOTPLUG_CPU
>>>>> @@ -117,3 +124,48 @@ void migrate_irqs(void)
>>>>> 	local_irq_restore(flags);
>>>>> }
>>>>> #endif /* CONFIG_HOTPLUG_CPU */
>>>>> +
>>>>> +/* Allocate an irq_stack for a cpu that is about to be brought up. */
>>>>> +int alloc_irq_stack(unsigned int cpu)
>>>>> +{
>>>>> +	struct page *irq_stack_page;
>>>>> +	union thread_union *irq_stack;
>>>>> +
>>>>> +	/* reuse stack allocated previously */
>>>>> +	if (per_cpu(irq_sp, cpu))
>>>>> +		return 0;
>>>> 
>>>> I'd like to avoid even this simple check since CPU hotplug could be heavily
>>>> used for power management.
>>> 
>>> I don't think its a problem:
>>> __cpu_up() contains a call to wait_for_completion_timeout() (which could
>>> eventually end up in the scheduler), so I don't think it could ever be on a
>>> 'really hot' path.
>>> 
>>> For really frequent hotplug-like power management, cpu_suspend() makes use
>>> of firmware support to power-off cores - from what I can see it doesn't use
>>> __cpu_up().
>> 
>> In case of some platforms, CPU hotplug is triggered via sysfs for power management
>> based on user data. What is advantage of putting stack allocation into this path?
> 
> It will only happen for CPUs that are brought up.
> 
> 
>> IRQ stack allocation is an critical one-shot operation. So, there would be no issue
>> to give this work to a booting core. 
> 
> I agree, but:
> 
> From include/linux/cpumask.h:
>> *  If HOTPLUG is enabled, then cpu_possible_mask is forced to have
>> *  all NR_CPUS bits set, otherwise it is just the set of CPUs that
>> *  ACPI reports present at boot.
> 
> (This doesn't seem to happen with DT - but might with ACPI.)
> 
> NR_CPUs could be much bigger than the number of cpus the system ever has.
> Allocating a stack for every possible cpu would waste memory. It is better
> to do it just-in-time, when we know the memory will be used.

Frankly I've not considered that kind of system, but this feature should be
supported smoothly for that system. I will move the allocation logic in v2.

> This already happens for the per-cpu idle task, (please check I traced
> these through correctly!)
> _cpu_up()
>  idle_thread_get()
>    init_idle()
>      fork_idle()
>        copy_process()
>          dup_task_struct()
>            alloc_task_struct_node()
>            alloc_thread_info_node()
>            arch_dup_task_struct()
> 
> So plenty of memory-allocation occurs during _cpu_up(), idle_init() checks
> whether the idle task has already been created.

Got it.

Thanks for the feedbacks.

Best Regards
Jungseok Lee

[RFC PATCH 0/3] Implement IRQ stack on ARM64

[Jungseok Lee]

On Sep 7, 2015, at 11:33 PM, James Morse wrote:
> On 04/09/15 15:23, Jungseok Lee wrote:
>> ARM64 kernel allocates 16KB kernel stack when creating a process. In case
>> of low memory platforms with tough workloads on userland, this order-2
>> allocation request reaches to memory pressure and performance degradation
>> simultaenously since VM page allocator falls into slowpath frequently,
>> which triggers page reclaim and compaction.
>> 
>> I believe that one of the best solutions is to reduce kernel stack size.
>> According to the following data from stack tracer with some fixes, [1],
>> a separate IRQ stack would greatly help to decrease a kernel stack depth.
>> 
> 
> Hi Jungseok Lee,

Hi James Morse,

> I was working on a similar patch for irq stack, (patch as a follow up email).
> 
> I suggest we work together on a single implementation. I think the only
> major difference is that you're using sp_el0 as a temporary register to
> store a copy of the stack-pointer to find struct thread_info, whereas I was
> copying it between stacks (ends up as 2x ldp/stps), which keeps the change
> restricted to irq_stack setup code.
> 
> We should get some feedback as to which approach is preferred.

Great idea!
I'd really like to figure out the most ideal implementation of this feature.

Best Regards
Jungseok Lee