Messages in this thread | | | From | Christophe Leroy <> | Subject | Re: [PATCH v1] ftrace: Make ftrace_graph_is_dead() static inline | Date | Fri, 25 Mar 2022 07:06:45 +0000 |
| |
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
| |