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