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

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 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 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

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

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.

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

Cc: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: James Morse <james.morse@arm.com>
Signed-off-by: Jungseok Lee <jungseoklee85@gmail.com>
---
 arch/arm64/include/asm/irq.h    | 12 +++++++++++
 arch/arm64/kernel/asm-offsets.c |  3 +++
 arch/arm64/kernel/entry.S       | 10 ++++++++--
 arch/arm64/kernel/stacktrace.c  | 22 ++++++++++++++++++++-
 arch/arm64/kernel/traps.c       | 13 ++++++++++++
 5 files changed, 57 insertions(+), 3 deletions(-)

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);
+
+static inline bool in_irq_stack(unsigned int cpu)
+{
+	unsigned long high = (unsigned long)per_cpu(irq_stacks, cpu).stack;
+
+	return (current_stack_pointer >= round_down(high, THREAD_SIZE)) &&
+		current_stack_pointer < high;
+}
+
 struct pt_regs;
 
 extern void migrate_irqs(void);
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index b16e3cf..fbb52f2d 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -42,6 +42,9 @@ int main(void)
   DEFINE(THREAD_CPU_CONTEXT,	offsetof(struct task_struct, thread.cpu_context));
   BLANK();
   DEFINE(IRQ_STACK,		offsetof(struct irq_stack, stack));
+  DEFINE(IRQ_FRAME_FP,		offsetof(struct irq_stack, frame.fp));
+  DEFINE(IRQ_FRAME_SP,		offsetof(struct irq_stack, frame.sp));
+  DEFINE(IRQ_FRAME_PC,		offsetof(struct irq_stack, frame.pc));
   BLANK();
   DEFINE(S_X0,			offsetof(struct pt_regs, regs[0]));
   DEFINE(S_X1,			offsetof(struct pt_regs, regs[1]));
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 6d4e8c5..650cc05 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -121,7 +121,8 @@
 	 * x21 - aborted SP
 	 * x22 - aborted PC
 	 * x23 - aborted PSTATE
-	*/
+	 * x29 - aborted FP
+	 */
 	.endm
 
 	.macro	kernel_exit, el
@@ -184,7 +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
 	.endm
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
+	 * 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.
+	 */
+	if (fp < low || fp > high - 0x10 || fp & 0xf)
 		return -EINVAL;
 
 	frame->sp = fp + 0x10;
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index f93aae5..44b2f828 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -146,6 +146,8 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
 static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 {
 	struct stackframe frame;
+	unsigned int cpu = smp_processor_id();
+	bool in_irq = in_irq_stack(cpu);
 
 	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
 
@@ -170,6 +172,10 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 	}
 
 	pr_emerg("Call trace:\n");
+repeat:
+	if (in_irq)
+		pr_emerg("<IRQ>\n");
+
 	while (1) {
 		unsigned long where = frame.pc;
 		int ret;
@@ -179,6 +185,13 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 			break;
 		dump_backtrace_entry(where, frame.sp);
 	}
+
+	if (in_irq) {
+		frame = per_cpu(irq_stacks, cpu).frame;
+		in_irq = false;
+		pr_emerg("<EOI>\n");
+		goto repeat;
+	}
 }
 
 void show_stack(struct task_struct *tsk, unsigned long *sp)
-- 
2.5.0

--
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] | [next] | [standalone]

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

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.


> 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%-- [...]
                    |

Changing the condition to 'fp >= high - 0x10' fixes this.

I agree it needs documenting, it is quite fiddly - I think Akashi Takahiro
is the expert.


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.


> +	 */
> +	if (fp < low || fp > high - 0x10 || fp & 0xf)
>  		return -EINVAL;
>  
>  	frame->sp = fp + 0x10;
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index f93aae5..44b2f828 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -146,6 +146,8 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>  static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>  {
>  	struct stackframe frame;
> +	unsigned int cpu = smp_processor_id();

I wonder if there is any case where dump_backtrace() is called on another cpu?

Setting the cpu value from task_thread_info(tsk)->cpu would protect against
this.


> +	bool in_irq = in_irq_stack(cpu);
>  
>  	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>  
> @@ -170,6 +172,10 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>  	}
>  
>  	pr_emerg("Call trace:\n");
> +repeat:
> +	if (in_irq)
> +		pr_emerg("<IRQ>\n");

Do we need these? 'el1_irq()' in the trace is a giveaway...


> +
>  	while (1) {
>  		unsigned long where = frame.pc;
>  		int ret;
> @@ -179,6 +185,13 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>  			break;
>  		dump_backtrace_entry(where, frame.sp);
>  	}
> +
> +	if (in_irq) {
> +		frame = per_cpu(irq_stacks, cpu).frame;
> +		in_irq = false;
> +		pr_emerg("<EOI>\n");
> +		goto repeat;
> +	}
>  }
>  
>  void show_stack(struct task_struct *tsk, unsigned long *sp)


Thanks!

James


[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
--
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]

#1244788

On Oct 9, 2015, at 11:24 PM, James Morse wrote:
> Hi Jungseok,

Hi James,

> 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

I will fix the typo.

>> 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.

It was an original intention to lead them to this patch, not the [1/2] one.
This separation would help anyone touching the call trace feature including
me focus on these changes apart from stack allocation, IRQ recursion check
and thread_info management.

In addition, I would like to add a clear and sufficient explanation on the
frame pointer condition.

>> 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.
> 
> 
>> 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.

Good idea. I'm favor of those terms.

>> +	 * 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.

What I meant is 0x20 is the highest valid frame pointer. The comment should
have been described more clearly.

> 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 ]

Clear description than mine!

> 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.

I believe my understanding is aligned with yours.

Under a current condition, 'fp > high - 0x18', it is impossible to catch the
'el1_irq' symbol. This is why I set x29 to high - 0x10 and changed the frame
pointer condition, but the changes fail to cover perf according to your data.

> This change is breaking perf using incantations [0] and [1]:

I'm reviewing how perf stack trace works..

> 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%-- [...]
>                    |
> 
> Changing the condition to 'fp >= high - 0x10' fixes this.

'fp >= high - 0x10' drops 'el1_irq' when dump_stack() or panic() is called.

> I agree it needs documenting, it is quite fiddly - I think Akashi Takahiro
> is the expert.

If possible, it would be greatly helpful.

> 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.

Frankly, I missed the case where perf does backtrace to userspace.

IMO, this statement supports why the stack trace feature commit should be
written independently. The [1/2] patch would be pretty stable if 64KB page
is supported. The separation might help us concentrate on the stack trace
feature in a generic dump stack, perf, and ftrace point of view.

>> +	 */
>> +	if (fp < low || fp > high - 0x10 || fp & 0xf)
>> 		return -EINVAL;
>> 
>> 	frame->sp = fp + 0x10;
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index f93aae5..44b2f828 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -146,6 +146,8 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>> static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>> {
>> 	struct stackframe frame;
>> +	unsigned int cpu = smp_processor_id();
> 
> I wonder if there is any case where dump_backtrace() is called on another cpu?
> 
> Setting the cpu value from task_thread_info(tsk)->cpu would protect against
> this.

IMO, no, but your suggestion makes sense. I will update it.

>> +	bool in_irq = in_irq_stack(cpu);
>> 
>> 	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>> 
>> @@ -170,6 +172,10 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>> 	}
>> 
>> 	pr_emerg("Call trace:\n");
>> +repeat:
>> +	if (in_irq)
>> +		pr_emerg("<IRQ>\n");
> 
> Do we need these? 'el1_irq()' in the trace is a giveaway…

I borrow this idea from x86 implementation in order to show a separate stack
explicitly. There is no issue to remove these tags, <IRQ> and <EOI>.

Great thanks!

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]

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

Hi Jungseok,

On 12/10/15 15:53, 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.
> 
> Frankly, I missed the case where perf does backtrace to userspace.
> 
> IMO, this statement supports why the stack trace feature commit should be
> written independently. The [1/2] patch would be pretty stable if 64KB page
> is supported.

If this hasn't been started yet, here is a build-test-only first-pass at
the 64K page support - based on the code in kernel/fork.c:

==================%<==================
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index a6bdf4d3a57c..deb057a735ad 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -27,8 +27,22 @@
 #include <linux/init.h>
 #include <linux/irqchip.h>
 #include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/topology.h>
 #include <linux/ratelimit.h>

+#if THREAD_SIZE >= PAGE_SIZE
+#define __alloc_irq_stack(x) (void *)__get_free_pages(THREADINFO_GFP,  \
+                                                     THREAD_SIZE_ORDER)
+
+extern struct kmem_cache *irq_stack_cache;     /* dummy declaration */
+#else
+#define __alloc_irq_stack(cpu) (void
*)kmem_cache_alloc_node(irq_stack_cache, \
+                                       THREADINFO_GFP, cpu_to_node(cpu))
+
+static struct kmem_cache *irq_stack_cache;
+#endif /* THREAD_SIZE >= PAGE_SIZE */

 unsigned long irq_err_count;

 DEFINE_PER_CPU(struct irq_stack, irq_stacks);
@@ -128,7 +142,17 @@ int alloc_irq_stack(unsigned int cpu)
        if (per_cpu(irq_stacks, cpu).stack)
                return 0;

-       stack = (void *)__get_free_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
+       if (THREAD_SIZE < PAGE_SIZE) {
+               if (!irq_stack_cache) {
+                       irq_stack_cache = kmem_cache_create("irq_stack",
+                                                           THREAD_SIZE,
+                                                           THREAD_SIZE, 0,
+                                                           NULL);
+                       BUG_ON(!irq_stack_cache);
+               }
+       }
+
+       stack = __alloc_irq_stack(cpu);
        if (!stack)
                return -ENOMEM;

==================%<==================
(my mail client will almost certainly mangle that)

Having two kmem_caches for 16K stacks on a 64K page system may be wasteful
(especially for systems with few cpus)...

The alternative is to defining CONFIG_ARCH_THREAD_INFO_ALLOCATOR and
allocate all stack memory from arch code. (Largely copied code, prevents
irq stacks being a different size, and nothing uses that define today!)


Thoughts?


> 
>>> +	 */
>>> +	if (fp < low || fp > high - 0x10 || fp & 0xf)
>>> 		return -EINVAL;
>>>
>>> 	frame->sp = fp + 0x10;
>>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>>> index f93aae5..44b2f828 100644
>>> --- a/arch/arm64/kernel/traps.c
>>> +++ b/arch/arm64/kernel/traps.c
>>> @@ -146,6 +146,8 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>> static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>>> {
>>> 	struct stackframe frame;
>>> +	unsigned int cpu = smp_processor_id();
>>
>> I wonder if there is any case where dump_backtrace() is called on another cpu?
>>
>> Setting the cpu value from task_thread_info(tsk)->cpu would protect against
>> this.
> 
> IMO, no, but your suggestion makes sense. I will update it.
> 
>>> +	bool in_irq = in_irq_stack(cpu);
>>>
>>> 	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
>>>
>>> @@ -170,6 +172,10 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
>>> 	}
>>>
>>> 	pr_emerg("Call trace:\n");
>>> +repeat:
>>> +	if (in_irq)
>>> +		pr_emerg("<IRQ>\n");
>>
>> Do we need these? 'el1_irq()' in the trace is a giveaway…
> 
> I borrow this idea from x86 implementation in order to show a separate stack
> explicitly. There is no issue to remove these tags, <IRQ> and <EOI>.

Ah okay - if its done elsewhere, its better to be consistent.


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]

#1245188

On Oct 13, 2015, at 1:34 AM, James Morse wrote:
> Hi Jungseok,

Hi James,

> On 12/10/15 15:53, 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.
>> 
>> Frankly, I missed the case where perf does backtrace to userspace.
>> 
>> IMO, this statement supports why the stack trace feature commit should be
>> written independently. The [1/2] patch would be pretty stable if 64KB page
>> is supported.
> 
> If this hasn't been started yet, here is a build-test-only first-pass at
> the 64K page support - based on the code in kernel/fork.c:
> 
> ==================%<==================
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index a6bdf4d3a57c..deb057a735ad 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -27,8 +27,22 @@
> #include <linux/init.h>
> #include <linux/irqchip.h>
> #include <linux/seq_file.h>
> +#include <linux/slab.h>
> +#include <linux/topology.h>
> #include <linux/ratelimit.h>
> 
> +#if THREAD_SIZE >= PAGE_SIZE
> +#define __alloc_irq_stack(x) (void *)__get_free_pages(THREADINFO_GFP,  \
> +                                                     THREAD_SIZE_ORDER)
> +
> +extern struct kmem_cache *irq_stack_cache;     /* dummy declaration */
> +#else
> +#define __alloc_irq_stack(cpu) (void
> *)kmem_cache_alloc_node(irq_stack_cache, \
> +                                       THREADINFO_GFP, cpu_to_node(cpu))
> +
> +static struct kmem_cache *irq_stack_cache;
> +#endif /* THREAD_SIZE >= PAGE_SIZE */
> 
> unsigned long irq_err_count;
> 
> DEFINE_PER_CPU(struct irq_stack, irq_stacks);
> @@ -128,7 +142,17 @@ int alloc_irq_stack(unsigned int cpu)
>        if (per_cpu(irq_stacks, cpu).stack)
>                return 0;
> 
> -       stack = (void *)__get_free_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
> +       if (THREAD_SIZE < PAGE_SIZE) {
> +               if (!irq_stack_cache) {
> +                       irq_stack_cache = kmem_cache_create("irq_stack",
> +                                                           THREAD_SIZE,
> +                                                           THREAD_SIZE, 0,
> +                                                           NULL);
> +                       BUG_ON(!irq_stack_cache);
> +               }
> +       }
> +
> +       stack = __alloc_irq_stack(cpu);
>        if (!stack)
>                return -ENOMEM;
> 
> ==================%<==================
> (my mail client will almost certainly mangle that)
> 
> Having two kmem_caches for 16K stacks on a 64K page system may be wasteful
> (especially for systems with few cpus)…

This would be a single concern. To address this issue, I drop the 'static'
keyword in thread_info_cache. Please refer to the below hunk.

> The alternative is to defining CONFIG_ARCH_THREAD_INFO_ALLOCATOR and
> allocate all stack memory from arch code. (Largely copied code, prevents
> irq stacks being a different size, and nothing uses that define today!)
> 
> 
> Thoughts?

Almost same story I've been testing.

I'm aligned with yours Regarding CONFIG_ARCH_THREAD_INFO_ALLOCATOR.

Another approach I've tried is the following data structure, but it's not
a good fit for this case due to __per_cpu_offset which is page-size aligned,
not thread-size.

struct irq_stack {
	char stack[THREAD_SIZE];
	char *highest;
} __aligned(THREAD_SIZE);

DEFINE_PER_CPU(struct irq_stack, irq_stacks);

----8<-----
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index 6ea82e8..d3619b3 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -1,7 +1,9 @@
 #ifndef __ASM_IRQ_H
 #define __ASM_IRQ_H
 
+#include <linux/gfp.h>
 #include <linux/irqchip/arm-gic-acpi.h>
+#include <linux/slab.h>
 
 #include <asm-generic/irq.h>
 
@@ -9,6 +11,21 @@ struct irq_stack {
        void *stack;
 };
 
+#if THREAD_SIZE >= PAGE_SIZE
+static inline void *__alloc_irq_stack(void)
+{
+       return (void *)__get_free_pages(THREADINFO_GFP | __GFP_ZERO,
+                                       THREAD_SIZE_ORDER);
+}
+#else
+extern struct kmem_cache *thread_info_cache;
+
+static inline void *__alloc_irq_stack(void)
+{
+       return kmem_cache_alloc(thread_info_cache, THREADINFO_GFP | __GFP_ZERO);
+}
+#endif
+
 struct pt_regs;
 
 extern void migrate_irqs(void);
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index a6bdf4d..4e13bdd 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -50,10 +50,13 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
        handle_arch_irq = handle_irq;
 }
 
+static char boot_irq_stack[THREAD_SIZE] __aligned(THREAD_SIZE);
+
 void __init init_IRQ(void)
 {
-       if (alloc_irq_stack(smp_processor_id()))
-               panic("Failed to allocate IRQ stack for a boot cpu");
+       unsigned int cpu = smp_processor_id();
+
+       per_cpu(irq_stacks, cpu).stack = boot_irq_stack + THREAD_START_SP;
 
        irqchip_init();
        if (!handle_arch_irq)
@@ -128,7 +131,7 @@ int alloc_irq_stack(unsigned int cpu)
        if (per_cpu(irq_stacks, cpu).stack)
                return 0;
 
-       stack = (void *)__get_free_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
+       stack = __alloc_irq_stack();
        if (!stack)
                return -ENOMEM;
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 2845623..9c55f86 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -172,7 +172,7 @@ static inline void free_thread_info(struct thread_info *ti)
        free_kmem_pages((unsigned long)ti, THREAD_SIZE_ORDER);
 }
 # else
-static struct kmem_cache *thread_info_cache;
+struct kmem_cache *thread_info_cache;
 
 static struct thread_info *alloc_thread_info_node(struct task_struct *tsk,
                                                  int node)
----8<-----

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]

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

Hi Jungseok,

On 12/10/15 23:13, Jungseok Lee wrote:
> On Oct 13, 2015, at 1:34 AM, James Morse wrote:
>> Having two kmem_caches for 16K stacks on a 64K page system may be wasteful
>> (especially for systems with few cpus)…
> 
> This would be a single concern. To address this issue, I drop the 'static'
> keyword in thread_info_cache. Please refer to the below hunk.

Its only a problem on systems with 64K pages, which don't have a multiple
of 4 cpus. I suspect if you turn on 64K pages, you have many cores with
plenty of memory...


>> The alternative is to defining CONFIG_ARCH_THREAD_INFO_ALLOCATOR and
>> allocate all stack memory from arch code. (Largely copied code, prevents
>> irq stacks being a different size, and nothing uses that define today!)
>>
>>
>> Thoughts?
> 
> Almost same story I've been testing.
> 
> I'm aligned with yours Regarding CONFIG_ARCH_THREAD_INFO_ALLOCATOR.
> 
> Another approach I've tried is the following data structure, but it's not
> a good fit for this case due to __per_cpu_offset which is page-size aligned,
> not thread-size.
> 
> struct irq_stack {
> 	char stack[THREAD_SIZE];
> 	char *highest;
> } __aligned(THREAD_SIZE);
> 
> DEFINE_PER_CPU(struct irq_stack, irq_stacks);

Yes, x86 does this - but it increases the Image size by 16K, as that space
could have some initialisation values. This isn't a problem on x86 as
no-one uses the uncompressed image.

I would avoid this approach due to the bloat!

> 
> ----8<-----
> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
> index 6ea82e8..d3619b3 100644
> --- a/arch/arm64/include/asm/irq.h
> +++ b/arch/arm64/include/asm/irq.h
> @@ -1,7 +1,9 @@
>  #ifndef __ASM_IRQ_H
>  #define __ASM_IRQ_H
>  
> +#include <linux/gfp.h>
>  #include <linux/irqchip/arm-gic-acpi.h>
> +#include <linux/slab.h>
>  
>  #include <asm-generic/irq.h>
>  
> @@ -9,6 +11,21 @@ struct irq_stack {
>         void *stack;
>  };
>  
> +#if THREAD_SIZE >= PAGE_SIZE
> +static inline void *__alloc_irq_stack(void)
> +{
> +       return (void *)__get_free_pages(THREADINFO_GFP | __GFP_ZERO,
> +                                       THREAD_SIZE_ORDER);
> +}
> +#else
> +extern struct kmem_cache *thread_info_cache;

If this has been made a published symbol, it should go in a header file.

> +
> +static inline void *__alloc_irq_stack(void)
> +{
> +       return kmem_cache_alloc(thread_info_cache, THREADINFO_GFP | __GFP_ZERO);
> +}
> +#endif
> +
>  struct pt_regs;
>  
>  extern void migrate_irqs(void);
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index a6bdf4d..4e13bdd 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -50,10 +50,13 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>         handle_arch_irq = handle_irq;
>  }
>  
> +static char boot_irq_stack[THREAD_SIZE] __aligned(THREAD_SIZE);
> +
>  void __init init_IRQ(void)
>  {
> -       if (alloc_irq_stack(smp_processor_id()))
> -               panic("Failed to allocate IRQ stack for a boot cpu");
> +       unsigned int cpu = smp_processor_id();
> +
> +       per_cpu(irq_stacks, cpu).stack = boot_irq_stack + THREAD_START_SP;
>  
>         irqchip_init();
>         if (!handle_arch_irq)
> @@ -128,7 +131,7 @@ int alloc_irq_stack(unsigned int cpu)
>         if (per_cpu(irq_stacks, cpu).stack)
>                 return 0;
>  
> -       stack = (void *)__get_free_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
> +       stack = __alloc_irq_stack();
>         if (!stack)
>                 return -ENOMEM;
>  
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 2845623..9c55f86 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -172,7 +172,7 @@ static inline void free_thread_info(struct thread_info *ti)
>         free_kmem_pages((unsigned long)ti, THREAD_SIZE_ORDER);
>  }
>  # else
> -static struct kmem_cache *thread_info_cache;
> +struct kmem_cache *thread_info_cache;
>  
>  static struct thread_info *alloc_thread_info_node(struct task_struct *tsk,
>                                                   int node)
> ----8<-----


Looks good!


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]

#1245827

On Oct 13, 2015, at 8:00 PM, James Morse wrote:
> Hi Jungseok,

Hi James,

> On 12/10/15 23:13, Jungseok Lee wrote:
>> On Oct 13, 2015, at 1:34 AM, James Morse wrote:
>>> Having two kmem_caches for 16K stacks on a 64K page system may be wasteful
>>> (especially for systems with few cpus)…
>> 
>> This would be a single concern. To address this issue, I drop the 'static'
>> keyword in thread_info_cache. Please refer to the below hunk.
> 
> Its only a problem on systems with 64K pages, which don't have a multiple
> of 4 cpus. I suspect if you turn on 64K pages, you have many cores with
> plenty of memory…

Yes, the problem 'two kmem_caches' comes from only 64K page system.

I don't get the statement 'which don't have a multiple of 4 cpus'.
Could you point out what I am missing?

Since I don't have platforms which have many cores and huge memory,
I cannot play with this series on them.

>>> The alternative is to defining CONFIG_ARCH_THREAD_INFO_ALLOCATOR and
>>> allocate all stack memory from arch code. (Largely copied code, prevents
>>> irq stacks being a different size, and nothing uses that define today!)
>>> 
>>> 
>>> Thoughts?
>> 
>> Almost same story I've been testing.
>> 
>> I'm aligned with yours Regarding CONFIG_ARCH_THREAD_INFO_ALLOCATOR.
>> 
>> Another approach I've tried is the following data structure, but it's not
>> a good fit for this case due to __per_cpu_offset which is page-size aligned,
>> not thread-size.
>> 
>> struct irq_stack {
>> 	char stack[THREAD_SIZE];
>> 	char *highest;
>> } __aligned(THREAD_SIZE);
>> 
>> DEFINE_PER_CPU(struct irq_stack, irq_stacks);
> 
> Yes, x86 does this - but it increases the Image size by 16K, as that space
> could have some initialisation values. This isn't a problem on x86 as
> no-one uses the uncompressed image.
> 
> I would avoid this approach due to the bloat!
> 
>> 
>> ----8<-----
>> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
>> index 6ea82e8..d3619b3 100644
>> --- a/arch/arm64/include/asm/irq.h
>> +++ b/arch/arm64/include/asm/irq.h
>> @@ -1,7 +1,9 @@
>> #ifndef __ASM_IRQ_H
>> #define __ASM_IRQ_H
>> 
>> +#include <linux/gfp.h>
>> #include <linux/irqchip/arm-gic-acpi.h>
>> +#include <linux/slab.h>
>> 
>> #include <asm-generic/irq.h>
>> 
>> @@ -9,6 +11,21 @@ struct irq_stack {
>>        void *stack;
>> };
>> 
>> +#if THREAD_SIZE >= PAGE_SIZE
>> +static inline void *__alloc_irq_stack(void)
>> +{
>> +       return (void *)__get_free_pages(THREADINFO_GFP | __GFP_ZERO,
>> +                                       THREAD_SIZE_ORDER);
>> +}
>> +#else
>> +extern struct kmem_cache *thread_info_cache;
> 
> If this has been made a published symbol, it should go in a header file.

Sure.

>> +
>> +static inline void *__alloc_irq_stack(void)
>> +{
>> +       return kmem_cache_alloc(thread_info_cache, THREADINFO_GFP | __GFP_ZERO);
>> +}
>> +#endif
>> +
>> struct pt_regs;
>> 
>> extern void migrate_irqs(void);
>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
>> index a6bdf4d..4e13bdd 100644
>> --- a/arch/arm64/kernel/irq.c
>> +++ b/arch/arm64/kernel/irq.c
>> @@ -50,10 +50,13 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
>>        handle_arch_irq = handle_irq;
>> }
>> 
>> +static char boot_irq_stack[THREAD_SIZE] __aligned(THREAD_SIZE);
>> +
>> void __init init_IRQ(void)
>> {
>> -       if (alloc_irq_stack(smp_processor_id()))
>> -               panic("Failed to allocate IRQ stack for a boot cpu");
>> +       unsigned int cpu = smp_processor_id();
>> +
>> +       per_cpu(irq_stacks, cpu).stack = boot_irq_stack + THREAD_START_SP;
>> 
>>        irqchip_init();
>>        if (!handle_arch_irq)
>> @@ -128,7 +131,7 @@ int alloc_irq_stack(unsigned int cpu)
>>        if (per_cpu(irq_stacks, cpu).stack)
>>                return 0;
>> 
>> -       stack = (void *)__get_free_pages(THREADINFO_GFP, THREAD_SIZE_ORDER);
>> +       stack = __alloc_irq_stack();
>>        if (!stack)
>>                return -ENOMEM;
>> 
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> index 2845623..9c55f86 100644
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -172,7 +172,7 @@ static inline void free_thread_info(struct thread_info *ti)
>>        free_kmem_pages((unsigned long)ti, THREAD_SIZE_ORDER);
>> }
>> # else
>> -static struct kmem_cache *thread_info_cache;
>> +struct kmem_cache *thread_info_cache;
>> 
>> static struct thread_info *alloc_thread_info_node(struct task_struct *tsk,
>>                                                  int node)
>> ----8<-----
> 
> 
> Looks good!

Thanks for reviewing the code!

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]

#1246613

On Oct 14, 2015, at 12:00 AM, Jungseok Lee wrote:
> On Oct 13, 2015, at 8:00 PM, James Morse wrote:
>> Hi Jungseok,
> 
> Hi James,
> 
>> On 12/10/15 23:13, Jungseok Lee wrote:
>>> On Oct 13, 2015, at 1:34 AM, James Morse wrote:
>>>> Having two kmem_caches for 16K stacks on a 64K page system may be wasteful
>>>> (especially for systems with few cpus)…
>>> 
>>> This would be a single concern. To address this issue, I drop the 'static'
>>> keyword in thread_info_cache. Please refer to the below hunk.
>> 
>> Its only a problem on systems with 64K pages, which don't have a multiple
>> of 4 cpus. I suspect if you turn on 64K pages, you have many cores with
>> plenty of memory…
> 
> Yes, the problem 'two kmem_caches' comes from only 64K page system.
> 
> I don't get the statement 'which don't have a multiple of 4 cpus'.
> Could you point out what I am missing?

You're talking about sl{a|u}b allocator behavior. If so, I got what you meant.

> Since I don't have platforms which have many cores and huge memory,
> I cannot play with this series on them.
> 
>>>> The alternative is to defining CONFIG_ARCH_THREAD_INFO_ALLOCATOR and
>>>> allocate all stack memory from arch code. (Largely copied code, prevents
>>>> irq stacks being a different size, and nothing uses that define today!)
>>>> 
>>>> 
>>>> Thoughts?
>>> 
>>> Almost same story I've been testing.
>>> 
>>> I'm aligned with yours Regarding CONFIG_ARCH_THREAD_INFO_ALLOCATOR.
>>> 
>>> Another approach I've tried is the following data structure, but it's not
>>> a good fit for this case due to __per_cpu_offset which is page-size aligned,
>>> not thread-size.
>>> 
>>> struct irq_stack {
>>> 	char stack[THREAD_SIZE];
>>> 	char *highest;
>>> } __aligned(THREAD_SIZE);
>>> 
>>> DEFINE_PER_CPU(struct irq_stack, irq_stacks);
>> 
>> Yes, x86 does this - but it increases the Image size by 16K, as that space
>> could have some initialisation values. This isn't a problem on x86 as
>> no-one uses the uncompressed image.
>> 
>> I would avoid this approach due to the bloat!
>> 
>>> 
>>> ----8<-----
>>> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
>>> index 6ea82e8..d3619b3 100644
>>> --- a/arch/arm64/include/asm/irq.h
>>> +++ b/arch/arm64/include/asm/irq.h
>>> @@ -1,7 +1,9 @@
>>> #ifndef __ASM_IRQ_H
>>> #define __ASM_IRQ_H
>>> 
>>> +#include <linux/gfp.h>
>>> #include <linux/irqchip/arm-gic-acpi.h>
>>> +#include <linux/slab.h>
>>> 
>>> #include <asm-generic/irq.h>
>>> 
>>> @@ -9,6 +11,21 @@ struct irq_stack {
>>>       void *stack;
>>> };
>>> 
>>> +#if THREAD_SIZE >= PAGE_SIZE
>>> +static inline void *__alloc_irq_stack(void)
>>> +{
>>> +       return (void *)__get_free_pages(THREADINFO_GFP | __GFP_ZERO,
>>> +                                       THREAD_SIZE_ORDER);
>>> +}
>>> +#else
>>> +extern struct kmem_cache *thread_info_cache;
>> 
>> If this has been made a published symbol, it should go in a header file.
> 
> Sure.

I had the wrong impression that there is a room under include/linux/*.

IMO, this is architectural option whether arch relies on thread_info_cache or not.
In other words, it would be clear to put this extern under arch/*/include/asm/*.

Thoughts?

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]

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

On 14/10/15 13:12, Jungseok Lee wrote:
> On Oct 14, 2015, at 12:00 AM, Jungseok Lee wrote:
>> On Oct 13, 2015, at 8:00 PM, James Morse wrote:
>>> On 12/10/15 23:13, Jungseok Lee wrote:
>>>> On Oct 13, 2015, at 1:34 AM, James Morse wrote:
>>>>> Having two kmem_caches for 16K stacks on a 64K page system may be wasteful
>>>>> (especially for systems with few cpus)…
>>>>
>>>> This would be a single concern. To address this issue, I drop the 'static'
>>>> keyword in thread_info_cache. Please refer to the below hunk.
>>>
>>> Its only a problem on systems with 64K pages, which don't have a multiple
>>> of 4 cpus. I suspect if you turn on 64K pages, you have many cores with
>>> plenty of memory…
>>
>> Yes, the problem 'two kmem_caches' comes from only 64K page system.
>>
>> I don't get the statement 'which don't have a multiple of 4 cpus'.
>> Could you point out what I am missing?
> 
> You're talking about sl{a|u}b allocator behavior. If so, I got what you meant.

Yes,
With Nx4 cpus, the (currently) 16K irq stacks take up Nx64K - a nice
multiple of pages, so no wasted memory.


>>> If this has been made a published symbol, it should go in a header file.
>>
>> Sure.
> 
> I had the wrong impression that there is a room under include/linux/*.

Yes, I see there isn't anywhere obvious to put it...


> IMO, this is architectural option whether arch relies on thread_info_cache or not.
> In other words, it would be clear to put this extern under arch/*/include/asm/*.

Its up to the arch whether or not to define
CONFIG_ARCH_THREAD_INFO_ALLOCATOR. In the case where it hasn't defined it,
and THREAD_SIZE >= PAGE_SIZE, your change is exposing thread_info_cache on
all architectures, so it ought go in a header file accessible to all
architectures.

Something like this, (only build-tested!):
=========%<=========
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -10,6 +10,8 @@
 #include <linux/types.h>
 #include <linux/bug.h>

+#include <asm/page.h>
+
 struct timespec;
 struct compat_timespec;

@@ -145,6 +147,12 @@ static inline bool test_and_clear_restore_sigmask(void)
 #error "no set_restore_sigmask() provided and default one won't work"
 #endif

+#ifndef CONFIG_ARCH_THREAD_INFO_ALLOCATOR
+#if THREAD_SIZE >= PAGE_SIZE
+extern struct kmem_cache *thread_info_cache;
+#endif /* THREAD_SIZE >= PAGE_SIZE */
+#endif /* CONFIG_ARCH_THREAD_INFO_ALLOCATOR */
+
 #endif /* __KERNEL__ */

 #endif /* _LINUX_THREAD_INFO_H */
=========%<=========
Quite ugly!

My concern is there could be push-back from the maintainer of
kernel/fork.c, saying "define CONFIG_ARCH_THREAD_INFO_ALLOCATOR if the
generic code isn't what you need", and push-back from the arm64 maintainers
about copy-pasting that chunk into arch/arm64.... both of which are fair,
hence my initial version created a second kmem_cache.


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]

#1248726

On Oct 16, 2015, at 12:59 AM, James Morse wrote:

Hi James,

> On 14/10/15 13:12, Jungseok Lee wrote:
>> On Oct 14, 2015, at 12:00 AM, Jungseok Lee wrote:
>>> On Oct 13, 2015, at 8:00 PM, James Morse wrote:
>>>> On 12/10/15 23:13, Jungseok Lee wrote:
>>>>> On Oct 13, 2015, at 1:34 AM, James Morse wrote:
>>>>>> Having two kmem_caches for 16K stacks on a 64K page system may be wasteful
>>>>>> (especially for systems with few cpus)…
>>>>> 
>>>>> This would be a single concern. To address this issue, I drop the 'static'
>>>>> keyword in thread_info_cache. Please refer to the below hunk.
>>>> 
>>>> Its only a problem on systems with 64K pages, which don't have a multiple
>>>> of 4 cpus. I suspect if you turn on 64K pages, you have many cores with
>>>> plenty of memory…
>>> 
>>> Yes, the problem 'two kmem_caches' comes from only 64K page system.
>>> 
>>> I don't get the statement 'which don't have a multiple of 4 cpus'.
>>> Could you point out what I am missing?
>> 
>> You're talking about sl{a|u}b allocator behavior. If so, I got what you meant.
> 
> Yes,
> With Nx4 cpus, the (currently) 16K irq stacks take up Nx64K - a nice
> multiple of pages, so no wasted memory.
> 
> 
>>>> If this has been made a published symbol, it should go in a header file.
>>> 
>>> Sure.
>> 
>> I had the wrong impression that there is a room under include/linux/*.
> 
> Yes, I see there isn't anywhere obvious to put it...
> 
> 
>> IMO, this is architectural option whether arch relies on thread_info_cache or not.
>> In other words, it would be clear to put this extern under arch/*/include/asm/*.
> 
> Its up to the arch whether or not to define
> CONFIG_ARCH_THREAD_INFO_ALLOCATOR. In the case where it hasn't defined it,
> and THREAD_SIZE >= PAGE_SIZE, your change is exposing thread_info_cache on
> all architectures, so it ought go in a header file accessible to all
> architectures.
> 
> Something like this, (only build-tested!):
> =========%<=========
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -10,6 +10,8 @@
> #include <linux/types.h>
> #include <linux/bug.h>
> 
> +#include <asm/page.h>
> +

As reviewing arch codes, it seems not to cover all architecture..

> struct timespec;
> struct compat_timespec;
> 
> @@ -145,6 +147,12 @@ static inline bool test_and_clear_restore_sigmask(void)
> #error "no set_restore_sigmask() provided and default one won't work"
> #endif
> 
> +#ifndef CONFIG_ARCH_THREAD_INFO_ALLOCATOR
> +#if THREAD_SIZE >= PAGE_SIZE
> +extern struct kmem_cache *thread_info_cache;
> +#endif /* THREAD_SIZE >= PAGE_SIZE */
> +#endif /* CONFIG_ARCH_THREAD_INFO_ALLOCATOR */
> +
> #endif /* __KERNEL__ */
> 
> #endif /* _LINUX_THREAD_INFO_H */
> =========%<=========
> Quite ugly!
> 
> My concern is there could be push-back from the maintainer of
> kernel/fork.c, saying "define CONFIG_ARCH_THREAD_INFO_ALLOCATOR if the
> generic code isn't what you need", and push-back from the arm64 maintainers
> about copy-pasting that chunk into arch/arm64.... both of which are fair,
> hence my initial version created a second kmem_cache.

Same concern. I believe now is the time to get feedbacks from maintainers.
It will help us to decide the next step.

I will do re-spin soon!

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]

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

On Fri, Oct 16, 2015 at 10:01:20PM +0900, Jungseok Lee wrote:
> On Oct 16, 2015, at 12:59 AM, James Morse wrote:
> > My concern is there could be push-back from the maintainer of
> > kernel/fork.c, saying "define CONFIG_ARCH_THREAD_INFO_ALLOCATOR if the
> > generic code isn't what you need", and push-back from the arm64 maintainers
> > about copy-pasting that chunk into arch/arm64.... both of which are fair,
> > hence my initial version created a second kmem_cache.
> 
> Same concern. I believe now is the time to get feedbacks from maintainers.
> It will help us to decide the next step.

I'll push back now to avoid further doubts in changing kernel/fork.c ;).

A reason to define a kmem_cache is performance for repeated allocations.
But here you only do it once during boot. So you could simply use
kmalloc() when THREAD_SIZE < PAGE_SIZE. BTW, the IRQ stack size doesn't
even need to be the same as THREAD_SIZE, though we could initially keep
them the same. But it's worth defining an IRQ_STACK_SIZE macro if we
ever need to change it.

BTW, a static allocation (DEFINE_PER_CPU for the whole irq stack) would
save us from another stack address reading on the IRQ entry path. I'm
not sure exactly where the 16K image increase comes from but at least it
doesn't grow with NR_CPUS, so we can probably live with this.

-- 
Catalin
--
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]

#1249327

On Oct 17, 2015, at 1:06 AM, Catalin Marinas wrote:

Hi Catalin,

> On Fri, Oct 16, 2015 at 10:01:20PM +0900, Jungseok Lee wrote:
>> On Oct 16, 2015, at 12:59 AM, James Morse wrote:
>>> My concern is there could be push-back from the maintainer of
>>> kernel/fork.c, saying "define CONFIG_ARCH_THREAD_INFO_ALLOCATOR if the
>>> generic code isn't what you need", and push-back from the arm64 maintainers
>>> about copy-pasting that chunk into arch/arm64.... both of which are fair,
>>> hence my initial version created a second kmem_cache.
>> 
>> Same concern. I believe now is the time to get feedbacks from maintainers.
>> It will help us to decide the next step.
> 
> I'll push back now to avoid further doubts in changing kernel/fork.c ;).

Thanks a lot!

> A reason to define a kmem_cache is performance for repeated allocations.
> But here you only do it once during boot. So you could simply use
> kmalloc() when THREAD_SIZE < PAGE_SIZE. BTW, the IRQ stack size doesn't
> even need to be the same as THREAD_SIZE, though we could initially keep
> them the same. But it's worth defining an IRQ_STACK_SIZE macro if we
> ever need to change it.

I will update the series using IRQ_* macro.

> BTW, a static allocation (DEFINE_PER_CPU for the whole irq stack) would
> save us from another stack address reading on the IRQ entry path. I'm
> not sure exactly where the 16K image increase comes from but at least it
> doesn't grow with NR_CPUS, so we can probably live with this.

I've tried the approach, a static allocation using DEFINE_PER_CPU, but
it dose not work on a top-bit comparison method (for IRQ re-entrance
check). The top-bit idea is based on the assumption that IRQ stack is
aligned with THREAD_SIZE. But, tpidr_el1 is PAGE_SIZE aligned. It leads
to IRQ re-entrance failure in case of 4KB page system.

IMHO, it is hard to avoid 16KB size increase for 64KB page support.
Secondary cores can rely on slab.h, but a boot core cannot. So, IRQ
stack for at least a boot cpu should be allocated statically.

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]

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

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

  	/*
--
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]

#1246619

On Oct 14, 2015, at 4:13 PM, AKASHI Takahiro wrote:
> 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().)

I've got a difference between perf and dump_backtrace() as reviewing perf call
chain operation. Perf relies on walk_stackframe(), but dump_backtrace() does not.
That is, a symbol is printed out *before* unwind_frame() call in case of perf.
By contrast, dump_backtrace() records a symbol *after* unwind_frame(). I think
perf behavior is correct since frame.pc is retrieved from a valid stack frame.

So, the following diff is a prerequisite. It looks reasonable to remove dump_mem()
call since frame.sp is calculated incorrectly now. If accepted, dump_backtrace()
could utilize walk_stackframe(), which simplifies the code.

----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?
In addition, I'm curious about an origin of #-32.

Thanks!

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]

#1246681

On Oct 14, 2015, at 9:24 PM, Jungseok Lee wrote:
> On Oct 14, 2015, at 4:13 PM, AKASHI Takahiro wrote:
>> 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().)
> 
> I've got a difference between perf and dump_backtrace() as reviewing perf call
> chain operation. Perf relies on walk_stackframe(), but dump_backtrace() does not.
> That is, a symbol is printed out *before* unwind_frame() call in case of perf.
> By contrast, dump_backtrace() records a symbol *after* unwind_frame(). I think
> perf behavior is correct since frame.pc is retrieved from a valid stack frame.
> 
> So, the following diff is a prerequisite. It looks reasonable to remove dump_mem()
> call since frame.sp is calculated incorrectly now. If accepted, dump_backtrace()
> could utilize walk_stackframe(), which simplifies the code.
> 
> ----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?

Sorry for confusion.

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]

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

Jungseok,

On 10/14/2015 09:55 PM, Jungseok Lee wrote:
> On Oct 14, 2015, at 9:24 PM, Jungseok Lee wrote:
>> On Oct 14, 2015, at 4:13 PM, AKASHI Takahiro wrote:
>>> 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().)
>>
>> I've got a difference between perf and dump_backtrace() as reviewing perf call
>> chain operation. Perf relies on walk_stackframe(), but dump_backtrace() does not.
>> That is, a symbol is printed out *before* unwind_frame() call in case of perf.
>> By contrast, dump_backtrace() records a symbol *after* unwind_frame(). I think
>> perf behavior is correct since frame.pc is retrieved from a valid stack frame.
>>
>> So, the following diff is a prerequisite. It looks reasonable to remove dump_mem()
>> call since frame.sp is calculated incorrectly now. If accepted, dump_backtrace()
>> could utilize walk_stackframe(), which simplifies the code.
>>
>> ----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."

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

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;

  	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));
+	/*
  	 * -4 here because we care about the PC at time of bl,
  	 * not where the return will go.
  	 */
-- 
1.7.9.5


--
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]

#1247803

On Oct 15, 2015, at 1:19 PM, AKASHI Takahiro wrote:
> Jungseok,

Hi Akashi,

> On 10/14/2015 09:55 PM, Jungseok Lee wrote:
>> On Oct 14, 2015, at 9:24 PM, Jungseok Lee wrote:
>>> On Oct 14, 2015, at 4:13 PM, AKASHI Takahiro wrote:
>>>> 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().)
>>> 
>>> I've got a difference between perf and dump_backtrace() as reviewing perf call
>>> chain operation. Perf relies on walk_stackframe(), but dump_backtrace() does not.
>>> That is, a symbol is printed out *before* unwind_frame() call in case of perf.
>>> By contrast, dump_backtrace() records a symbol *after* unwind_frame(). I think
>>> perf behavior is correct since frame.pc is retrieved from a valid stack frame.
>>> 
>>> So, the following diff is a prerequisite. It looks reasonable to remove dump_mem()
>>> call since frame.sp is calculated incorrectly now. If accepted, dump_backtrace()
>>> could utilize walk_stackframe(), which simplifies the code.
>>> 
>>> ----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?

> 
> 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)

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]

#1247841

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

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

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]

Back to top | Article view | linux.kernel

csiph-web