[PATCH 0/3] IRQ stack support for ARM

[PATCH 0/3] IRQ stack support for ARM

[Maninder Singh]

Observed Stack Overflow on 8KB kernel stack on ARM specially 
incase on network interrupts, which results in undeterministic behaviour. 
So there is need for per cpu dedicated IRQ stack for ARM.

As ARm does not have extra co-processor register
to save thread info pointer, IRQ stack will be at some
performance cost, so code is under CONFIG_IRQ_STACK.

and we don't have much knowledge and set up for CLANG
and ARM_UNWIND, so dependency added for both cases.

Tested patch set with QEMU for latest kernel
and 4.1 kernel for ARM target with same patch set.

Maninder Singh, Vaneet Narang (3):
  arm: introduce self pointer in thread info
  arm: introduce IRQ stacks
  arm: Modify stack trace and dump for use with irq stack

 arch/arm/Kconfig                   | 10 ++++++++
 arch/arm/include/asm/assembler.h   | 11 +++++++++
 arch/arm/include/asm/irq.h         | 13 +++++++++++
 arch/arm/include/asm/thread_info.h | 27 ++++++++++++++++++++++
 arch/arm/kernel/entry-armv.S       | 41 ++++++++++++++++++++++++++++++++-
 arch/arm/kernel/irq.c              | 24 +++++++++++++++++++
 arch/arm/kernel/stacktrace.c       | 21 +++++++++++++++++
 arch/arm/kernel/traps.c            | 47 +++++++++++++++++++++++++++++++++++---
 arch/arm/lib/backtrace.S           | 18 +++++++++++++++
 include/linux/thread_info.h        |  4 ++++
 kernel/fork.c                      |  1 +
 11 files changed, 213 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH 1/3] arm: introduce self pointer in thread info

[Maninder Singh]

self pointer to thread info is added in thread info.
It is base change required for IRQ stack on ARM.

both stacks will have pointer to thread_info at bottom.

Co-developed-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 arch/arm/Kconfig                   | 10 ++++++++++
 arch/arm/include/asm/assembler.h   |  3 +++
 arch/arm/include/asm/thread_info.h | 27 +++++++++++++++++++++++++++
 include/linux/thread_info.h        |  4 ++++
 kernel/fork.c                      |  1 +
 5 files changed, 45 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index fe2f17eb2..434442f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1667,6 +1667,16 @@ config STACKPROTECTOR_PER_TASK
 	  Enable this option to switch to a different method that uses a
 	  different canary value for each task.
 
+config IRQ_STACK
+	bool "use separate stacks for Interrupts"
+	default n
+	depends on FRAME_POINTER && !CC_IS_CLANG
+	help
+	  Select this option to use separate stacks for Interrupt handling code.
+	  It will add latency in fetching thread info of one more derefer operation
+	  and add latency in Interrupt serve at time as for each Interrupt, thread_info
+	  pointer needs to be stored at bottom of interrupt stack.
+
 endmenu
 
 menu "Boot options"
diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index fce52eed..8512bdc 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -207,6 +207,9 @@
  THUMB(	mov	\rd, sp			)
  THUMB(	lsr	\rd, \rd, #THREAD_SIZE_ORDER + PAGE_SHIFT	)
 	mov	\rd, \rd, lsl #THREAD_SIZE_ORDER + PAGE_SHIFT
+#ifdef CONFIG_IRQ_STACK
+	ldr	\rd, [\rd]
+#endif
 	.endm
 
 /*
diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 536b6b9..a4d5f76 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -44,6 +44,9 @@ struct cpu_context_save {
  * __switch_to() assumes cpu_context follows immediately after cpu_domain.
  */
 struct thread_info {
+#ifdef CONFIG_IRQ_STACK
+	void			*tinfo_ptr;	/* pointer to self thread_info */
+#endif
 	unsigned long		flags;		/* low level flags */
 	int			preempt_count;	/* 0 => preemptable, <0 => bug */
 	mm_segment_t		addr_limit;	/* address limit */
@@ -67,14 +70,33 @@ struct thread_info {
 #endif
 };
 
+
+#ifdef CONFIG_IRQ_STACK
+#define INIT_THREAD_SELF_PTR				\
+	.tinfo_ptr	= &init_thread_union.thread_info,
+#else
+#define INIT_THREAD_SELF_PTR
+#endif
+
 #define INIT_THREAD_INFO(tsk)						\
 {									\
+	INIT_THREAD_SELF_PTR						\
 	.task		= &tsk,						\
 	.flags		= 0,						\
 	.preempt_count	= INIT_PREEMPT_COUNT,				\
 	.addr_limit	= KERNEL_DS,					\
 }
 
+
+#ifdef CONFIG_IRQ_STACK
+#define TASK_THREAD_SELF_POINTER(tsk)				\
+{								\
+	struct thread_info *ti = task_thread_info(tsk);		\
+								\
+	ti->tinfo_ptr = ti;					\
+}
+#endif
+
 /*
  * how to get the thread information struct from C
  */
@@ -82,8 +104,13 @@ struct thread_info {
 
 static inline struct thread_info *current_thread_info(void)
 {
+#ifdef CONFIG_IRQ_STACK
+	return (struct thread_info *)
+		(*((unsigned long *)(current_stack_pointer & ~(THREAD_SIZE - 1))));
+#else
 	return (struct thread_info *)
 		(current_stack_pointer & ~(THREAD_SIZE - 1));
+#endif
 }
 
 #define thread_saved_pc(tsk)	\
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index e93e249..ddf7b43 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -43,6 +43,10 @@ enum {
 #define THREAD_ALIGN	THREAD_SIZE
 #endif
 
+#ifndef TASK_THREAD_SELF_POINTER
+#define TASK_THREAD_SELF_POINTER(tsk)
+#endif
+
 #define THREADINFO_GFP		(GFP_KERNEL_ACCOUNT | __GFP_ZERO)
 
 /*
diff --git a/kernel/fork.c b/kernel/fork.c
index 7ef3eb3..d53f5eb 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -901,6 +901,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node)
 #endif
 
 	setup_thread_stack(tsk, orig);
+	TASK_THREAD_SELF_POINTER(tsk);
 	clear_user_return_notifier(tsk);
 	clear_tsk_need_resched(tsk);
 	set_task_stack_end_magic(tsk);
-- 
1.9.1

[PATCH 2/3] arm: introduce IRQ stacks

[Maninder Singh]

This patch adds code for switching to IRQ stack.
IRQ stack and Kernel SVC stack have below design.

IRQ STACK:-
                    ------------ IRQ stack top
                    |          |
                    ------------
                    .          .
                    .          .
                    .          .
                    ------------
                    |    sp    | <- irq_stack_base + 0x8
                    ------------
                    |    fp    | <- irq_stack_base + 0x4
                    ------------
                    |tinfo_ptr | /* pointer to thread info */
irq_stack_ptr -->   ------------ IRQ stack base

Kernel SVC stack:-
                    ------------  Kernel stack top
                    |          |
                    ------------
                    .          .
                    .          .
                    .          .
                    ------------
                    |          |
                    |          |
                    ------------
                    |tinfo_ptr |  /* pointer to thread info */
                    ------------ Kernel stack base

Co-developed-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 arch/arm/include/asm/assembler.h |  8 ++++++++
 arch/arm/include/asm/irq.h       |  6 ++++++
 arch/arm/kernel/entry-armv.S     | 41 +++++++++++++++++++++++++++++++++++++++-
 arch/arm/kernel/irq.c            | 24 +++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 8512bdc..82ee6ee 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -212,6 +212,14 @@
 #endif
 	.endm
 
+	.macro	this_cpu_ptr, sym, reg, tmp
+	ldr	\reg, =\sym
+#if defined(CONFIG_SMP) && !defined(CONFIG_CPU_V6)
+	mrc	p15, 0, \tmp, cr13, c0, 4
+	add	\reg, \reg, \tmp
+#endif
+	.endm
+
 /*
  * Increment/decrement the preempt count.
  */
diff --git a/arch/arm/include/asm/irq.h b/arch/arm/include/asm/irq.h
index 46d4114..f3299ab 100644
--- a/arch/arm/include/asm/irq.h
+++ b/arch/arm/include/asm/irq.h
@@ -22,10 +22,16 @@
 #define NO_IRQ	((unsigned int)(-1))
 #endif
 
+#define IRQ_STACK_SIZE	THREAD_SIZE
+
 #ifndef __ASSEMBLY__
 struct irqaction;
 struct pt_regs;
 
+#ifdef CONFIG_IRQ_STACK
+DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
+#endif
+
 extern void asm_do_IRQ(unsigned int, struct pt_regs *);
 void handle_IRQ(unsigned int, struct pt_regs *);
 void init_IRQ(void);
diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 55a47df..13a5889 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -32,6 +32,43 @@
 #include "entry-header.S"
 #include <asm/entry-macro-multi.S>
 #include <asm/probes.h>
+#ifdef CONFIG_IRQ_STACK
+#include <asm/irq.h>
+#endif
+
+	.macro  irq_stack_entry
+#ifdef CONFIG_IRQ_STACK
+	mov	r6, sp	/* preserve sp */
+
+	this_cpu_ptr irq_stack_ptr, r7, r8
+	ldr	r7, [r7]
+	mov	r8, r7
+
+	/*
+	 * Compare sp with base of IRQ stack.
+	 * if the top ~(#THREAD_SIZE_ORDER + PAGE_SHIFT) bits match,
+	 * we are on a irq stack.
+	 */
+	eor	r8, r8, sp
+	lsrs	r8, #THREAD_SIZE_ORDER + PAGE_SHIFT
+	beq	9998f
+
+	/*
+	 * store thread info pointer on IRQ stack and
+	 * switch to the irq stack.
+	 */
+	get_thread_info r8
+	stm	r7, {r8, fp, sp}
+	add	sp, r7, #IRQ_STACK_SIZE
+9998:
+#endif
+        .endm
+
+	.macro	irq_stack_exit
+#ifdef CONFIG_IRQ_STACK
+	mov	sp, r6	/* restore sp */
+#endif
+       .endm
 
 /*
  * Interrupt handling.
@@ -41,11 +78,13 @@
 	ldr	r1, =handle_arch_irq
 	mov	r0, sp
 	badr	lr, 9997f
+	irq_stack_entry
 	ldr	pc, [r1]
+9997:
+	irq_stack_exit
 #else
 	arch_irq_handler_default
 #endif
-9997:
 	.endm
 
 	.macro	pabt_helper
diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 698b6f6..79872e5 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -43,6 +43,15 @@
 
 unsigned long irq_err_count;
 
+#ifdef CONFIG_IRQ_STACK
+DEFINE_PER_CPU(unsigned long *, irq_stack_ptr);
+
+/*
+ * irq_stack must be IRQ_STACK_SIZE(THREAD_SIZE) aligned,
+ */
+DEFINE_PER_CPU(unsigned long [IRQ_STACK_SIZE/sizeof(long)], irq_stack) __aligned(IRQ_STACK_SIZE);
+#endif
+
 int arch_show_interrupts(struct seq_file *p, int prec)
 {
 #ifdef CONFIG_FIQ
@@ -55,6 +64,20 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 	return 0;
 }
 
+#ifdef CONFIG_IRQ_STACK
+static void init_irq_stacks(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		per_cpu(irq_stack_ptr, cpu) = per_cpu(irq_stack, cpu);
+}
+#else
+static inline void init_irq_stacks(void)
+{
+}
+#endif
+
 /*
  * handle_IRQ handles all hardware IRQ's.  Decoded IRQs should
  * not come via this function.  Instead, they should provide their
@@ -79,6 +102,7 @@ void __init init_IRQ(void)
 {
 	int ret;
 
+	init_irq_stacks();
 	if (IS_ENABLED(CONFIG_OF) && !machine_desc->init_irq)
 		irqchip_init();
 	else
-- 
1.9.1

[PATCH 3/3] arm: Modify stack trace and dump for use with irq stack

[Maninder Singh]

This patch allows unwind_frame() to traverse from interrupt stack to task
stack correctly.

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

c_backtrace() is also modified on same logic, when traversing from last
IRQ frame, update fp with SVC mode fp.

Co-developed-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 arch/arm/include/asm/irq.h   |  7 +++++++
 arch/arm/kernel/stacktrace.c | 21 ++++++++++++++++++++
 arch/arm/kernel/traps.c      | 47 +++++++++++++++++++++++++++++++++++++++++---
 arch/arm/lib/backtrace.S     | 18 +++++++++++++++++
 4 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/irq.h b/arch/arm/include/asm/irq.h
index f3299ab..d4c66e9 100644
--- a/arch/arm/include/asm/irq.h
+++ b/arch/arm/include/asm/irq.h
@@ -30,6 +30,13 @@
 
 #ifdef CONFIG_IRQ_STACK
 DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
+
+#define IRQ_STACK_BASE_PTR	(unsigned long)(raw_cpu_read(irq_stack_ptr))
+#define IRQ_STACK_TOP_PTR	(unsigned long)(raw_cpu_read(irq_stack_ptr) + IRQ_STACK_SIZE)
+
+#define IRQ_STACK_TO_TASK_FRAME(ptr)	(*((unsigned long *)(ptr + 0x4)))
+#define IRQ_STACK_TO_TASK_STACK(ptr)	(*((unsigned long *)(ptr + 0x8)))
+
 #endif
 
 extern void asm_do_IRQ(unsigned int, struct pt_regs *);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 76ea417..65b9634 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -7,6 +7,9 @@
 #include <asm/sections.h>
 #include <asm/stacktrace.h>
 #include <asm/traps.h>
+#ifdef CONFIG_IRQ_STACK
+#include <asm/irq.h>
+#endif
 
 #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
 /*
@@ -42,6 +45,13 @@ int notrace unwind_frame(struct stackframe *frame)
 {
 	unsigned long high, low;
 	unsigned long fp = frame->fp;
+#ifdef CONFIG_IRQ_STACK
+	unsigned long irq_stack_base_p;
+	unsigned long irq_stack_p;
+
+	irq_stack_base_p = IRQ_STACK_BASE_PTR;
+	irq_stack_p = irq_stack_base_p + IRQ_STACK_SIZE;
+#endif
 
 	/* only go to a higher address on the stack */
 	low = frame->sp;
@@ -67,6 +77,17 @@ int notrace unwind_frame(struct stackframe *frame)
 	frame->pc = *(unsigned long *)(fp - 4);
 #endif
 
+#ifdef CONFIG_IRQ_STACK
+	/*
+	 * Check whether we are going to walk through from interrupt stack
+	 * to task stack.
+	 * If we reach the end of the stack - and its an interrupt stack,
+	 * read the original task stack pointer base + 4 of IRQ stack.
+	 */
+	if (frame->sp == irq_stack_p)
+		frame->sp = IRQ_STACK_TO_TASK_STACK(irq_stack_base_p);
+#endif
+
 	return 0;
 }
 #endif
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 17d5a78..36b0cda 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -65,8 +65,20 @@ static int __init user_debug_setup(char *str)
 void dump_backtrace_entry(unsigned long where, unsigned long from,
 			  unsigned long frame, const char *loglvl)
 {
-	unsigned long end = frame + 4 + sizeof(struct pt_regs);
+	unsigned long end;
 
+#ifdef CONFIG_IRQ_STACK
+	unsigned long irq_stack_base_p;
+	unsigned long irq_stack_p;
+
+	irq_stack_base_p = IRQ_STACK_BASE_PTR;
+	irq_stack_p = irq_stack_base_p + IRQ_STACK_SIZE;
+
+	if (frame < irq_stack_p && (frame + sizeof(struct pt_regs)) > irq_stack_p)
+		frame = IRQ_STACK_TO_TASK_STACK(irq_stack_base_p) - 4;
+#endif
+
+	end = frame + 4 + sizeof(struct pt_regs);
 #ifdef CONFIG_KALLSYMS
 	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
 		loglvl, where, (void *)where, from, (void *)from);
@@ -113,6 +125,35 @@ static int verify_stack(unsigned long sp)
 
 	return 0;
 }
+
+#ifdef CONFIG_IRQ_STACK
+static int fp_underflow(unsigned long fp, struct task_struct *tsk)
+{
+	unsigned long end = (unsigned long)end_of_stack(tsk);
+	unsigned long irq_stack_base_p;
+	unsigned long irq_stack_p;
+
+	irq_stack_base_p = IRQ_STACK_BASE_PTR;
+	irq_stack_p = irq_stack_base_p + IRQ_STACK_SIZE;
+
+
+	if (fp > end && fp < end + THREAD_SIZE)
+		return 0;
+
+	if (fp > irq_stack_base_p && fp < irq_stack_p)
+		return 0;
+
+	return 1;
+}
+#else
+static int fp_underflow(unsigned long fp, struct task_struct *tsk)
+{
+	if (fp < (unsigned long)end_of_stack(tsk))
+		return 1;
+
+	return 0;
+}
+#endif
 #endif
 
 /*
@@ -238,7 +279,7 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 	} else if (verify_stack(fp)) {
 		pr_cont("invalid frame pointer 0x%08x", fp);
 		ok = 0;
-	} else if (fp < (unsigned long)end_of_stack(tsk))
+	} else if (fp_underflow(fp, tsk))
 		pr_cont("frame pointer underflow");
 	pr_cont("\n");
 
@@ -292,7 +333,7 @@ static int __die(const char *str, int err, struct pt_regs *regs)
 
 	if (!user_mode(regs) || in_interrupt()) {
 		dump_mem(KERN_EMERG, "Stack: ", regs->ARM_sp,
-			 THREAD_SIZE + (unsigned long)task_stack_page(tsk));
+			 THREAD_SIZE + (unsigned long)(regs->ARM_sp & ~(THREAD_SIZE - 1)));
 		dump_backtrace(regs, tsk, KERN_EMERG);
 		dump_instr(KERN_EMERG, regs);
 	}
diff --git a/arch/arm/lib/backtrace.S b/arch/arm/lib/backtrace.S
index 872f658..1a8e645 100644
--- a/arch/arm/lib/backtrace.S
+++ b/arch/arm/lib/backtrace.S
@@ -9,6 +9,9 @@
 #include <linux/kern_levels.h>
 #include <linux/linkage.h>
 #include <asm/assembler.h>
+#ifdef CONFIG_IRQ_STACK
+#include <asm/irq.h>
+#endif
 		.text
 
 @ fp is 0 or stack frame
@@ -96,6 +99,21 @@ for_each_frame:	tst	frame, mask		@ Check for address exceptions
 		teq	sv_fp, #0		@ zero saved fp means
 		beq	no_frame		@ no further frames
 
+#ifdef CONFIG_IRQ_STACK
+		/*
+		 * check if it is swtiching from IRQ to SVC,
+		 * then update frame accordingly.
+		 */
+		this_cpu_ptr irq_stack_ptr r2 r3
+		ldr	r3, [r2]
+		add	r2, r3, #IRQ_STACK_SIZE
+		ldr	r0, [frame, #-8]
+		cmp	r2, r0
+		ldreq	sv_fp, [r3, #4]
+		moveq	frame, sv_fp
+		beq	for_each_frame
+#endif
+
 		cmp	sv_fp, frame		@ next frame must be
 		mov	frame, sv_fp		@ above the current frame
 		bhi	for_each_frame
-- 
1.9.1

Re: [PATCH 0/3] IRQ stack support for ARM

[Sebastian Andrzej Siewior]

On 2020-10-08 12:45:30 [+0530], Maninder Singh wrote:
> Observed Stack Overflow on 8KB kernel stack on ARM specially 
> incase on network interrupts, which results in undeterministic behaviour. 
> So there is need for per cpu dedicated IRQ stack for ARM.

You could try to look where this stack overflow is coming from. If this
is limited to softirq processing/NAPI (since you mentioned network) you
could try implementing do_softirq_own_stack().

Sebastian

Re: [PATCH 0/3] IRQ stack support for ARM

[Russell King - ARM Linux admin]

On Thu, Oct 08, 2020 at 12:45:30PM +0530, Maninder Singh wrote:
> Observed Stack Overflow on 8KB kernel stack on ARM specially 
> incase on network interrupts, which results in undeterministic behaviour. 
> So there is need for per cpu dedicated IRQ stack for ARM.
> 
> As ARm does not have extra co-processor register
> to save thread info pointer, IRQ stack will be at some
> performance cost, so code is under CONFIG_IRQ_STACK.
> 
> and we don't have much knowledge and set up for CLANG
> and ARM_UNWIND, so dependency added for both cases.
> 
> Tested patch set with QEMU for latest kernel
> and 4.1 kernel for ARM target with same patch set.

You need to investigate and show where and why this is happening. My
guess is you have a network driver that uses a lot of kernel stack
space, which itself would be a bug.

Note that there are compiler versions out there that mis-optimise and
eat stack space - the kernel build should be warning if a function
uses a large amount of stack.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 0/3] IRQ stack support for ARM

[Nick Desaulniers]

On Thu, Oct 8, 2020 at 1:30 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Oct 08, 2020 at 12:45:30PM +0530, Maninder Singh wrote:
> > Observed Stack Overflow on 8KB kernel stack on ARM specially
> > incase on network interrupts, which results in undeterministic behaviour.
> > So there is need for per cpu dedicated IRQ stack for ARM.
> >
> > As ARm does not have extra co-processor register
> > to save thread info pointer, IRQ stack will be at some
> > performance cost, so code is under CONFIG_IRQ_STACK.
> >
> > and we don't have much knowledge and set up for CLANG
> > and ARM_UNWIND, so dependency added for both cases.
> >
> > Tested patch set with QEMU for latest kernel
> > and 4.1 kernel for ARM target with same patch set.
>
> You need to investigate and show where and why this is happening. My
> guess is you have a network driver that uses a lot of kernel stack
> space, which itself would be a bug.
>
> Note that there are compiler versions out there that mis-optimise and
> eat stack space - the kernel build should be warning if a function
> uses a large amount of stack.

For tracking down those not-super-helpful compiler warnings, I wrote a
tool where if you rebuild with debug info, and give it the object file
and string of the function the compiler warned about it will parse the
DWARF to tell you the size of each local variable, and if it came from
an inline frame.  Generally, it's possible to stack allocate something
that's way too big; instead those should be allocated on the heap.
https://github.com/ClangBuiltLinux/frame-larger-than
(I haven't had time to sit down and use it to resolve all outstanding
issues, but it has worked well for me in the past)
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 0/3] IRQ stack support for ARM

[Florian Fainelli]

On 10/15/20 1:59 PM, Nick Desaulniers wrote:
> On Thu, Oct 8, 2020 at 1:30 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
>>
>> On Thu, Oct 08, 2020 at 12:45:30PM +0530, Maninder Singh wrote:
>>> Observed Stack Overflow on 8KB kernel stack on ARM specially
>>> incase on network interrupts, which results in undeterministic behaviour.
>>> So there is need for per cpu dedicated IRQ stack for ARM.
>>>
>>> As ARm does not have extra co-processor register
>>> to save thread info pointer, IRQ stack will be at some
>>> performance cost, so code is under CONFIG_IRQ_STACK.
>>>
>>> and we don't have much knowledge and set up for CLANG
>>> and ARM_UNWIND, so dependency added for both cases.
>>>
>>> Tested patch set with QEMU for latest kernel
>>> and 4.1 kernel for ARM target with same patch set.
>>
>> You need to investigate and show where and why this is happening. My
>> guess is you have a network driver that uses a lot of kernel stack
>> space, which itself would be a bug.
>>
>> Note that there are compiler versions out there that mis-optimise and
>> eat stack space - the kernel build should be warning if a function
>> uses a large amount of stack.
> 
> For tracking down those not-super-helpful compiler warnings, I wrote a
> tool where if you rebuild with debug info, and give it the object file
> and string of the function the compiler warned about it will parse the
> DWARF to tell you the size of each local variable, and if it came from
> an inline frame.  Generally, it's possible to stack allocate something
> that's way too big; instead those should be allocated on the heap.
> https://github.com/ClangBuiltLinux/frame-larger-than
> (I haven't had time to sit down and use it to resolve all outstanding
> issues, but it has worked well for me in the past)

Things get a bit more difficult with the network stack and you easily
recurse into functions and blow up the stack size. This is especially
true if you have some complex network tunneling or filtering going on.

For one, in the 4.1 kernel that appears to have been used as a basis for
this work, if you have CONFIG_BPF enabled but not
CONFIG_BPF_JIT_ALWAYS_ON, __bpf_prog_run will require about 724 bytes of
stack last I measured, that's nearly 10% of the stack that goes away
just like that.
--
Florian

Re: [PATCH 0/3] IRQ stack support for ARM

[Arnd Bergmann]

(replying to my own mail, apparently my normal outgoing email server is
blacklisted, so resending from @kernel.org)

On Fri, Oct 16, 2020 at 12:09 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Oct 8, 2020 at 10:32 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Thu, Oct 08, 2020 at 12:45:30PM +0530, Maninder Singh wrote:
> > > Observed Stack Overflow on 8KB kernel stack on ARM specially
> > > incase on network interrupts, which results in undeterministic behaviour.
> > > So there is need for per cpu dedicated IRQ stack for ARM.
> > >
> > > As ARm does not have extra co-processor register
> > > to save thread info pointer, IRQ stack will be at some
> > > performance cost, so code is under CONFIG_IRQ_STACK.
> > >
> > > and we don't have much knowledge and set up for CLANG
> > > and ARM_UNWIND, so dependency added for both cases.
> > >
> > > Tested patch set with QEMU for latest kernel
> > > and 4.1 kernel for ARM target with same patch set.
> >
> > You need to investigate and show where and why this is happening. My
> > guess is you have a network driver that uses a lot of kernel stack
> > space, which itself would be a bug.
>
> Agreed.
>
> > Note that there are compiler versions out there that mis-optimise and
> > eat stack space - the kernel build should be warning if a function
> > uses a large amount of stack.
>
> Some more ideas for figuring it out:
>
> CONFIG_DEBUG_STACK_USAGE may also be helpful in identifying
> code paths that are deeply nested with multiple functions taking a
> lot of stack space, but each one staying under the limit.
>
> CONFIG_DEBUG_STACKOVERFLOW would also help here but
> is not supported on Arm at the moment. There was a patch[1] from
> Uwe Kleine-König to add this, and I suppose we should still add
> that, in particular if it helps debug this problem.
>
> CONFIG_VMAP_STACK is probably the best way to debug
> random runtime stack overflows because using a guard page
> turns random memory corruption into an immediate oops,
> but I don't think there is an implementation for Arm yet and
> using a lot of vmalloc space means we might not be able to
> default to this.
>
> Regardless of identifying and fixing the bug Maninder found, I
> also think that supporting separate async stacks on Arm is useful
> for determinism. Most of the popular architectures use irqstack
> for this reason, and I was actually surprised that we don't do it
> on arch/arm/.
>
>      Arnd
>
> [1] https://lore.kernel.org/linux-arm-kernel/20200108082913.29710-1-u.kleine-koenig@pengutronix.de/

Re: [PATCH 0/3] IRQ stack support for ARM

[Russell King - ARM Linux admin]

On Wed, Oct 21, 2020 at 01:58:21PM +0200, Arnd Bergmann wrote:
> (replying to my own mail, apparently my normal outgoing email server is
> blacklisted, so resending from @kernel.org)
> 
> On Fri, Oct 16, 2020 at 12:09 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Thu, Oct 8, 2020 at 10:32 AM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > > On Thu, Oct 08, 2020 at 12:45:30PM +0530, Maninder Singh wrote:
> > > > Observed Stack Overflow on 8KB kernel stack on ARM specially
> > > > incase on network interrupts, which results in undeterministic behaviour.
> > > > So there is need for per cpu dedicated IRQ stack for ARM.
> > > >
> > > > As ARm does not have extra co-processor register
> > > > to save thread info pointer, IRQ stack will be at some
> > > > performance cost, so code is under CONFIG_IRQ_STACK.
> > > >
> > > > and we don't have much knowledge and set up for CLANG
> > > > and ARM_UNWIND, so dependency added for both cases.
> > > >
> > > > Tested patch set with QEMU for latest kernel
> > > > and 4.1 kernel for ARM target with same patch set.
> > >
> > > You need to investigate and show where and why this is happening. My
> > > guess is you have a network driver that uses a lot of kernel stack
> > > space, which itself would be a bug.
> >
> > Agreed.
> >
> > > Note that there are compiler versions out there that mis-optimise and
> > > eat stack space - the kernel build should be warning if a function
> > > uses a large amount of stack.
> >
> > Some more ideas for figuring it out:
> >
> > CONFIG_DEBUG_STACK_USAGE may also be helpful in identifying
> > code paths that are deeply nested with multiple functions taking a
> > lot of stack space, but each one staying under the limit.
> >
> > CONFIG_DEBUG_STACKOVERFLOW would also help here but
> > is not supported on Arm at the moment. There was a patch[1] from
> > Uwe Kleine-König to add this, and I suppose we should still add
> > that, in particular if it helps debug this problem.
> >
> > CONFIG_VMAP_STACK is probably the best way to debug
> > random runtime stack overflows because using a guard page
> > turns random memory corruption into an immediate oops,
> > but I don't think there is an implementation for Arm yet and
> > using a lot of vmalloc space means we might not be able to
> > default to this.
> >
> > Regardless of identifying and fixing the bug Maninder found, I
> > also think that supporting separate async stacks on Arm is useful
> > for determinism. Most of the popular architectures use irqstack
> > for this reason, and I was actually surprised that we don't do it
> > on arch/arm/.
> >
> >      Arnd
> >
> > [1] https://lore.kernel.org/linux-arm-kernel/20200108082913.29710-1-u.kleine-koenig@pengutronix.de/

We don't do it because we don't have a separate register to be able
to store the thread_info pointer, and copying that lump between the
SVC and IRQ stack will add massively to IRQ latency, especially for
older machines.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

(also resending this reply from @kernel.org)

On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> >
> > This patch adds code for switching to IRQ stack.
> > IRQ stack and Kernel SVC stack have below design.
> >
> > IRQ STACK:-
> >                     ------------ IRQ stack top
> >                     |          |
> >                     ------------
> >                     .          .
> >                     .          .
> >                     .          .
> >                     ------------
> >                     |    sp    | <- irq_stack_base + 0x8
> >                     ------------
> >                     |    fp    | <- irq_stack_base + 0x4
> >                     ------------
> >                     |tinfo_ptr | /* pointer to thread info */
> > irq_stack_ptr -->   ------------ IRQ stack base
> >
> > Kernel SVC stack:-
> >                     ------------  Kernel stack top
> >                     |          |
> >                     ------------
> >                     .          .
> >                     .          .
> >                     .          .
> >                     ------------
> >                     |          |
> >                     |          |
> >                     ------------
> >                     |tinfo_ptr |  /* pointer to thread info */
> >                     ------------ Kernel stack base
>
> The extra indirection doesn't look great, and I don't see any of the
> other architectures need that. Since we can access percpu data
> without going through thread_info, maybe doing the same as
> x86 would work here:
>
> - define 'current' as 'this_cpu_read_stable(current_task);'
> - convert to CONFIG_THREAD_INFO_IN_TASK
>
>        Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Russell King - ARM Linux admin]

On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> (also resending this reply from @kernel.org)
> 
> On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> > >
> > > This patch adds code for switching to IRQ stack.
> > > IRQ stack and Kernel SVC stack have below design.
> > >
> > > IRQ STACK:-
> > >                     ------------ IRQ stack top
> > >                     |          |
> > >                     ------------
> > >                     .          .
> > >                     .          .
> > >                     .          .
> > >                     ------------
> > >                     |    sp    | <- irq_stack_base + 0x8
> > >                     ------------
> > >                     |    fp    | <- irq_stack_base + 0x4
> > >                     ------------
> > >                     |tinfo_ptr | /* pointer to thread info */
> > > irq_stack_ptr -->   ------------ IRQ stack base
> > >
> > > Kernel SVC stack:-
> > >                     ------------  Kernel stack top
> > >                     |          |
> > >                     ------------
> > >                     .          .
> > >                     .          .
> > >                     .          .
> > >                     ------------
> > >                     |          |
> > >                     |          |
> > >                     ------------
> > >                     |tinfo_ptr |  /* pointer to thread info */
> > >                     ------------ Kernel stack base
> >
> > The extra indirection doesn't look great, and I don't see any of the
> > other architectures need that. Since we can access percpu data
> > without going through thread_info, maybe doing the same as
> > x86 would work here:
> >
> > - define 'current' as 'this_cpu_read_stable(current_task);'
> > - convert to CONFIG_THREAD_INFO_IN_TASK

That means we need to also code that up in assembly - remember, we
need to access thread_info from assembly code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 0/3] IRQ stack support for ARM

[Arnd Bergmann]

On Wed, Oct 21, 2020 at 2:34 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> We don't do it because we don't have a separate register to be able
> to store the thread_info pointer, and copying that lump between the
> SVC and IRQ stack will add massively to IRQ latency, especially for
> older machines.

I forwarded my other reply as well, in which I suggested using
CONFIG_THREAD_INFO_IN_TASK, wouldn't that solve the problem?

      Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Wed, Oct 21, 2020 at 2:45 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> > >
> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
>
> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

Are there any obvious places that need patching aside from
.macro get_thread_info? I did expect the above conversion to
be somewhat more complicated than Maninder's original patch,
but that part seems easy enough.

       Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Russell King - ARM Linux admin]

On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> On Wed, Oct 21, 2020 at 02:42:48PM +0200, Arnd Bergmann wrote:
> > (also resending this reply from @kernel.org)
> > 
> > On Fri, Oct 16, 2020 at 12:19 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Thu, Oct 8, 2020 at 9:20 AM Maninder Singh <maninder1.s@samsung.com> wrote:
> > > >
> > > > This patch adds code for switching to IRQ stack.
> > > > IRQ stack and Kernel SVC stack have below design.
> > > >
> > > > IRQ STACK:-
> > > >                     ------------ IRQ stack top
> > > >                     |          |
> > > >                     ------------
> > > >                     .          .
> > > >                     .          .
> > > >                     .          .
> > > >                     ------------
> > > >                     |    sp    | <- irq_stack_base + 0x8
> > > >                     ------------
> > > >                     |    fp    | <- irq_stack_base + 0x4
> > > >                     ------------
> > > >                     |tinfo_ptr | /* pointer to thread info */
> > > > irq_stack_ptr -->   ------------ IRQ stack base
> > > >
> > > > Kernel SVC stack:-
> > > >                     ------------  Kernel stack top
> > > >                     |          |
> > > >                     ------------
> > > >                     .          .
> > > >                     .          .
> > > >                     .          .
> > > >                     ------------
> > > >                     |          |
> > > >                     |          |
> > > >                     ------------
> > > >                     |tinfo_ptr |  /* pointer to thread info */
> > > >                     ------------ Kernel stack base
> > >
> > > The extra indirection doesn't look great, and I don't see any of the
> > > other architectures need that. Since we can access percpu data
> > > without going through thread_info, maybe doing the same as
> > > x86 would work here:
> > >
> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
> 
> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

Note also that there is a circular dependency involved. If you make
thread_info accessible via per-cpu, then:

#ifndef __my_cpu_offset
#define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
#endif
#ifdef CONFIG_DEBUG_PREEMPT
#define my_cpu_offset per_cpu_offset(smp_processor_id())
#else
#define my_cpu_offset __my_cpu_offset
#endif

smp_processor_id() ultimately ends up as raw_smp_processor_id() which
is:

#define raw_smp_processor_id() (current_thread_info()->cpu)

and if current_thread_info() itself involves reading from per-cpu data,
we end up recursing... infinitely.

This is why I said in the other thread:

"We don't do it because we don't have a separate register to be able
to store the thread_info pointer, and copying that lump between the
SVC and IRQ stack will add massively to IRQ latency, especially for
older machines."

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH 2/3] arm: introduce IRQ stacks

[Vaneet Narang]

Hi Russel & Arnd,

> > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > - convert to CONFIG_THREAD_INFO_IN_TASK
> 
unlike ARM64 ARM doesn't suport dedicated register like "sp_el0", so 
arm still stores thread info on stack and fetches using current task using
stack pointer (SP) so CONFIG_THREAD_INFO_IN_TASK is not supported on ARM. 
To implement solution inline with current ARM design, we have used indirection.

> That means we need to also code that up in assembly - remember, we
> need to access thread_info from assembly code.

> Note also that there is a circular dependency involved. If you make
> thread_info accessible via per-cpu, then:

> "We don't do it because we don't have a separate register to be able
> to store the thread_info pointer, and copying that lump between the
> SVC and IRQ stack will add massively to IRQ latency, especially for
> older machines."

We tried to minimize latency in switching between SVC stack and IRQ stack
by just copying extra thread info pointer to IRQ stack. Apart from this, 
Most of the code of SVC to IRQ switching is similar to ARM64 implementation.

Also We tried to minimize latency of get_current() by introducting self pointer in
SVC stack to avoid if check to determine, is current is called from SVC context
or IRQ context. This way will always do one extra indirection in get_current, 
irrespective get_current is called from SVC or IRQ context.

Yes we agree still there will be minor additional latency on accessing current
task and SVC to IRQ switching  which might be significant for older machines,
So this is the reason why we kept this sol. under kconfig. This solution
provides extra stack to kernel developers for further development,instead of increasing
stack size to 16KB or spending time on debugging stack overflow issues.

PS: We have debugged and resolved stack overflow issue but we still implemented this sol. to avoid
debugging such issues in future also it gives us flexibility for kernel enhancement on ARM.

Thanks & Regards,
Vaneet Narang

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Wed, Oct 21, 2020 at 2:57 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> > > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > > - convert to CONFIG_THREAD_INFO_IN_TASK
> >
> > That means we need to also code that up in assembly - remember, we
> > need to access thread_info from assembly code.
>
> Note also that there is a circular dependency involved. If you make
> thread_info accessible via per-cpu, then:
>
> #ifndef __my_cpu_offset
> #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
> #endif
> #ifdef CONFIG_DEBUG_PREEMPT
> #define my_cpu_offset per_cpu_offset(smp_processor_id())
> #else
> #define my_cpu_offset __my_cpu_offset
> #endif

Right, I had missed the fallback path using asm-generic/percpu.h
that is used with CONFIG_SMP && CONFIG_CPU_V6
Almost everything either uses fixed percpu data (on UP builds)
or TPIDRPRW when building a v7-only or v6k/v7 kernel without
v6 support.

> smp_processor_id() ultimately ends up as raw_smp_processor_id() which
> is:
>
> #define raw_smp_processor_id() (current_thread_info()->cpu)
>
> and if current_thread_info() itself involves reading from per-cpu data,
> we end up recursing... infinitely.
>
> This is why I said in the other thread:
>
> "We don't do it because we don't have a separate register to be able
> to store the thread_info pointer, and copying that lump between the
> SVC and IRQ stack will add massively to IRQ latency, especially for
> older machines."

As discussed on IRC, I think it can still be done in one of these
ways, though admittedly none of them are perfect:

a) add runtime patching for __my_cpu_offset() when
  CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
  fallback for for SMP&&CPU_V6. It possibly also speeds up
  running on single-cpu systems if the TPIDRPRW access adds
  any measurable runtime overhead compared to patching it out.

b) If irq stacks are left as a compile-time option, that could be
  made conditional on "!(SMP&&CPU_V6)". Presumably very
  few people still run kernels built that way any more. The only
  supported platforms are i.MX3, OMAP2 and Realview-eb, all of
  which are fairly uncommon these days and would usually
  run v6-only non-SMP kernels.

c) If we decide that we no longer care about that configuration
  at all, we could decide to just make SMP depend on !CPU_V6,
  and possibly kill off the entire SMP_ON_UP patching logic.
  I suspect we still want to keep SMP_ON_UP for performance
  reasons, but I don't know how significant they are to start with.

       Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201021 16:07]:
> On Wed, Oct 21, 2020 at 2:57 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Wed, Oct 21, 2020 at 01:45:42PM +0100, Russell King - ARM Linux admin wrote:
> > > > > - define 'current' as 'this_cpu_read_stable(current_task);'
> > > > > - convert to CONFIG_THREAD_INFO_IN_TASK
> > >
> > > That means we need to also code that up in assembly - remember, we
> > > need to access thread_info from assembly code.
> >
> > Note also that there is a circular dependency involved. If you make
> > thread_info accessible via per-cpu, then:
> >
> > #ifndef __my_cpu_offset
> > #define __my_cpu_offset per_cpu_offset(raw_smp_processor_id())
> > #endif
> > #ifdef CONFIG_DEBUG_PREEMPT
> > #define my_cpu_offset per_cpu_offset(smp_processor_id())
> > #else
> > #define my_cpu_offset __my_cpu_offset
> > #endif
> 
> Right, I had missed the fallback path using asm-generic/percpu.h
> that is used with CONFIG_SMP && CONFIG_CPU_V6
> Almost everything either uses fixed percpu data (on UP builds)
> or TPIDRPRW when building a v7-only or v6k/v7 kernel without
> v6 support.
> 
> > smp_processor_id() ultimately ends up as raw_smp_processor_id() which
> > is:
> >
> > #define raw_smp_processor_id() (current_thread_info()->cpu)
> >
> > and if current_thread_info() itself involves reading from per-cpu data,
> > we end up recursing... infinitely.
> >
> > This is why I said in the other thread:
> >
> > "We don't do it because we don't have a separate register to be able
> > to store the thread_info pointer, and copying that lump between the
> > SVC and IRQ stack will add massively to IRQ latency, especially for
> > older machines."
> 
> As discussed on IRC, I think it can still be done in one of these
> ways, though admittedly none of them are perfect:
> 
> a) add runtime patching for __my_cpu_offset() when
>   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
>   fallback for for SMP&&CPU_V6. It possibly also speeds up
>   running on single-cpu systems if the TPIDRPRW access adds
>   any measurable runtime overhead compared to patching it out.

Out of these options a) sounds best to me.

> b) If irq stacks are left as a compile-time option, that could be
>   made conditional on "!(SMP&&CPU_V6)". Presumably very
>   few people still run kernels built that way any more. The only
>   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
>   which are fairly uncommon these days and would usually
>   run v6-only non-SMP kernels.

This has been working just fine for years though. In general,
removing the conditional compile ifdefferey has made things quite
a bit easier for us, so let's continue on that.

> c) If we decide that we no longer care about that configuration
>   at all, we could decide to just make SMP depend on !CPU_V6,
>   and possibly kill off the entire SMP_ON_UP patching logic.
>   I suspect we still want to keep SMP_ON_UP for performance
>   reasons, but I don't know how significant they are to start with.

And this too has been working just fine for years :)

Regards,

Tony

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > As discussed on IRC, I think it can still be done in one of these
> > ways, though admittedly none of them are perfect:
> >
> > a) add runtime patching for __my_cpu_offset() when
> >   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
> >   fallback for for SMP&&CPU_V6. It possibly also speeds up
> >   running on single-cpu systems if the TPIDRPRW access adds
> >   any measurable runtime overhead compared to patching it out.
>
> Out of these options a) sounds best to me.

Ok. Maninder, would you like to give implementing this a try?

> > b) If irq stacks are left as a compile-time option, that could be
> >   made conditional on "!(SMP&&CPU_V6)". Presumably very
> >   few people still run kernels built that way any more. The only
> >   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
> >   which are fairly uncommon these days and would usually
> >   run v6-only non-SMP kernels.
>
> This has been working just fine for years though. In general,
> removing the conditional compile ifdefferey has made things quite
> a bit easier for us, so let's continue on that.
>
> > c) If we decide that we no longer care about that configuration
> >   at all, we could decide to just make SMP depend on !CPU_V6,
> >   and possibly kill off the entire SMP_ON_UP patching logic.
> >   I suspect we still want to keep SMP_ON_UP for performance
> >   reasons, but I don't know how significant they are to start with.
>
> And this too has been working just fine for years :)

I know it works, my point was that I'm not sure anyone cares
any more ;-)

I suppose the existence of omap2plus_defconfig and
imx_v6_v7_defconfig means it does at least get tested
regularly.

       Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > As discussed on IRC, I think it can still be done in one of these
> > > ways, though admittedly none of them are perfect:
> > >
> > > a) add runtime patching for __my_cpu_offset() when
> > >   CONFIG_SMP_ON_UP is set. This adds complexity but avoids the
> > >   fallback for for SMP&&CPU_V6. It possibly also speeds up
> > >   running on single-cpu systems if the TPIDRPRW access adds
> > >   any measurable runtime overhead compared to patching it out.
> >
> > Out of these options a) sounds best to me.
> 
> Ok. Maninder, would you like to give implementing this a try?
> 
> > > b) If irq stacks are left as a compile-time option, that could be
> > >   made conditional on "!(SMP&&CPU_V6)". Presumably very
> > >   few people still run kernels built that way any more. The only
> > >   supported platforms are i.MX3, OMAP2 and Realview-eb, all of
> > >   which are fairly uncommon these days and would usually
> > >   run v6-only non-SMP kernels.
> >
> > This has been working just fine for years though. In general,
> > removing the conditional compile ifdefferey has made things quite
> > a bit easier for us, so let's continue on that.
> >
> > > c) If we decide that we no longer care about that configuration
> > >   at all, we could decide to just make SMP depend on !CPU_V6,
> > >   and possibly kill off the entire SMP_ON_UP patching logic.
> > >   I suspect we still want to keep SMP_ON_UP for performance
> > >   reasons, but I don't know how significant they are to start with.
> >
> > And this too has been working just fine for years :)
> 
> I know it works, my point was that I'm not sure anyone cares
> any more ;-)

Well for example whatever Linux running ARMv6 LTE modems out there might
need to be supported for quite some time. Not sure how many of them are
able to update kernels though. Certainly network security related issues
would be a good reason to update the kernels.

> I suppose the existence of omap2plus_defconfig and
> imx_v6_v7_defconfig means it does at least get tested
> regularly.

Yes. I probably would just stop any random ARMv6 related testing if
it it needs a custom .config.

Regards,

Tony

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Tue, Nov 10, 2020 at 10:19 AM Tony Lindgren <tony@atomide.com> wrote:
> * Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> > On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > I know it works, my point was that I'm not sure anyone cares
> > any more ;-)
>
> Well for example whatever Linux running ARMv6 LTE modems out there might
> need to be supported for quite some time. Not sure how many of them are
> able to update kernels though. Certainly network security related issues
> would be a good reason to update the kernels.

While I agree they should update their kernels, I suspect none of those
modems do. I am however certain that none of them are running an
SMP-enabled multiplatform kernel on an ARM1136r0!

Are these actually ARMv6? Most ARM11 cores you'd come across
in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
in particular every SoC that has any mainline support except for
the ARM1136r0 based OMAP2 and i.MX3.

        Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201110 10:04]:
> On Tue, Nov 10, 2020 at 10:19 AM Tony Lindgren <tony@atomide.com> wrote:
> > * Arnd Bergmann <arnd@kernel.org> [201109 19:10]:
> > > On Mon, Nov 9, 2020 at 3:45 PM Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > I know it works, my point was that I'm not sure anyone cares
> > > any more ;-)
> >
> > Well for example whatever Linux running ARMv6 LTE modems out there might
> > need to be supported for quite some time. Not sure how many of them are
> > able to update kernels though. Certainly network security related issues
> > would be a good reason to update the kernels.
> 
> While I agree they should update their kernels, I suspect none of those
> modems do. I am however certain that none of them are running an
> SMP-enabled multiplatform kernel on an ARM1136r0!

Nope, AFAIK all the SMP parts are ARMv6K :)

> Are these actually ARMv6? Most ARM11 cores you'd come across
> in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> in particular every SoC that has any mainline support except for
> the ARM1136r0 based OMAP2 and i.MX3.

I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
for omap2, no SMP on those.

Regards,

Tony

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Arnd Bergmann]

On Tue, Nov 10, 2020 at 1:06 PM Tony Lindgren <tony@atomide.com> wrote:

> > Are these actually ARMv6? Most ARM11 cores you'd come across
> > in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> > in particular every SoC that has any mainline support except for
> > the ARM1136r0 based OMAP2 and i.MX3.
>
> I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
> for omap2, no SMP on those.

Obviously all SMP hardware is ARMv6K, the only question I raised
in point "c)" is what we would lose by making ARMv6 (ARM1136r0)
support and SMP mutually exclusive in a kernel configuration, and
I suppose the answer remains "testing".

      Arnd

Re: [PATCH 2/3] arm: introduce IRQ stacks

[Tony Lindgren]

* Arnd Bergmann <arnd@kernel.org> [201110 13:35]:
> On Tue, Nov 10, 2020 at 1:06 PM Tony Lindgren <tony@atomide.com> wrote:
> 
> > > Are these actually ARMv6? Most ARM11 cores you'd come across
> > > in practice are ARMv6K (ARM1136r1, ARM1167, ARM11MPCore),
> > > in particular every SoC that has any mainline support except for
> > > the ARM1136r0 based OMAP2 and i.MX3.
> >
> > I've been only using smp_on_up for the ARMv6 ARM1136r0 variants
> > for omap2, no SMP on those.
> 
> Obviously all SMP hardware is ARMv6K, the only question I raised
> in point "c)" is what we would lose by making ARMv6 (ARM1136r0)
> support and SMP mutually exclusive in a kernel configuration, and
> I suppose the answer remains "testing".

Agreed that is probably the biggest reason to keep it at this point.

Regards,

Tony