From: AKASHI Takahiro <takahiro.akashi@linaro.org> To: James Morse <james.morse@arm.com>, Jungseok Lee <jungseoklee85@gmail.com> Cc: catalin.marinas@arm.com, will.deacon@arm.com, linux-arm-kernel@lists.infradead.org, mark.rutland@arm.com, barami97@gmail.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Date: Wed, 14 Oct 2015 16:13:31 +0900 [thread overview] Message-ID: <561E009B.3070001@linaro.org> (raw) In-Reply-To: <5617CE26.10604@arm.com> On 10/09/2015 11:24 PM, James Morse wrote: > Hi Jungseok, > > On 07/10/15 16:28, Jungseok Lee wrote: >> Currently, a call trace drops a process stack walk when a separate IRQ >> stack is used. It makes a call trace information much less useful when >> a system gets paniked in interrupt context. > > panicked > >> This patch addresses the issue with the following schemes: >> >> - Store aborted stack frame data >> - Decide whether another stack walk is needed or not via current sp >> - Loosen the frame pointer upper bound condition > > It may be worth merging this patch with its predecessor - anyone trying to > bisect a problem could land between these two patches, and spend time > debugging the truncated call traces. > > >> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h >> index 6ea82e8..e5904a1 100644 >> --- a/arch/arm64/include/asm/irq.h >> +++ b/arch/arm64/include/asm/irq.h >> @@ -2,13 +2,25 @@ >> #define __ASM_IRQ_H >> >> #include <linux/irqchip/arm-gic-acpi.h> >> +#include <asm/stacktrace.h> >> >> #include <asm-generic/irq.h> >> >> struct irq_stack { >> void *stack; >> + struct stackframe frame; >> }; >> >> +DECLARE_PER_CPU(struct irq_stack, irq_stacks); > > Good idea, storing this in the per-cpu data makes it immune to stack > corruption. Is this the only reason that you have a dummy stack frame in per-cpu data? By placing this frame in an interrupt stack, I think, we will be able to eliminate changes in dump_stace(). and > >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >> index 407991b..5124649 100644 >> --- a/arch/arm64/kernel/stacktrace.c >> +++ b/arch/arm64/kernel/stacktrace.c >> @@ -43,7 +43,27 @@ int notrace unwind_frame(struct stackframe *frame) >> low = frame->sp; >> high = ALIGN(low, THREAD_SIZE); >> >> - if (fp < low || fp > high - 0x18 || fp & 0xf) >> + /* >> + * A frame pointer would reach an upper bound if a prologue of the >> + * first function of call trace looks as follows: >> + * >> + * stp x29, x30, [sp,#-16]! >> + * mov x29, sp >> + * >> + * Thus, the upper bound is (top of stack - 0x20) with consideration > > The terms 'top' and 'bottom' of the stack are confusing, your 'top' appears > to be the highest address, which is used first, making it the bottom of the > stack. > > I would try to use the terms low/est and high/est, in keeping with the > variable names in use here. > > >> + * of a 16-byte empty space in THREAD_START_SP. >> + * >> + * The value, 0x20, however, does not cover all cases as interrupts >> + * are handled using a separate stack. That is, a call trace can start >> + * from elx_irq exception vectors. The symbols could not be promoted >> + * to candidates for a stack trace under the restriction, 0x20. >> + * >> + * The scenario is handled without complexity as 1) considering >> + * (bottom of stack + THREAD_START_SP) as a dummy frame pointer, the >> + * content of which is 0, and 2) allowing the case, which changes >> + * the value to 0x10 from 0x20. > > Where has 0x20 come from? The old value was 0x18. > > My understanding is the highest part of the stack looks like this: > high [ off-stack ] > high - 0x08 [ left free by THREAD_START_SP ] > high - 0x10 [ left free by THREAD_START_SP ] > high - 0x18 [#1 x30 ] > high - 0x20 [#1 x29 ] > > So the condition 'fp > high - 0x18' prevents returning either 'left free' > address, or off-stack-value as a frame. Changing it to 'fp > high - 0x10' > allows the first half of that reserved area to be a valid stack frame. > > This change is breaking perf using incantations [0] and [1]: > > Before, with just patch 1/2: > ---__do_softirq > | > |--92.95%-- __handle_domain_irq > | __irqentry_text_start > | el1_irq > | > > After, with both patches: > ---__do_softirq > | > |--83.83%-- __handle_domain_irq > | __irqentry_text_start > | el1_irq > | | > | |--99.39%-- 0x400008040d00000c > | --0.61%-- [...] > | This also shows that walk_stackframe() doesn't walk through a process stack. Now I'm trying the following hack on top of Jungseok's patch. (It doesn't traverse from an irq stack to an process stack yet. I need modify unwind_frame().) Thanks, -Takahiro AKASHI ----8<---- diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 650cc05..5fbd1ea 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -185,14 +185,12 @@ alternative_endif mov x23, sp and x23, x23, #~(THREAD_SIZE - 1) cmp x20, x23 // check irq re-enterance + mov x19, sp beq 1f - str x29, [x19, #IRQ_FRAME_FP] - str x21, [x19, #IRQ_FRAME_SP] - str x22, [x19, #IRQ_FRAME_PC] - mov x29, x24 -1: mov x19, sp - csel x23, x19, x24, eq // x24 = top of irq stack - mov sp, x23 + mov sp, x24 // x24 = top of irq stack + stp x29, x22, [sp, #-32]! + mov x29, sp +1: .endm /*
WARNING: multiple messages have this Message-ID (diff)
From: takahiro.akashi@linaro.org (AKASHI Takahiro) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Date: Wed, 14 Oct 2015 16:13:31 +0900 [thread overview] Message-ID: <561E009B.3070001@linaro.org> (raw) In-Reply-To: <5617CE26.10604@arm.com> On 10/09/2015 11:24 PM, James Morse wrote: > Hi Jungseok, > > On 07/10/15 16:28, Jungseok Lee wrote: >> Currently, a call trace drops a process stack walk when a separate IRQ >> stack is used. It makes a call trace information much less useful when >> a system gets paniked in interrupt context. > > panicked > >> This patch addresses the issue with the following schemes: >> >> - Store aborted stack frame data >> - Decide whether another stack walk is needed or not via current sp >> - Loosen the frame pointer upper bound condition > > It may be worth merging this patch with its predecessor - anyone trying to > bisect a problem could land between these two patches, and spend time > debugging the truncated call traces. > > >> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h >> index 6ea82e8..e5904a1 100644 >> --- a/arch/arm64/include/asm/irq.h >> +++ b/arch/arm64/include/asm/irq.h >> @@ -2,13 +2,25 @@ >> #define __ASM_IRQ_H >> >> #include <linux/irqchip/arm-gic-acpi.h> >> +#include <asm/stacktrace.h> >> >> #include <asm-generic/irq.h> >> >> struct irq_stack { >> void *stack; >> + struct stackframe frame; >> }; >> >> +DECLARE_PER_CPU(struct irq_stack, irq_stacks); > > Good idea, storing this in the per-cpu data makes it immune to stack > corruption. Is this the only reason that you have a dummy stack frame in per-cpu data? By placing this frame in an interrupt stack, I think, we will be able to eliminate changes in dump_stace(). and > >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >> index 407991b..5124649 100644 >> --- a/arch/arm64/kernel/stacktrace.c >> +++ b/arch/arm64/kernel/stacktrace.c >> @@ -43,7 +43,27 @@ int notrace unwind_frame(struct stackframe *frame) >> low = frame->sp; >> high = ALIGN(low, THREAD_SIZE); >> >> - if (fp < low || fp > high - 0x18 || fp & 0xf) >> + /* >> + * A frame pointer would reach an upper bound if a prologue of the >> + * first function of call trace looks as follows: >> + * >> + * stp x29, x30, [sp,#-16]! >> + * mov x29, sp >> + * >> + * Thus, the upper bound is (top of stack - 0x20) with consideration > > The terms 'top' and 'bottom' of the stack are confusing, your 'top' appears > to be the highest address, which is used first, making it the bottom of the > stack. > > I would try to use the terms low/est and high/est, in keeping with the > variable names in use here. > > >> + * of a 16-byte empty space in THREAD_START_SP. >> + * >> + * The value, 0x20, however, does not cover all cases as interrupts >> + * are handled using a separate stack. That is, a call trace can start >> + * from elx_irq exception vectors. The symbols could not be promoted >> + * to candidates for a stack trace under the restriction, 0x20. >> + * >> + * The scenario is handled without complexity as 1) considering >> + * (bottom of stack + THREAD_START_SP) as a dummy frame pointer, the >> + * content of which is 0, and 2) allowing the case, which changes >> + * the value to 0x10 from 0x20. > > Where has 0x20 come from? The old value was 0x18. > > My understanding is the highest part of the stack looks like this: > high [ off-stack ] > high - 0x08 [ left free by THREAD_START_SP ] > high - 0x10 [ left free by THREAD_START_SP ] > high - 0x18 [#1 x30 ] > high - 0x20 [#1 x29 ] > > So the condition 'fp > high - 0x18' prevents returning either 'left free' > address, or off-stack-value as a frame. Changing it to 'fp > high - 0x10' > allows the first half of that reserved area to be a valid stack frame. > > This change is breaking perf using incantations [0] and [1]: > > Before, with just patch 1/2: > ---__do_softirq > | > |--92.95%-- __handle_domain_irq > | __irqentry_text_start > | el1_irq > | > > After, with both patches: > ---__do_softirq > | > |--83.83%-- __handle_domain_irq > | __irqentry_text_start > | el1_irq > | | > | |--99.39%-- 0x400008040d00000c > | --0.61%-- [...] > | This also shows that walk_stackframe() doesn't walk through a process stack. Now I'm trying the following hack on top of Jungseok's patch. (It doesn't traverse from an irq stack to an process stack yet. I need modify unwind_frame().) Thanks, -Takahiro AKASHI ----8<---- diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 650cc05..5fbd1ea 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -185,14 +185,12 @@ alternative_endif mov x23, sp and x23, x23, #~(THREAD_SIZE - 1) cmp x20, x23 // check irq re-enterance + mov x19, sp beq 1f - str x29, [x19, #IRQ_FRAME_FP] - str x21, [x19, #IRQ_FRAME_SP] - str x22, [x19, #IRQ_FRAME_PC] - mov x29, x24 -1: mov x19, sp - csel x23, x19, x24, eq // x24 = top of irq stack - mov sp, x23 + mov sp, x24 // x24 = top of irq stack + stp x29, x22, [sp, #-32]! + mov x29, sp +1: .endm /*
next prev parent reply other threads:[~2015-10-14 7:13 UTC|newest] Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-10-07 15:28 [PATCH v4 0/2] arm64: Introduce IRQ stack Jungseok Lee 2015-10-07 15:28 ` Jungseok Lee 2015-10-07 15:28 ` [PATCH v4 1/2] " Jungseok Lee 2015-10-07 15:28 ` Jungseok Lee 2015-10-08 10:25 ` Pratyush Anand 2015-10-08 10:25 ` Pratyush Anand 2015-10-08 14:32 ` Jungseok Lee 2015-10-08 14:32 ` Jungseok Lee 2015-10-08 16:51 ` Pratyush Anand 2015-10-08 16:51 ` Pratyush Anand 2015-10-07 15:28 ` [PATCH v4 2/2] arm64: Expand the stack trace feature to support " Jungseok Lee 2015-10-07 15:28 ` Jungseok Lee 2015-10-09 14:24 ` James Morse 2015-10-09 14:24 ` James Morse 2015-10-12 14:53 ` Jungseok Lee 2015-10-12 14:53 ` Jungseok Lee 2015-10-12 16:34 ` James Morse 2015-10-12 16:34 ` James Morse 2015-10-12 22:13 ` Jungseok Lee 2015-10-12 22:13 ` Jungseok Lee 2015-10-13 11:00 ` James Morse 2015-10-13 11:00 ` James Morse 2015-10-13 15:00 ` Jungseok Lee 2015-10-13 15:00 ` Jungseok Lee 2015-10-14 12:12 ` Jungseok Lee 2015-10-14 12:12 ` Jungseok Lee 2015-10-15 15:59 ` James Morse 2015-10-15 15:59 ` James Morse 2015-10-16 13:01 ` Jungseok Lee 2015-10-16 13:01 ` Jungseok Lee 2015-10-16 16:06 ` Catalin Marinas 2015-10-16 16:06 ` Catalin Marinas 2015-10-17 13:38 ` Jungseok Lee 2015-10-17 13:38 ` Jungseok Lee 2015-10-19 16:18 ` Catalin Marinas 2015-10-19 16:18 ` Catalin Marinas 2015-10-20 13:08 ` Jungseok Lee 2015-10-20 13:08 ` Jungseok Lee 2015-10-21 15:14 ` Jungseok Lee 2015-10-21 15:14 ` Jungseok Lee 2015-10-14 7:13 ` AKASHI Takahiro [this message] 2015-10-14 7:13 ` AKASHI Takahiro 2015-10-14 12:24 ` Jungseok Lee 2015-10-14 12:24 ` Jungseok Lee 2015-10-14 12:55 ` Jungseok Lee 2015-10-14 12:55 ` Jungseok Lee 2015-10-15 4:19 ` AKASHI Takahiro 2015-10-15 4:19 ` AKASHI Takahiro 2015-10-15 13:39 ` Jungseok Lee 2015-10-15 13:39 ` Jungseok Lee 2015-10-19 6:47 ` AKASHI Takahiro 2015-10-19 6:47 ` AKASHI Takahiro 2015-10-20 13:19 ` Jungseok Lee 2015-10-20 13:19 ` Jungseok Lee 2015-10-15 14:24 ` Jungseok Lee 2015-10-15 14:24 ` Jungseok Lee 2015-10-15 16:01 ` James Morse 2015-10-15 16:01 ` James Morse 2015-10-16 13:02 ` Jungseok Lee 2015-10-16 13:02 ` Jungseok Lee
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=561E009B.3070001@linaro.org \ --to=takahiro.akashi@linaro.org \ --cc=barami97@gmail.com \ --cc=catalin.marinas@arm.com \ --cc=james.morse@arm.com \ --cc=jungseoklee85@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.