Messages in this thread | | | Date | Tue, 19 Apr 2022 21:27:31 +0800 | Subject | Re: [External] Re: [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly | From | Chengming Zhou <> |
| |
On 2022/4/19 20:55, Mark Rutland wrote: > On Sat, Apr 09, 2022 at 11:35:54PM +0800, Chengming Zhou wrote: >> As we do in commit 0c0593b45c9b ("x86/ftrace: Make function graph >> use ftrace directly"), we don't need special hook for graph tracer, >> but instead we use graph_ops:func function to install return_hooker. >> >> Since commit 3b23e4991fb6 ("arm64: implement ftrace with regs") add >> implementation for FTRACE_WITH_REGS on arm64, we can easily adopt >> the same cleanup on arm64. >> >> And this cleanup only changes the FTRACE_WITH_REGS implementation, >> so the mcount-based implementation is unaffected. > > Could you please say *why* we only do this for FTRACE_WITH_REGS? IIUC that's > possible, but would require more invasive refactoring of the core code; have I > understood correctly?
Yes, I think so. The static mcount-based implementation should also be changed in this way, but I haven't look too deep into that asm implementation yet.
> > If so, could we please make this: > > | While in theory it would be possible to make a similar cleanup for > | !FTRACE_WITH_REGS, this will require rework of the core code, and so for now > | we only change the FTRACE_WITH_REGS implementation. > > It'd be quite nice if we could clean up the !FTRACE_WITH_REGS case similarly, > but as it appeass that would require far more invasive changes, I'm happy to > leave that for future work.
Ok, will add it in the commit message. And leave this for future work.
> >> >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> >> --- >> v3: >> - Add comments in ftrace_graph_func() as suggested by Steve. Thanks. >> >> v2: >> - Remove FTRACE_WITH_REGS ftrace_graph_caller asm, thanks Mark. >> --- >> arch/arm64/include/asm/ftrace.h | 7 +++++++ >> arch/arm64/kernel/entry-ftrace.S | 17 ----------------- >> arch/arm64/kernel/ftrace.c | 17 +++++++++++++++++ >> 3 files changed, 24 insertions(+), 17 deletions(-) >> >> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h >> index 1494cfa8639b..dbc45a4157fa 100644 >> --- a/arch/arm64/include/asm/ftrace.h >> +++ b/arch/arm64/include/asm/ftrace.h >> @@ -80,8 +80,15 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr) >> >> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS >> struct dyn_ftrace; >> +struct ftrace_ops; >> +struct ftrace_regs; >> + >> int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec); >> #define ftrace_init_nop ftrace_init_nop >> + >> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, >> + struct ftrace_ops *op, struct ftrace_regs *fregs); >> +#define ftrace_graph_func ftrace_graph_func >> #endif >> >> #define ftrace_return_address(n) return_address(n) >> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S >> index e535480a4069..d42a205ef625 100644 >> --- a/arch/arm64/kernel/entry-ftrace.S >> +++ b/arch/arm64/kernel/entry-ftrace.S >> @@ -97,12 +97,6 @@ SYM_CODE_START(ftrace_common) >> SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL) >> bl ftrace_stub >> >> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER >> -SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller(); >> - nop // If enabled, this will be replaced >> - // "b ftrace_graph_caller" >> -#endif >> - >> /* >> * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved >> * x19-x29 per the AAPCS, and we created frame records upon entry, so we need >> @@ -127,17 +121,6 @@ ftrace_common_return: >> ret x9 >> SYM_CODE_END(ftrace_common) >> >> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER >> -SYM_CODE_START(ftrace_graph_caller) >> - ldr x0, [sp, #S_PC] >> - sub x0, x0, #AARCH64_INSN_SIZE // ip (callsite's BL insn) >> - add x1, sp, #S_LR // parent_ip (callsite's LR) >> - ldr x2, [sp, #PT_REGS_SIZE] // parent fp (callsite's FP) >> - bl prepare_ftrace_return >> - b ftrace_common_return >> -SYM_CODE_END(ftrace_graph_caller) >> -#endif >> - >> #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ >> >> /* >> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c >> index 4506c4a90ac1..35eb7c9b5e53 100644 >> --- a/arch/arm64/kernel/ftrace.c >> +++ b/arch/arm64/kernel/ftrace.c >> @@ -268,6 +268,22 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent, >> } >> >> #ifdef CONFIG_DYNAMIC_FTRACE >> + >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS >> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip, >> + struct ftrace_ops *op, struct ftrace_regs *fregs) >> +{ >> + /* >> + * Athough graph_ops doesn't have FTRACE_OPS_FL_SAVE_REGS set in flags, >> + * regs can't be NULL in DYNAMIC_FTRACE_WITH_REGS. By design, it should >> + * be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented. >> + */ > > This is a bit confusing, since it makes it sound like there's an bug in the > current implementation, rather than something that would need to change if > support for DYNAMIC_FTRACE_WITH_ARGS is added. > > Could we please make this: > > /* > * When DYNAMIC_FTRACE_WITH_REGS is selected, `fregs` can never be NULL > * and arch_ftrace_get_regs(fregs) will always give a non-NULL pt_regs > * in which we can safely modify the LR. > */ >
Ok, will do. This expression is nicer, the previous comment maybe make people think it's an bug to be fixed.
> Other than that, this looks good to me. I gave it a spin under QEMU atop > v5.18-rc3. The CONFIG_FTRACE_STARTUP_TEST tests all pass, and I played with the > graph tracer with: > > | # echo do_el0_svc > /sys/kernel/tracing/set_graph_function > | # echo function_graph > /sys/kernel/tracing/current_tracer > > ... for which the resutls looks sane. > > To make sure this didn't adversely affect the return address rewriting, I also > concurrently ran perf with: > > | # perf record -g -e raw_syscalls:sys_enter:k /bin/true > | # perf report > > ... for which the results also looked fine. > > I also tested the !DYNAMIC_FTRACE_WITH_REGS modes by building with an older > compiler and also building with !DYNAMIC_FTRACE, which all looked good. > > So FWIW: > > Tested-by: Mark Rutland <mark.rutland@arm.com> > > ... and if you make the changes I requested above: > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > If you could spin a v5 with that folded in, that would be great.
Of course, will do in v5.
Thanks.
> > Thanks, > Mark.
| |