Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1241592 > unrolled thread

[PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack

Started byJungseok Lee <jungseoklee85@gmail.com>
First post2015-10-07 17:30 +0200
Last post2015-10-16 15:10 +0200
Articles 5 on this page of 25 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-07 17:30 +0200
    Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack James Morse <james.morse@arm.com> - 2015-10-09 16:30 +0200
      Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-12 17:00 +0200
        Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack James Morse <james.morse@arm.com> - 2015-10-12 18:40 +0200
          Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-13 00:20 +0200
            Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack James Morse <james.morse@arm.com> - 2015-10-13 13:10 +0200
              Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-13 17:10 +0200
                Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-14 14:20 +0200
                  Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack James Morse <james.morse@arm.com> - 2015-10-15 18:10 +0200
                    Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-16 15:10 +0200
                      Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack Catalin Marinas <catalin.marinas@arm.com> - 2015-10-16 18:10 +0200
                        Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-17 15:40 +0200
                          Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack Catalin Marinas <catalin.marinas@arm.com> - 2015-10-19 18:20 +0200
                            Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-20 15:10 +0200
                              Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-21 17:20 +0200
      Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-10-14 09:20 +0200
        Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-14 14:30 +0200
          Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-14 15:00 +0200
            Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-10-15 06:30 +0200
              Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-15 15:40 +0200
                Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack AKASHI Takahiro <takahiro.akashi@linaro.org> - 2015-10-19 08:50 +0200
                  Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-20 15:20 +0200
      Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-15 16:30 +0200
        Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support  IRQ stack James Morse <james.morse@arm.com> - 2015-10-15 18:10 +0200
          Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack Jungseok Lee <jungseoklee85@gmail.com> - 2015-10-16 15:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1250382 — Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack

FromAKASHI Takahiro <takahiro.akashi@linaro.org>
Date2015-10-19 08:50 +0200
SubjectRe: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack
Message-ID<qlcqB-4U1-5@gated-at.bofh.it>
In reply to#1247803
Jungseok,

On 10/15/2015 10:39 PM, Jungseok Lee wrote:
> On Oct 15, 2015, at 1:19 PM, AKASHI Takahiro wrote:
>> Jungseok,
>
>>>> ----8<----
>>>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>>>> index f93aae5..e18be43 100644
>>>> --- a/arch/arm64/kernel/traps.c
>>>> +++ b/arch/arm64/kernel/traps.c
>>>> @@ -103,12 +103,15 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
>>>>         set_fs(fs);
>>>> }
>>>>
>>>> -static void dump_backtrace_entry(unsigned long where, unsigned long stack)
>>>> +static void dump_backtrace_entry(unsigned long where)
>>>> {
>>>> +       /*
>>>> +        * PC has a physical address when MMU is disabled.
>>>> +        */
>>>> +       if (!kernel_text_address(where))
>>>> +               where = (unsigned long)phys_to_virt(where);
>>>> +
>>>>         print_ip_sym(where);
>>>> -       if (in_exception_text(where))
>>>> -               dump_mem("", "Exception stack", stack,
>>>> -                        stack + sizeof(struct pt_regs), false);
>>>> }
>>>>
>>>> static void dump_instr(const char *lvl, struct pt_regs *regs)
>>>> @@ -172,12 +175,17 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>>>>         pr_emerg("Call trace:\n");
>>>>         while (1) {
>>>>                 unsigned long where = frame.pc;
>>>> +               unsigned long stack;
>>>>                 int ret;
>>>>
>>>> +               dump_backtrace_entry(where);
>>>>                 ret = unwind_frame(&frame);
>>>>                 if (ret < 0)
>>>>                         break;
>>>> -               dump_backtrace_entry(where, frame.sp);
>>>> +               stack = frame.sp;
>>>> +               if (in_exception_text(where))
>>>> +                       dump_mem("", "Exception stack", stack,
>>>> +                                stack + sizeof(struct pt_regs), false);
>>>>         }
>>>> }
>>>> ----8<----
>>>>
>>>>> 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
>>>>>
>>>>> 	/*
>>>>
>>>> Is it possible to decide which stack is used without aborted SP information?
>>>
>>> We could know which stack is used via current SP, but how could we decide
>>> a variable 'low' in unwind_frame() when walking a process stack?
>>
>> The following patch, replacing your [PATCH 2/2], seems to work nicely,
>> traversing from interrupt stack to process stack. I tried James' method as well
>> as "echo c > /proc/sysrq-trigger."
>
> Great thanks!
>
> Since I'm favor of your approach, I've played with this patch instead of my one.
> A kernel panic is observed when using 'perf record with -g option' and sysrq.
> I guess some other changes are on your tree..
>
> Please refer to my analysis.
>
>> The only issue that I have now is that dump_backtrace() does not show
>> correct "pt_regs" data on process stack (actually it dumps interrupt stack):
>>
>> CPU1: stopping
>> CPU: 1 PID: 0 Comm: swapper/1 Tainted: G      D         4.3.0-rc5+ #24
>> Hardware name: ARM Arm Versatile Express/Arm Versatile Express, BIOS 11:37:19 Jul 16 2015
>> Call trace:
>> [<ffffffc00008a7b0>] dump_backtrace+0x0/0x19c
>> [<ffffffc00008a968>] show_stack+0x1c/0x28
>> [<ffffffc0003936d0>] dump_stack+0x88/0xc8
>> [<ffffffc00008fdf8>] handle_IPI+0x258/0x268
>> [<ffffffc000082530>] gic_handle_irq+0x88/0xa4
>> Exception stack(0xffffffc87b1bffa0 to 0xffffffc87b1c00c0) <== HERE
>> ffa0: ffffffc87b18fe30 ffffffc87b1bc000 ffffffc87b18ff50 ffffffc000086ac8
>> ffc0: ffffffc87b18c000 afafafafafafafaf ffffffc87b18ff50 ffffffc000086ac8
>> ffe0: ffffffc87b18ff50 ffffffc87b18ff50 afafafafafafafaf afafafafafafafaf
>> 0000: 0000000000000000 ffffffffffffffff ffffffc87b195c00 0000000200000002
>> 0020: 0000000057ac6e9d afafafafafafafaf afafafafafafafaf afafafafafafafaf
>> 0040: afafafafafafafaf afafafafafafafaf afafafafafafafaf afafafafafafafaf
>> 0060: afafafafafafafaf afafafafafafafaf afafafafafafafaf afafafafafafafaf
>> 0080: afafafafafafafaf afafafafafafafaf afafafafafafafaf afafafafafafafaf
>> 00a0: afafafafafafafaf afafafafafafafaf afafafafafafafaf afafafafafafafaf
>> [<ffffffc0000855e0>] el1_irq+0xa0/0x114
>> [<ffffffc000086ac4>] arch_cpu_idle+0x14/0x20
>> [<ffffffc0000fc110>] default_idle_call+0x1c/0x34
>> [<ffffffc0000fc464>] cpu_startup_entry+0x2cc/0x30c
>> [<ffffffc00008f7c4>] secondary_start_kernel+0x120/0x148
>> [<ffffffc0000827a8>] secondary_startup+0x8/0x20
>
> My 'dump_backtrace() rework' patch is in your working tree. Right?

Yeah. I applied your irq stack v5 and "Synchronise dump_backtrace()..." v3,
and tried to reproduce your problem, but didn't.

>>
>> Thanks,
>> -Takahiro AKASHI
>>
>> ----8<----
>>  From 1aa8d4e533d44099f69ff761acfa3c1045a00796 Mon Sep 17 00:00:00 2001
>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> Date: Thu, 15 Oct 2015 09:04:10 +0900
>> Subject: [PATCH] arm64: revamp unwind_frame for interrupt stack
>>
>> This patch allows unwind_frame() to traverse from interrupt stack
>> to process stack correctly by having a dummy stack frame for irq_handler
>> created at its prologue.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> arch/arm64/kernel/entry.S      |   22 ++++++++++++++++++++--
>> arch/arm64/kernel/stacktrace.c |   14 +++++++++++++-
>> 2 files changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>> index 6d4e8c5..25cabd9 100644
>> --- a/arch/arm64/kernel/entry.S
>> +++ b/arch/arm64/kernel/entry.S
>> @@ -185,8 +185,26 @@ alternative_endif
>> 	and	x23, x23, #~(THREAD_SIZE - 1)
>> 	cmp	x20, x23			// check irq re-enterance
>> 	mov	x19, sp
>> -	csel	x23, x19, x24, eq		// x24 = top of irq stack
>> -	mov	sp, x23
>> +	beq	1f
>> +	mov	sp, x24				// x24 = top of irq stack
>> +	stp	x29, x21, [sp, #-16]!		// for sanity check
>> +	stp	x29, x22, [sp, #-16]!		// dummy stack frame
>> +	mov	x29, sp
>> +1:
>> +	/*
>> +	 * Layout of interrupt stack after this macro is invoked:
>> +	 *
>> +	 *     |                |
>> +	 *-0x20+----------------+ <= dummy stack frame
>> +	 *     |      fp        |    : fp on process stack
>> +	 *-0x18+----------------+
>> +	 *     |      lr        |    : return address
>> +	 *-0x10+----------------+
>> +	 *     |    fp (copy)   |    : for sanity check
>> +	 * -0x8+----------------+
>> +	 *     |      sp        |    : sp on process stack
>> +	 *  0x0+----------------+
>> +	 */
>> 	.endm
>>
>> 	/*
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 407991b..03611a1 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -43,12 +43,24 @@ 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 - 0x20 || fp & 0xf)
>> 		return -EINVAL;
>
> IMO, this condition should be changes as follows.
>
> 	if (fp < low || fp > high - 0x20 || fp & 0xf || !fp)

If fp is NULL, (fp < low) should also be true.

-Takahiro AKASHI


> Please refer to the below for details.
>
>>
>> 	frame->sp = fp + 0x10;
>> 	frame->fp = *(unsigned long *)(fp);
>> 	/*
>> +	 * check whether we are going to walk trough from interrupt stack
>> +	 * to process stack
>> +	 * If the previous frame is the initial (dummy) stack frame on
>> +	 * interrupt stack, frame->sp now points to just below the frame
>> +	 * (dummy frame + 0x10).
>> +	 * See entry.S
>> +	 */
>> +#define STACK_LOW(addr) round_down((addr), THREAD_SIZE)
>> +	if ((STACK_LOW(frame->sp) != STACK_LOW(frame->fp)) &&
>> +			(frame->fp == *(unsigned long *)frame->sp))
>> +		frame->sp = *((unsigned long *)(frame->sp + 8));
>
> An original intention seems to catch a stack change from IRQ stack to process one.
> Unfortunately, this condition hits when the last of stack frame of swapper is
> retrieved. This leads to NULL pointer access due to the following code snippet.
>
> ENTRY(__secondary_switched)
>          ldr     x0, [x21]                       // get secondary_data.stack
>          mov     sp, x0
>          mov     x29, #0
>          b       secondary_start_kernel
> ENDPROC(__secondary_switched)
>
> This is why x29 should be checked.
>
> Best Regards
> Jungseok Lee
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1251669

FromJungseok Lee <jungseoklee85@gmail.com>
Date2015-10-20 15:20 +0200
Message-ID<qlEZA-4Nu-21@gated-at.bofh.it>
In reply to#1250382
On Oct 19, 2015, at 3:47 PM, AKASHI Takahiro wrote:
> Jungseok,
> 
> On 10/15/2015 10:39 PM, Jungseok Lee wrote:
>> On Oct 15, 2015, at 1:19 PM, AKASHI Takahiro wrote:
>>> Jungseok,
>> 
>>>>> ----8<----
>>>>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>>>>> index f93aae5..e18be43 100644
>>>>> --- a/arch/arm64/kernel/traps.c
>>>>> +++ b/arch/arm64/kernel/traps.c
>>>>> @@ -103,12 +103,15 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
>>>>>        set_fs(fs);
>>>>> }
>>>>> 
>>>>> -static void dump_backtrace_entry(unsigned long where, unsigned long stack)
>>>>> +static void dump_backtrace_entry(unsigned long where)
>>>>> {
>>>>> +       /*
>>>>> +        * PC has a physical address when MMU is disabled.
>>>>> +        */
>>>>> +       if (!kernel_text_address(where))
>>>>> +               where = (unsigned long)phys_to_virt(where);
>>>>> +
>>>>>        print_ip_sym(where);
>>>>> -       if (in_exception_text(where))
>>>>> -               dump_mem("", "Exception stack", stack,
>>>>> -                        stack + sizeof(struct pt_regs), false);
>>>>> }
>>>>> 
>>>>> static void dump_instr(const char *lvl, struct pt_regs *regs)
>>>>> @@ -172,12 +175,17 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>>>>>        pr_emerg("Call trace:\n");
>>>>>        while (1) {
>>>>>                unsigned long where = frame.pc;
>>>>> +               unsigned long stack;
>>>>>                int ret;
>>>>> 
>>>>> +               dump_backtrace_entry(where);
>>>>>                ret = unwind_frame(&frame);
>>>>>                if (ret < 0)
>>>>>                        break;
>>>>> -               dump_backtrace_entry(where, frame.sp);
>>>>> +               stack = frame.sp;
>>>>> +               if (in_exception_text(where))
>>>>> +                       dump_mem("", "Exception stack", stack,
>>>>> +                                stack + sizeof(struct pt_regs), false);
>>>>>        }
>>>>> }
>>>>> ----8<----
>>>>> 
>>>>>> 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
>>>>>> 
>>>>>> 	/*
>>>>> 
>>>>> Is it possible to decide which stack is used without aborted SP information?
>>>> 
>>>> We could know which stack is used via current SP, but how could we decide
>>>> a variable 'low' in unwind_frame() when walking a process stack?
>>> 
>>> The following patch, replacing your [PATCH 2/2], seems to work nicely,
>>> traversing from interrupt stack to process stack. I tried James' method as well
>>> as "echo c > /proc/sysrq-trigger."
>> 
>> Great thanks!
>> 
>> Since I'm favor of your approach, I've played with this patch instead of my one.
>> A kernel panic is observed when using 'perf record with -g option' and sysrq.
>> I guess some other changes are on your tree..
>> 
>> Please refer to my analysis.
>> 
>>> The only issue that I have now is that dump_backtrace() does not show
>>> correct "pt_regs" data on process stack (actually it dumps interrupt stack):
>>> 
>>> CPU1: stopping
>>> CPU: 1 PID: 0 Comm: swapper/1 Tainted: G      D         4.3.0-rc5+ #24
>>> Hardware name: ARM Arm Versatile Express/Arm Versatile Express, BIOS 11:37:19 Jul 16 2015
>>> Call trace:
>>> [<ffffffc00008a7b0>] dump_backtrace+0x0/0x19c
>>> [<ffffffc00008a968>] show_stack+0x1c/0x28
>>> [<ffffffc0003936d0>] dump_stack+0x88/0xc8
>>> [<ffffffc00008fdf8>] handle_IPI+0x258/0x268
>>> [<ffffffc000082530>] gic_handle_irq+0x88/0xa4
>>> Exception stack(0xffffffc87b1bffa0 to 0xffffffc87b1c00c0) <== HERE
>>> ffa0: ffffffc87b18fe30 ffffffc87b1bc000 ffffffc87b18ff50 ffffffc000086ac8
>>> ffc0: ffffffc87b18c000 afafafafafafafaf ffffffc87b18ff50 ffffffc000086ac8
>>> ffe0: ffffffc87b18ff50 ffffffc87b18ff50 afafafafafafafaf afafafafafafafaf
>>> 0000: 0000000000000000 ffffffffffffffff ffffffc87b195c00 0000000200000002
>>> 0020: 0000000057ac6e9d afafafafafafafaf afafafafafafafaf afafafafafafafaf
>>> 0040: afafafafafafafaf afafafafafafafaf afafafafafafafaf afafafafafafafaf
>>> 0060: afafafafafafafaf afafafafafafafaf afafafafafafafaf afafafafafafafaf
>>> 0080: afafafafafafafaf afafafafafafafaf afafafafafafafaf afafafafafafafaf
>>> 00a0: afafafafafafafaf afafafafafafafaf afafafafafafafaf afafafafafafafaf
>>> [<ffffffc0000855e0>] el1_irq+0xa0/0x114
>>> [<ffffffc000086ac4>] arch_cpu_idle+0x14/0x20
>>> [<ffffffc0000fc110>] default_idle_call+0x1c/0x34
>>> [<ffffffc0000fc464>] cpu_startup_entry+0x2cc/0x30c
>>> [<ffffffc00008f7c4>] secondary_start_kernel+0x120/0x148
>>> [<ffffffc0000827a8>] secondary_startup+0x8/0x20
>> 
>> My 'dump_backtrace() rework' patch is in your working tree. Right?
> 
> Yeah. I applied your irq stack v5 and "Synchronise dump_backtrace()..." v3,
> and tried to reproduce your problem, but didn't.

I've have not seen this problem yet with my patches and your v2.

>>> 
>>> Thanks,
>>> -Takahiro AKASHI
>>> 
>>> ----8<----
>>> From 1aa8d4e533d44099f69ff761acfa3c1045a00796 Mon Sep 17 00:00:00 2001
>>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> Date: Thu, 15 Oct 2015 09:04:10 +0900
>>> Subject: [PATCH] arm64: revamp unwind_frame for interrupt stack
>>> 
>>> This patch allows unwind_frame() to traverse from interrupt stack
>>> to process stack correctly by having a dummy stack frame for irq_handler
>>> created at its prologue.
>>> 
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>> arch/arm64/kernel/entry.S      |   22 ++++++++++++++++++++--
>>> arch/arm64/kernel/stacktrace.c |   14 +++++++++++++-
>>> 2 files changed, 33 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
>>> index 6d4e8c5..25cabd9 100644
>>> --- a/arch/arm64/kernel/entry.S
>>> +++ b/arch/arm64/kernel/entry.S
>>> @@ -185,8 +185,26 @@ alternative_endif
>>> 	and	x23, x23, #~(THREAD_SIZE - 1)
>>> 	cmp	x20, x23			// check irq re-enterance
>>> 	mov	x19, sp
>>> -	csel	x23, x19, x24, eq		// x24 = top of irq stack
>>> -	mov	sp, x23
>>> +	beq	1f
>>> +	mov	sp, x24				// x24 = top of irq stack
>>> +	stp	x29, x21, [sp, #-16]!		// for sanity check
>>> +	stp	x29, x22, [sp, #-16]!		// dummy stack frame
>>> +	mov	x29, sp
>>> +1:
>>> +	/*
>>> +	 * Layout of interrupt stack after this macro is invoked:
>>> +	 *
>>> +	 *     |                |
>>> +	 *-0x20+----------------+ <= dummy stack frame
>>> +	 *     |      fp        |    : fp on process stack
>>> +	 *-0x18+----------------+
>>> +	 *     |      lr        |    : return address
>>> +	 *-0x10+----------------+
>>> +	 *     |    fp (copy)   |    : for sanity check
>>> +	 * -0x8+----------------+
>>> +	 *     |      sp        |    : sp on process stack
>>> +	 *  0x0+----------------+
>>> +	 */
>>> 	.endm
>>> 
>>> 	/*
>>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>>> index 407991b..03611a1 100644
>>> --- a/arch/arm64/kernel/stacktrace.c
>>> +++ b/arch/arm64/kernel/stacktrace.c
>>> @@ -43,12 +43,24 @@ 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 - 0x20 || fp & 0xf)
>>> 		return -EINVAL;
>> 
>> IMO, this condition should be changes as follows.
>> 
>> 	if (fp < low || fp > high - 0x20 || fp & 0xf || !fp)
> 
> If fp is NULL, (fp < low) should also be true.

I will report with the value of low, high, and fp if detected.

Best Regards
Jungseok Lee--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247841

FromJungseok Lee <jungseoklee85@gmail.com>
Date2015-10-15 16:30 +0200
Message-ID<qjRHz-AM-5@gated-at.bofh.it>
In reply to#1243413
On Oct 9, 2015, at 11:24 PM, James Morse wrote:

Hi James,

[ ... ]

> I think unwind_frame() needs to walk the irq stack too. [2] is an example
> of perf tracing back to userspace, (and there are patches on the list to
> do/fix this), so we need to walk back to the start of the first stack for
> the perf accounting to be correct.

I plan to do re-spin this series without [PATCH 2/2] since 1) Akashi's
approach looks better than mine and 2) you have the perf patches for [2].
This would help us to move forward.

Thoughts?

[ ... ]

> [0] sudo ./perf record -e mem:<address of __do_softirq()>:x -ag -- sleep 10
> [1] sudo ./perf report --call-graph --stdio
> [2] http://www.brendangregg.com/FlameGraphs/cpuflamegraphs.html

Best Regards
Jungseok Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247936 — Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack

FromJames Morse <james.morse@arm.com>
Date2015-10-15 18:10 +0200
SubjectRe: [PATCH v4 2/2] arm64: Expand the stack trace feature to support IRQ stack
Message-ID<qjTgl-2Z7-3@gated-at.bofh.it>
In reply to#1247841
On 15/10/15 15:24, Jungseok Lee wrote:
> On Oct 9, 2015, at 11:24 PM, James Morse wrote:
>> I think unwind_frame() needs to walk the irq stack too. [2] is an example
>> of perf tracing back to userspace, (and there are patches on the list to
>> do/fix this), so we need to walk back to the start of the first stack for
>> the perf accounting to be correct.
> 
> I plan to do re-spin this series without [PATCH 2/2] since 1) Akashi's
> approach looks better than mine and 2) you have the perf patches for [2].

They aren't my patches - the ones I saw on the list were for arm:
https://lkml.org/lkml/2015/10/1/769 - its evidently something perf
supports, so we shouldn't make it worse...


Thanks!

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

[toc] | [prev] | [next] | [standalone]

#1248733

FromJungseok Lee <jungseoklee85@gmail.com>
Date2015-10-16 15:10 +0200
Message-ID<qkcVJ-7cO-31@gated-at.bofh.it>
In reply to#1247936
On Oct 16, 2015, at 1:01 AM, James Morse wrote:
> On 15/10/15 15:24, Jungseok Lee wrote:
>> On Oct 9, 2015, at 11:24 PM, James Morse wrote:
>>> I think unwind_frame() needs to walk the irq stack too. [2] is an example
>>> of perf tracing back to userspace, (and there are patches on the list to
>>> do/fix this), so we need to walk back to the start of the first stack for
>>> the perf accounting to be correct.
>> 
>> I plan to do re-spin this series without [PATCH 2/2] since 1) Akashi's
>> approach looks better than mine and 2) you have the perf patches for [2].
> 
> They aren't my patches - the ones I saw on the list were for arm:
> https://lkml.org/lkml/2015/10/1/769 - its evidently something perf
> supports, so we shouldn't make it worse…

Aha, thanks for the information!

Best Regards
Jungseok Lee--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web