Re: [PATCH] tracing/fgraph: Simplify return address printing in function graph tracer

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Thu, 10 Oct 2024 10:40:02 +0800
Donglin Peng <dolinux.peng@gmail.com> wrote:

> On Thu, Oct 10, 2024 at 7:35 AM Masami Hiramatsu (Google)
> <mhiramat@kernel.org> wrote:
> >
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Simplify return address printing in the function graph tracer by removing
> > fgraph_extras. Since this feature is only used by the function graph
> > tracer and the feature flags can directly accessible from the function
> > graph tracer, fgraph_extras can be removed from the fgraph callback.
> >
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  include/linux/ftrace.h               |   16 ++++--------
> >  kernel/trace/fgraph.c                |   45 ++++++++++++++++++++++------------
> >  kernel/trace/ftrace.c                |    3 +-
> >  kernel/trace/trace.h                 |    3 +-
> >  kernel/trace/trace_functions_graph.c |   14 +++++------
> >  kernel/trace/trace_irqsoff.c         |    3 +-
> >  kernel/trace/trace_sched_wakeup.c    |    3 +-
> >  kernel/trace/trace_selftest.c        |    8 ++----
> >  8 files changed, 48 insertions(+), 47 deletions(-)
> >
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 2ac3b3b53cd0..997e1f0335b6 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -1068,29 +1068,20 @@ struct ftrace_graph_ret {
> >         unsigned long long rettime;
> >  } __packed;
> >
> > -struct fgraph_extras;
> >  struct fgraph_ops;
> >
> >  /* Type of the callback handlers for tracing function graph*/
> >  typedef void (*trace_func_graph_ret_t)(struct ftrace_graph_ret *,
> >                                        struct fgraph_ops *); /* return */
> >  typedef int (*trace_func_graph_ent_t)(struct ftrace_graph_ent *,
> > -                                     struct fgraph_ops *,
> > -                                     struct fgraph_extras *); /* entry */
> > +                                     struct fgraph_ops *); /* entry */
> >
> >  extern int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace,
> > -                                  struct fgraph_ops *gops,
> > -                                  struct fgraph_extras *extras);
> > +                                  struct fgraph_ops *gops);
> >  bool ftrace_pids_enabled(struct ftrace_ops *ops);
> >
> >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> >
> > -/* Used to convey some extra datas when creating a graph entry */
> > -struct fgraph_extras {
> > -       u32 flags;
> > -       unsigned long retaddr;
> > -};
> > -
> >  struct fgraph_ops {
> >         trace_func_graph_ent_t          entryfunc;
> >         trace_func_graph_ret_t          retfunc;
> > @@ -1131,12 +1122,15 @@ function_graph_enter(unsigned long ret, unsigned long func,
> >
> >  struct ftrace_ret_stack *
> >  ftrace_graph_get_ret_stack(struct task_struct *task, int skip);
> > +unsigned long ftrace_graph_top_ret_addr(struct task_struct *task);
> >
> >  unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,
> >                                     unsigned long ret, unsigned long *retp);
> >  unsigned long *fgraph_get_task_var(struct fgraph_ops *gops);
> >
> >  u32 graph_tracer_flags_get(u32 flags);
> > +#define graph_tracer_flags_is_set(flags)       \
> > +       (graph_tracer_flags_get(flags) == (flags))
> 
> Do we need to retain the function graph_tracer_flags_get? it will be
> only invoked in the
> file trace_functions_graph.c, so we may access the tracer_flags
> variable directly.

Good catch!
Let me send v2.

Thanks!


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>