Re: [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@xxxxxxx>
> ---
> 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--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/