* [PATCH v1] ftrace: Make ftrace_graph_is_dead() static inline @ 2022-03-24 10:01 Christophe Leroy 2022-03-24 22:33 ` Steven Rostedt 0 siblings, 1 reply; 3+ messages in thread From: Christophe Leroy @ 2022-03-24 10:01 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar; +Cc: Christophe Leroy, linux-kernel, linuxppc-dev ftrace_graph_is_dead() is used on hot paths, it just reads a variable in memory and is not worth suffering function call constraints. For instance, at entry of prepare_ftrace_return(), inlining it avoids saving prepare_ftrace_return() parameters to stack and restoring them after calling ftrace_graph_is_dead(). The performance improvement is noticeable. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- include/linux/ftrace.h | 15 ++++++++++++++- kernel/trace/fgraph.c | 14 +------------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 9999e29187de..cecdda515059 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -1006,7 +1006,20 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx, extern int register_ftrace_graph(struct fgraph_ops *ops); extern void unregister_ftrace_graph(struct fgraph_ops *ops); -extern bool ftrace_graph_is_dead(void); +/** + * ftrace_graph_is_dead - returns true if ftrace_graph_stop() was called + * + * ftrace_graph_stop() is called when a severe error is detected in + * the function graph tracing. This function is called by the critical + * paths of function graph to keep those paths from doing any more harm. + */ +extern bool kill_ftrace_graph; + +static inline bool ftrace_graph_is_dead(void) +{ + return kill_ftrace_graph; +} + extern void ftrace_graph_stop(void); /* The current handlers in use */ diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c index 22061d38fc00..3961f5884063 100644 --- a/kernel/trace/fgraph.c +++ b/kernel/trace/fgraph.c @@ -23,24 +23,12 @@ #define ASSIGN_OPS_HASH(opsname, val) #endif -static bool kill_ftrace_graph; +bool kill_ftrace_graph; int ftrace_graph_active; /* Both enabled by default (can be cleared by function_graph tracer flags */ static bool fgraph_sleep_time = true; -/** - * ftrace_graph_is_dead - returns true if ftrace_graph_stop() was called - * - * ftrace_graph_stop() is called when a severe error is detected in - * the function graph tracing. This function is called by the critical - * paths of function graph to keep those paths from doing any more harm. - */ -bool ftrace_graph_is_dead(void) -{ - return kill_ftrace_graph; -} - /** * ftrace_graph_stop - set to permanently disable function graph tracing * -- 2.35.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1] ftrace: Make ftrace_graph_is_dead() static inline 2022-03-24 10:01 [PATCH v1] ftrace: Make ftrace_graph_is_dead() static inline Christophe Leroy @ 2022-03-24 22:33 ` Steven Rostedt 2022-03-25 7:06 ` Christophe Leroy 0 siblings, 1 reply; 3+ messages in thread From: Steven Rostedt @ 2022-03-24 22:33 UTC (permalink / raw) To: Christophe Leroy; +Cc: Ingo Molnar, linux-kernel, linuxppc-dev On Thu, 24 Mar 2022 11:01:45 +0100 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > @@ -1006,7 +1006,20 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx, > extern int register_ftrace_graph(struct fgraph_ops *ops); > extern void unregister_ftrace_graph(struct fgraph_ops *ops); > > -extern bool ftrace_graph_is_dead(void); > +/** > + * ftrace_graph_is_dead - returns true if ftrace_graph_stop() was called > + * > + * ftrace_graph_stop() is called when a severe error is detected in > + * the function graph tracing. This function is called by the critical > + * paths of function graph to keep those paths from doing any more harm. > + */ > +extern bool kill_ftrace_graph; > + > +static inline bool ftrace_graph_is_dead(void) > +{ > + return kill_ftrace_graph; > +} > + > extern void ftrace_graph_stop(void); The reason I did not expose that variable, is because I didn't want it to be touched outside of the kernel/trace directory. Or the ftrace.c file for that matter (although, I could put it in fgraph.c :-/) What would be better, is to make it a static branch. extern struct static_key fgraph_dead; static inline bool ftrace_graph_is_dead(void) { if (static_key_false(&fgraph_dead)) return true; return false; } That way we even get rid of the conditional branch. Yeah, the fgraph_dead is still exposed for anyone to touch, but it still requires a function to modify it, so I'm not as worried it will be touched as easily. -- Steve ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1] ftrace: Make ftrace_graph_is_dead() static inline 2022-03-24 22:33 ` Steven Rostedt @ 2022-03-25 7:06 ` Christophe Leroy 0 siblings, 0 replies; 3+ messages in thread From: Christophe Leroy @ 2022-03-25 7:06 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, linux-kernel, linuxppc-dev Le 24/03/2022 à 23:33, Steven Rostedt a écrit : > On Thu, 24 Mar 2022 11:01:45 +0100 > Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > >> @@ -1006,7 +1006,20 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx, >> extern int register_ftrace_graph(struct fgraph_ops *ops); >> extern void unregister_ftrace_graph(struct fgraph_ops *ops); >> >> -extern bool ftrace_graph_is_dead(void); >> +/** >> + * ftrace_graph_is_dead - returns true if ftrace_graph_stop() was called >> + * >> + * ftrace_graph_stop() is called when a severe error is detected in >> + * the function graph tracing. This function is called by the critical >> + * paths of function graph to keep those paths from doing any more harm. >> + */ >> +extern bool kill_ftrace_graph; >> + >> +static inline bool ftrace_graph_is_dead(void) >> +{ >> + return kill_ftrace_graph; >> +} >> + >> extern void ftrace_graph_stop(void); > > The reason I did not expose that variable, is because I didn't want it to > be touched outside of the kernel/trace directory. Or the ftrace.c file for > that matter (although, I could put it in fgraph.c :-/) > > What would be better, is to make it a static branch. > > extern struct static_key fgraph_dead; > > static inline bool ftrace_graph_is_dead(void) > { > if (static_key_false(&fgraph_dead)) > return true; > return false; > } > > That way we even get rid of the conditional branch. > > Yeah, the fgraph_dead is still exposed for anyone to touch, but it still > requires a function to modify it, so I'm not as worried it will be touched > as easily. > If your concern is about kill_ftrace_graph being visible in ftrace.h, we could have hidden it with: static inline bool ftrace_graph_is_dead(void) { extern bool kill_ftrace_graph; return kill_ftrace_graph; } But using a static branch is even better, I'll do that. By the way, I was wondering if that call is still necessary. In 2014, in commit 545d47b8f359 ("ftrace-graph: Remove usage of ftrace_stop() in ftrace_graph_stop()") you wrote: "All archs now use ftrace_graph_is_dead() to stop function graph tracing". But when I grep it nowadays, it seems not all architecture are using it: $ git grep "prepare_ftrace_return(" arch/*ftrace.c arch/arm/kernel/ftrace.c:void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, arch/arm64/kernel/ftrace.c:void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent, arch/csky/kernel/ftrace.c:void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, arch/microblaze/kernel/ftrace.c:void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr) arch/mips/kernel/ftrace.c:void prepare_ftrace_return(unsigned long *parent_ra_addr, unsigned long self_ra, arch/nds32/kernel/ftrace.c:void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, arch/nds32/kernel/ftrace.c: prepare_ftrace_return(parent_ip, selfpc, frame_pointer); arch/parisc/kernel/ftrace.c:static void __hot prepare_ftrace_return(unsigned long *parent, arch/parisc/kernel/ftrace.c: prepare_ftrace_return(parent_rp, self_addr); arch/powerpc/kernel/trace/ftrace.c:unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip, arch/powerpc/kernel/trace/ftrace.c: fregs->regs.link = prepare_ftrace_return(parent_ip, ip, fregs->regs.gpr[1]); arch/riscv/kernel/ftrace.c:void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr, arch/s390/kernel/ftrace.c:unsigned long prepare_ftrace_return(unsigned long ra, unsigned long sp, arch/sh/kernel/ftrace.c:void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr) arch/sparc/kernel/ftrace.c:unsigned long prepare_ftrace_return(unsigned long parent, arch/x86/kernel/ftrace.c:void prepare_ftrace_return(unsigned long ip, unsigned long *parent, arch/x86/kernel/ftrace.c:void prepare_ftrace_return(unsigned long ip, unsigned long *parent, arch/x86/kernel/ftrace.c: prepare_ftrace_return(ip, (unsigned long *)stack, 0); $ git grep "ftrace_graph_is_dead()" arch/*ftrace.c arch/microblaze/kernel/ftrace.c: if (unlikely(ftrace_graph_is_dead())) arch/mips/kernel/ftrace.c: if (unlikely(ftrace_graph_is_dead())) arch/parisc/kernel/ftrace.c: if (unlikely(ftrace_graph_is_dead())) arch/powerpc/kernel/trace/ftrace.c: if (unlikely(ftrace_graph_is_dead())) arch/s390/kernel/ftrace.c: if (unlikely(ftrace_graph_is_dead())) arch/sh/kernel/ftrace.c: if (unlikely(ftrace_graph_is_dead())) arch/x86/kernel/ftrace.c: if (unlikely(ftrace_graph_is_dead())) But back in commit 545d47b8f359, there was already architectures implementing prepare_ftrace_return() without calling ftrace_graph_is_dead(), for instance s390. So I must be missing something I guess ? Thanks Christophe ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-25 7:06 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-24 10:01 [PATCH v1] ftrace: Make ftrace_graph_is_dead() static inline Christophe Leroy 2022-03-24 22:33 ` Steven Rostedt 2022-03-25 7:06 ` Christophe Leroy
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).