* [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable @ 2022-04-09 15:35 Chengming Zhou 2022-04-09 15:35 ` [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly Chengming Zhou ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Chengming Zhou @ 2022-04-09 15:35 UTC (permalink / raw) To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa, broonie, mark.rutland, ardb, zhouchengming, linux-arm-kernel Cc: linux-kernel, duanxiongchun, songmuchun The ftrace_[enable,disable]_ftrace_graph_caller() are used to do special hooks for graph tracer, which are not needed on some ARCHs that use graph_ops:func function to install return_hooker. So introduce the weak version in ftrace core code to cleanup in x86. Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> --- v4: - put weak ftrace_enable,disable_ftrace_graph_caller() in fgraph.c instead of ftrace.c as suggested by Steve. v3: - consolidate two #if into a single #if, suggested by Steve. Thanks. --- arch/x86/kernel/ftrace.c | 17 ++--------------- kernel/trace/fgraph.c | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index 1e31c7d21597..b09d73c2ba89 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops) #ifdef CONFIG_FUNCTION_GRAPH_TRACER -#ifdef CONFIG_DYNAMIC_FTRACE - -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) extern void ftrace_graph_call(void); static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr) { @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void) return ftrace_mod_jmp(ip, &ftrace_stub); } -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ -int ftrace_enable_ftrace_graph_caller(void) -{ - return 0; -} - -int ftrace_disable_ftrace_graph_caller(void) -{ - return 0; -} -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ -#endif /* !CONFIG_DYNAMIC_FTRACE */ +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ /* * Hook the return address and push it in the stack of return addrs diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c index 8f4fb328133a..289311680c29 100644 --- a/kernel/trace/fgraph.c +++ b/kernel/trace/fgraph.c @@ -30,6 +30,24 @@ int ftrace_graph_active; /* Both enabled by default (can be cleared by function_graph tracer flags */ static bool fgraph_sleep_time = true; +/* + * archs can override this function if they must do something + * to enable hook for graph tracer. + */ +int __weak ftrace_enable_ftrace_graph_caller(void) +{ + return 0; +} + +/* + * archs can override this function if they must do something + * to disable hook for graph tracer. + */ +int __weak ftrace_disable_ftrace_graph_caller(void) +{ + return 0; +} + /** * ftrace_graph_stop - set to permanently disable function graph tracing * -- 2.35.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly 2022-04-09 15:35 [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou @ 2022-04-09 15:35 ` Chengming Zhou 2022-04-19 12:55 ` Mark Rutland 2022-04-18 13:24 ` [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou 2022-04-19 11:41 ` Mark Rutland 2 siblings, 1 reply; 7+ messages in thread From: Chengming Zhou @ 2022-04-09 15:35 UTC (permalink / raw) To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa, broonie, mark.rutland, ardb, zhouchengming, linux-arm-kernel Cc: linux-kernel, duanxiongchun, songmuchun 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. 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. + */ + struct pt_regs *regs = arch_ftrace_get_regs(fregs); + unsigned long *parent = (unsigned long *)&procedure_link_pointer(regs); + + prepare_ftrace_return(ip, parent, frame_pointer(regs)); +} +#else /* * Turn on/off the call to ftrace_graph_caller() in ftrace_caller() * depending on @enable. @@ -297,5 +313,6 @@ int ftrace_disable_ftrace_graph_caller(void) { return ftrace_modify_graph_caller(false); } +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */ #endif /* CONFIG_DYNAMIC_FTRACE */ #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ -- 2.35.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly 2022-04-09 15:35 ` [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly Chengming Zhou @ 2022-04-19 12:55 ` Mark Rutland 2022-04-19 13:27 ` [External] " Chengming Zhou 0 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2022-04-19 12:55 UTC (permalink / raw) To: Chengming Zhou Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel, duanxiongchun, songmuchun 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? 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. > > 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. */ 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. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [External] Re: [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly 2022-04-19 12:55 ` Mark Rutland @ 2022-04-19 13:27 ` Chengming Zhou 0 siblings, 0 replies; 7+ messages in thread From: Chengming Zhou @ 2022-04-19 13:27 UTC (permalink / raw) To: Mark Rutland Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel, duanxiongchun, songmuchun 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable 2022-04-09 15:35 [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou 2022-04-09 15:35 ` [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly Chengming Zhou @ 2022-04-18 13:24 ` Chengming Zhou 2022-04-19 11:41 ` Mark Rutland 2 siblings, 0 replies; 7+ messages in thread From: Chengming Zhou @ 2022-04-18 13:24 UTC (permalink / raw) To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa, broonie, mark.rutland, ardb, linux-arm-kernel Cc: linux-kernel, duanxiongchun, songmuchun Hello, friendly ping :-) On 2022/4/9 23:35, Chengming Zhou wrote: > The ftrace_[enable,disable]_ftrace_graph_caller() are used to do > special hooks for graph tracer, which are not needed on some ARCHs > that use graph_ops:func function to install return_hooker. > > So introduce the weak version in ftrace core code to cleanup > in x86. > > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > v4: > - put weak ftrace_enable,disable_ftrace_graph_caller() in > fgraph.c instead of ftrace.c as suggested by Steve. > > v3: > - consolidate two #if into a single #if, suggested by Steve. Thanks. > --- > arch/x86/kernel/ftrace.c | 17 ++--------------- > kernel/trace/fgraph.c | 18 ++++++++++++++++++ > 2 files changed, 20 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > index 1e31c7d21597..b09d73c2ba89 100644 > --- a/arch/x86/kernel/ftrace.c > +++ b/arch/x86/kernel/ftrace.c > @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops) > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > > -#ifdef CONFIG_DYNAMIC_FTRACE > - > -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS > +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) > extern void ftrace_graph_call(void); > static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr) > { > @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void) > > return ftrace_mod_jmp(ip, &ftrace_stub); > } > -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ > -int ftrace_enable_ftrace_graph_caller(void) > -{ > - return 0; > -} > - > -int ftrace_disable_ftrace_graph_caller(void) > -{ > - return 0; > -} > -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ > -#endif /* !CONFIG_DYNAMIC_FTRACE */ > +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ > > /* > * Hook the return address and push it in the stack of return addrs > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c > index 8f4fb328133a..289311680c29 100644 > --- a/kernel/trace/fgraph.c > +++ b/kernel/trace/fgraph.c > @@ -30,6 +30,24 @@ int ftrace_graph_active; > /* Both enabled by default (can be cleared by function_graph tracer flags */ > static bool fgraph_sleep_time = true; > > +/* > + * archs can override this function if they must do something > + * to enable hook for graph tracer. > + */ > +int __weak ftrace_enable_ftrace_graph_caller(void) > +{ > + return 0; > +} > + > +/* > + * archs can override this function if they must do something > + * to disable hook for graph tracer. > + */ > +int __weak ftrace_disable_ftrace_graph_caller(void) > +{ > + return 0; > +} > + > /** > * ftrace_graph_stop - set to permanently disable function graph tracing > * _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable 2022-04-09 15:35 [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou 2022-04-09 15:35 ` [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly Chengming Zhou 2022-04-18 13:24 ` [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou @ 2022-04-19 11:41 ` Mark Rutland 2022-04-20 15:30 ` [External] " Chengming Zhou 2 siblings, 1 reply; 7+ messages in thread From: Mark Rutland @ 2022-04-19 11:41 UTC (permalink / raw) To: Chengming Zhou Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel, duanxiongchun, songmuchun On Sat, Apr 09, 2022 at 11:35:53PM +0800, Chengming Zhou wrote: > The ftrace_[enable,disable]_ftrace_graph_caller() are used to do > special hooks for graph tracer, which are not needed on some ARCHs > that use graph_ops:func function to install return_hooker. > > So introduce the weak version in ftrace core code to cleanup > in x86. > > Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> > --- > v4: > - put weak ftrace_enable,disable_ftrace_graph_caller() in > fgraph.c instead of ftrace.c as suggested by Steve. > > v3: > - consolidate two #if into a single #if, suggested by Steve. Thanks. > --- > arch/x86/kernel/ftrace.c | 17 ++--------------- > kernel/trace/fgraph.c | 18 ++++++++++++++++++ > 2 files changed, 20 insertions(+), 15 deletions(-) > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > index 1e31c7d21597..b09d73c2ba89 100644 > --- a/arch/x86/kernel/ftrace.c > +++ b/arch/x86/kernel/ftrace.c > @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops) > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > > -#ifdef CONFIG_DYNAMIC_FTRACE > - > -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS > +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) > extern void ftrace_graph_call(void); > static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr) > { > @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void) > > return ftrace_mod_jmp(ip, &ftrace_stub); > } > -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ > -int ftrace_enable_ftrace_graph_caller(void) > -{ > - return 0; > -} > - > -int ftrace_disable_ftrace_graph_caller(void) > -{ > - return 0; > -} > -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ > -#endif /* !CONFIG_DYNAMIC_FTRACE */ > +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ > > /* > * Hook the return address and push it in the stack of return addrs > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c > index 8f4fb328133a..289311680c29 100644 > --- a/kernel/trace/fgraph.c > +++ b/kernel/trace/fgraph.c > @@ -30,6 +30,24 @@ int ftrace_graph_active; > /* Both enabled by default (can be cleared by function_graph tracer flags */ > static bool fgraph_sleep_time = true; > > +/* > + * archs can override this function if they must do something > + * to enable hook for graph tracer. > + */ > +int __weak ftrace_enable_ftrace_graph_caller(void) > +{ > + return 0; > +} > + > +/* > + * archs can override this function if they must do something > + * to disable hook for graph tracer. > + */ > +int __weak ftrace_disable_ftrace_graph_caller(void) > +{ > + return 0; > +} IIUC an arch should either: * Have ftrace_graph_call() * Have both ftrace_enable_ftrace_graph_caller() and ftrace_disable_ftrace_graph_caller() ... and I can't think of a reason an arch would need both ftrace_graph_call() *and* the enable/disable functions. Given that, could we drop the `__weak` and place these within ifdeffery, i.e. make the above: | #ifndef ftrace_graph_call | int ftrace_enable_ftrace_graph_caller(void) { return 0; } | int ftrace_disable_ftrace_graph_caller(void) { return 0; } | #endif /* ftrace_graph_call *. That way we'd catch cases when: * An architecture meant to provide one of these functions, but forgot (e.g. the name got typo'd) * An architecture provides an unnecessary implementation of either of these functions. Regardless, this looks ok to me. Steve, are you happy with this? I suspect we'd need to take this via the arm64 tree with the next patch, so we'd need your Ack. Thanks, Mark. > + > /** > * ftrace_graph_stop - set to permanently disable function graph tracing > * > -- > 2.35.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [External] Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable 2022-04-19 11:41 ` Mark Rutland @ 2022-04-20 15:30 ` Chengming Zhou 0 siblings, 0 replies; 7+ messages in thread From: Chengming Zhou @ 2022-04-20 15:30 UTC (permalink / raw) To: Mark Rutland, rostedt Cc: mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel, duanxiongchun, songmuchun Hi, On 2022/4/19 19:41, Mark Rutland wrote: > On Sat, Apr 09, 2022 at 11:35:53PM +0800, Chengming Zhou wrote: >> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do >> special hooks for graph tracer, which are not needed on some ARCHs >> that use graph_ops:func function to install return_hooker. >> >> So introduce the weak version in ftrace core code to cleanup >> in x86. >> >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> >> --- >> v4: >> - put weak ftrace_enable,disable_ftrace_graph_caller() in >> fgraph.c instead of ftrace.c as suggested by Steve. >> >> v3: >> - consolidate two #if into a single #if, suggested by Steve. Thanks. >> --- >> arch/x86/kernel/ftrace.c | 17 ++--------------- >> kernel/trace/fgraph.c | 18 ++++++++++++++++++ >> 2 files changed, 20 insertions(+), 15 deletions(-) >> >> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c >> index 1e31c7d21597..b09d73c2ba89 100644 >> --- a/arch/x86/kernel/ftrace.c >> +++ b/arch/x86/kernel/ftrace.c >> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops) >> >> #ifdef CONFIG_FUNCTION_GRAPH_TRACER >> >> -#ifdef CONFIG_DYNAMIC_FTRACE >> - >> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS >> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS) >> extern void ftrace_graph_call(void); >> static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr) >> { >> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void) >> >> return ftrace_mod_jmp(ip, &ftrace_stub); >> } >> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ >> -int ftrace_enable_ftrace_graph_caller(void) >> -{ >> - return 0; >> -} >> - >> -int ftrace_disable_ftrace_graph_caller(void) >> -{ >> - return 0; >> -} >> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ >> -#endif /* !CONFIG_DYNAMIC_FTRACE */ >> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */ >> >> /* >> * Hook the return address and push it in the stack of return addrs >> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c >> index 8f4fb328133a..289311680c29 100644 >> --- a/kernel/trace/fgraph.c >> +++ b/kernel/trace/fgraph.c >> @@ -30,6 +30,24 @@ int ftrace_graph_active; >> /* Both enabled by default (can be cleared by function_graph tracer flags */ >> static bool fgraph_sleep_time = true; >> >> +/* >> + * archs can override this function if they must do something >> + * to enable hook for graph tracer. >> + */ >> +int __weak ftrace_enable_ftrace_graph_caller(void) >> +{ >> + return 0; >> +} >> + >> +/* >> + * archs can override this function if they must do something >> + * to disable hook for graph tracer. >> + */ >> +int __weak ftrace_disable_ftrace_graph_caller(void) >> +{ >> + return 0; >> +} > > IIUC an arch should either: > > * Have ftrace_graph_call() > > * Have both ftrace_enable_ftrace_graph_caller() and > ftrace_disable_ftrace_graph_caller() > > ... and I can't think of a reason an arch would need both ftrace_graph_call() > *and* the enable/disable functions. This way is more precise, but we have to add "#define ftrace_graph_call ftrace_graph_call" in all ARCH's asm/ftrace.h correctly. Previously we only need to define override ftrace_[enable,disable]_ftrace_graph_caller() in ARCH's ftrace.c, looks like a little more complexity? > > Given that, could we drop the `__weak` and place these within ifdeffery, i.e. > make the above: > > | #ifndef ftrace_graph_call > | int ftrace_enable_ftrace_graph_caller(void) { return 0; } > | int ftrace_disable_ftrace_graph_caller(void) { return 0; } > | #endif /* ftrace_graph_call *. > > That way we'd catch cases when: > > * An architecture meant to provide one of these functions, but forgot (e.g. the > name got typo'd) > > * An architecture provides an unnecessary implementation of either of these > functions. I tried this way today, and it works too. I'm ok with both way. Maybe I should send v5 with this patch unchanged first. Looking forward to Steve's opinion. Thanks. > > Regardless, this looks ok to me. Steve, are you happy with this? I suspect we'd > need to take this via the arm64 tree with the next patch, so we'd need your Ack. > > Thanks, > Mark. > >> + >> /** >> * ftrace_graph_stop - set to permanently disable function graph tracing >> * >> -- >> 2.35.1 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-20 15:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-09 15:35 [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou 2022-04-09 15:35 ` [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly Chengming Zhou 2022-04-19 12:55 ` Mark Rutland 2022-04-19 13:27 ` [External] " Chengming Zhou 2022-04-18 13:24 ` [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou 2022-04-19 11:41 ` Mark Rutland 2022-04-20 15:30 ` [External] " Chengming Zhou
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).