Messages in this thread | | | Subject | Re: [PATCH v5 2/6] arm64: ftrace: fix a stack tracer's output under function graph tracer | From | AKASHI Takahiro <> | Date | Mon, 16 Nov 2015 18:23:51 +0900 |
| |
Jungseok,
On 11/14/2015 12:01 AM, Jungseok Lee wrote: > (+ Li Bin in CC) > > On Nov 10, 2015, at 11:42 AM, AKASHI Takahiro wrote: >> On 11/09/2015 11:04 PM, Jungseok Lee wrote: >>> On Nov 6, 2015, at 3:44 PM, AKASHI Takahiro wrote: >>> >>> Hi Akashi, >>> >>>> Function graph tracer modifies a return address (LR) in a stack frame >>>> to hook a function return. This will result in many useless entries >>>> (return_to_handler) showing up in a stack tracer's output. >>>> >>>> This patch replaces such entries with originals values preserved in >>>> current->ret_stack[]. >>>> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>> --- >>>> arch/arm64/include/asm/ftrace.h | 2 ++ >>>> arch/arm64/kernel/stacktrace.c | 21 +++++++++++++++++++++ >>>> 2 files changed, 23 insertions(+) >>>> >>>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h >>>> index c5534fa..3c60f37 100644 >>>> --- a/arch/arm64/include/asm/ftrace.h >>>> +++ b/arch/arm64/include/asm/ftrace.h >>>> @@ -28,6 +28,8 @@ struct dyn_arch_ftrace { >>>> >>>> extern unsigned long ftrace_graph_call; >>>> >>>> +extern void return_to_handler(void); >>>> + >>>> static inline unsigned long ftrace_call_adjust(unsigned long addr) >>>> { >>>> /* >>>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>>> index ccb6078..5fd3477 100644 >>>> --- a/arch/arm64/kernel/stacktrace.c >>>> +++ b/arch/arm64/kernel/stacktrace.c >>>> @@ -17,6 +17,7 @@ >>>> */ >>>> #include <linux/kernel.h> >>>> #include <linux/export.h> >>>> +#include <linux/ftrace.h> >>>> #include <linux/sched.h> >>>> #include <linux/stacktrace.h> >>>> >>>> @@ -73,6 +74,9 @@ struct stack_trace_data { >>>> struct stack_trace *trace; >>>> unsigned int no_sched_functions; >>>> unsigned int skip; >>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER >>>> + unsigned int ret_stack_index; >>>> +#endif >>>> }; >>>> >>>> static int save_trace(struct stackframe *frame, void *d) >>>> @@ -81,6 +85,20 @@ static int save_trace(struct stackframe *frame, void *d) >>>> struct stack_trace *trace = data->trace; >>>> unsigned long addr = frame->pc; >>>> >>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER >>>> + if (addr == (unsigned long)return_to_handler - AARCH64_INSN_SIZE) { >>> >>> not if (adds == (unsigned long)return_to_handler)? >>> >>>> + /* >>>> + * This is a case where function graph tracer has >>>> + * modified a return address (LR) in a stack frame >>>> + * to hook a function return. >>>> + * So replace it to an original value. >>>> + */ >>>> + frame->pc = addr = >>>> + current->ret_stack[data->ret_stack_index--].ret >>>> + - AARCH64_INSN_SIZE; >>> >>> Ditto. not without AARCH64_INSN_SIZE? >>> >>> I've observed many return_to_handler without the changes. >>> Am I missing something? >> >> You're right! >> I thought I had tested the patches, but... >> >>>> + } >>>> +#endif /* CONFIG_FUNCTION_GRAPH_TRACER */ >>>> + >>>> if (data->no_sched_functions && in_sched_functions(addr)) >>>> return 0; >>>> if (data->skip) { >>>> @@ -100,6 +118,9 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace) >>>> >>>> data.trace = trace; >>>> data.skip = trace->skip; >>>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER >>>> + data.ret_stack_index = current->curr_ret_stack; >>> >>> Can I get an idea on why current->curr_ret_stack is used instead of >>> tsk->curr_ret_stack? >> >> Thanks for pointing this out. >> Will fix it although it works without a change since save_stack_trace_sp() is >> called only in a 'current task' context. >> >> -Takahiro AKASHI > > As reading function_graph related codes in arm64, I've realized that this issue > can be observed from three different sources. > > (A) stack tracer of ftrace > (B) perf call trace (perf record with '-g' option) > (C) dump_backtrace > > The issue is orthogonal to the commit, e306dfd06f, and its revert. It seems that > Steve's approach, 7ee991fbc6, would be valid on arm64 and cover all three cases. > It does in case of x86. Li Bin posted a patch [1] to solve the issue from case(C) > in Steve's way. This hunk deals with case(A) with its own implementation. But, > case(B) is not covered yet. It can be reproduced easily with the following steps. > > # echo function_graph > /sys/kernel/debug/tracing/current_tracer > # perf record -g sleep 2 > # perf report --call-graph > > So, how about considering Steve's approach on arm64 and then covering all three > cases with it? It would be good in code consolidation perspective. Note that the > idea is applied to arch/sh.
Thank you for pointing this out. I've already fixed all the cases, (A),(B) and (C), but in a different way. I think that the point is that we should take care of frame->pc under function graph tracer in one place, that is, unwind_frame().
After a bit more testing, I will submit a new version. Then please review it again.
Thanks, -Takahiro AKASHI
> Best Regards > Jungseok Lee > > [1] https://lkml.org/lkml/2015/10/15/368 >
| |