On Oct 16, 2015, at 2:26 AM, Will Deacon wrote:
Hi Will,
> On Thu, Oct 15, 2015 at 01:21:54PM +0000, Jungseok Lee wrote:
>> dump_backtrace() has its own backtrace logic unlike perf callchain which
>> relies on walk_stackframe(). They behave differently when a symbol is
>> recorded. Perf writes it down *before* calling unwind_frame(), but
>> dump_backtrace() prints it out *after* unwind_frame(). As a result, the
>> last valid symbol is not added to a list in case of dump_backtrace().
>>
>> This patch catches up the last symbol as synchronising dump_backtrace()
>> with perf callchain. However, the patch does not cover a case where MMU
>> is disabled. That is, a physical address can be stored in stack frame,
>> but it's not handled. For example, a swapper process falls into this case.
>> Unlike a swapper from a secondary core, a swapper on a boot cpu, which
>> starting from __mmap_switched(), can't be tracked down with a simple
>> conversion, phys_to_virt(), because PC is retrieved from LR - 4, not LR.
>
> It would be good to have an example backtrace before and after this patch
> is applied, to show what it fixes.
Agreed. I will add a call trace data to the commit message.
>> It is a big tradeoff to change both head.S and unwind_frame() structure
>> for a few of symbols in *.S, so this hunk does not take care of the case.
>>
>> Cc: AKASHI Takahiro <takahiro.aka...@linaro.org>
>> Cc: James Morse <james.mo...@arm.com>
>> Cc: Mark Rutland <mark.rutl...@arm.com>
>> Signed-off-by: Jungseok Lee <jungseokle...@gmail.com>
>> ---
>> arch/arm64/kernel/traps.c | 16 +++++++++++-----
>> 1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index f93aae5..4ddb928 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -103,12 +103,13 @@ 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)
>> {
>> + /*
>> + * The highest stack frame of a swapper process stores PC in a form
>> + * of physical address, but this case is not handled.
>> + */
>> 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 +173,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);
>
> AFAICT, the original code is all based on unwind_backtrace in
> arch/arm/kernel/unwind.c. Does that need updating too (as a separate patch)?
I think so, but I don't have any evidence from a real hardware..
Best Regards
Jungseok Lee--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
