On Tue, Nov 11, 2025 at 4:43 AM Steven Rostedt <[email protected]> wrote:
>
> On Sun, 12 Oct 2025 00:41:56 +0800
> pengdonglin <[email protected]> wrote:
>
> > From: pengdonglin <[email protected]>
>
> Sorry for the late reply. I finally got around to looking at this.
>
> >
> > Currently, funcgraph-args and funcgraph-retaddr cannot be used together.
> > This patch
> > resolves the conflict by having funcgraph-retaddr reuse the implementation
> > of
> > funcgraph-args -- specifically, storing the return address in the last
> > entry of the
> > args array.
> >
> > As a result, both features now coexist seamlessly and function as intended.
> >
> > To verify the change, use perf to trace vfs_write with both options enabled:
> >
> > # perf_6_17 ftrace -G vfs_write --graph-opts args,retaddr
> > ......
> > 0) | down_read(sem=0xffff8880100bea78) { /*
> > <-n_tty_write+0xa3/0x540 */
> > 0) 0.075 us | __cond_resched(); /* <-down_read+0x12/0x160 */
> > 0) 0.079 us | preempt_count_add(val=1); /* <-down_read+0x3b/0x160
> > */
> > 0) 0.077 us | preempt_count_sub(val=1); /* <-down_read+0x8b/0x160
> > */
> > 0) 0.754 us | }
> >
> > Cc: Steven Rostedt (Google) <[email protected]>
> > Cc: Sven Schnelle <[email protected]>
> > Cc: Masami Hiramatsu <[email protected]>
> > Signed-off-by: pengdonglin <[email protected]>
> > Signed-off-by: pengdonglin <[email protected]>
> > ---
> > include/linux/ftrace.h | 11 --
> > kernel/trace/trace.h | 7 --
> > kernel/trace/trace_entries.h | 29 +-----
> > kernel/trace/trace_functions_graph.c | 148 +++++++++++----------------
> > kernel/trace/trace_selftest.c | 1 -
> > 5 files changed, 58 insertions(+), 138 deletions(-)
> >
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 7ded7df6e9b5..88cb54d73bdb 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
> > int depth;
> > } __packed;
> >
> > -/*
> > - * Structure that defines an entry function trace with retaddr.
> > - * It's already packed but the attribute "packed" is needed
> > - * to remove extra padding at the end.
> > - */
> > -struct fgraph_retaddr_ent {
> > - unsigned long func; /* Current function */
> > - int depth;
> > - unsigned long retaddr; /* Return address */
> > -} __packed;
>
> I really like the clean up, but unfortunately, this breaks user space.
>
> We still need the retaddr event, as that is what user space expects.
>
> That said, this could do the same thing as the func-args. That is, it can
> add the function arguments after the retaddr field.
Thanks, will do in v2.
Thanks,
Donglin
>
> > TRACE_GRAPH_RET,
> > diff --git a/kernel/trace/trace_functions_graph.c
> > b/kernel/trace/trace_functions_graph.c
> > index a7f4b9a47a71..b618b6a673b7 100644
> > --- a/kernel/trace/trace_functions_graph.c
> > +++ b/kernel/trace/trace_functions_graph.c
> > @@ -16,6 +16,12 @@
> > #include "trace.h"
> > #include "trace_output.h"
> >
> > +#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > +#define STORE_RETADDR 1
> > +#else
> > +#define STORE_RETADDR 0
> > +#endif
> > +
> > /* When set, irq functions will be ignored */
> > static int ftrace_graph_skip_irqs;
> >
> > @@ -29,19 +35,17 @@ struct fgraph_cpu_data {
> >
> > struct fgraph_ent_args {
> > struct ftrace_graph_ent_entry ent;
> > - /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
> > - unsigned long args[FTRACE_REGS_MAX_ARGS];
> > + /* Force the sizeof of args[] to have
> > (FTRACE_REGS_MAX_ARGS+STORE_RETADDR) entries,
> > + * and the last entry is used to store the retaddr
> > + */
> > + unsigned long args[FTRACE_REGS_MAX_ARGS +
> > STORE_RETADDR];
>
> I'm thinking, if it is a different event, then it can still use the same
> array. But retaddr will be first, and not last.
Thanks, will do in v2.
Thanks,
Donglin
>
> > };
> >
> > struct fgraph_data {
> > struct fgraph_cpu_data __percpu *cpu_data;
> >
> > /* Place to preserve last processed entry. */
> > - union {
> > - struct fgraph_ent_args ent;
> > - /* TODO allow retaddr to have args */
> > - struct fgraph_retaddr_ent_entry rent;
> > - };
> > + struct fgraph_ent_args ent;
>
> Where this could still be the same.
>
> > struct ftrace_graph_ret_entry ret;
> > int failed;
> > int cpu;
> > @@ -127,11 +131,19 @@ static int __graph_entry(struct trace_array *tr,
> > struct ftrace_graph_ent *trace,
> > struct ring_buffer_event *event;
> > struct trace_buffer *buffer = tr->array_buffer.buffer;
> > struct ftrace_graph_ent_entry *entry;
> > + unsigned long retaddr = 0;
> > int size;
> > + int i = 0;
> >
> > /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
> > size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs *
> > sizeof(long));
> >
> > + if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
> > + tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
> > + retaddr = ftrace_graph_top_ret_addr(current);
> > + size += sizeof(long);
> > + }
> > +
> > event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size,
> > trace_ctx);
> > if (!event)
> > return 0;
> > @@ -141,11 +153,17 @@ static int __graph_entry(struct trace_array *tr,
> > struct ftrace_graph_ent *trace,
> >
> > #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> > if (fregs) {
> > - for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
> > + for (; i < FTRACE_REGS_MAX_ARGS; i++)
> > entry->args[i] = ftrace_regs_get_argument(fregs, i);
> > }
> > #endif
> >
> > + /*
> > + * Store retaddr to [0] if fregs is NULL, else to
> > [FTRACE_REGS_MAX_ARGS]
> > + */
> > + if (retaddr)
> > + entry->args[i] = retaddr;
> > +
>
> Move this up before the args.
Thanks, will do in v2.
Thanks,
Donglin
>
> > trace_buffer_unlock_commit_nostack(buffer, event);
> >
> > return 1;
> > @@ -158,38 +176,6 @@ int __trace_graph_entry(struct trace_array *tr,
> > return __graph_entry(tr, trace, trace_ctx, NULL);
> > }
> >
> > -#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> > -int __trace_graph_retaddr_entry(struct trace_array *tr,
> > - struct ftrace_graph_ent *trace,
> > - unsigned int trace_ctx,
> > - unsigned long retaddr)
> > -{
> > - struct ring_buffer_event *event;
> > - struct trace_buffer *buffer = tr->array_buffer.buffer;
> > - struct fgraph_retaddr_ent_entry *entry;
> > -
> > - event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
> > - sizeof(*entry), trace_ctx);
>
> The reserve would need to use TRACE_GRAPH_RETADDR_ENT if retaddr is set.
> But I still think you can consolidate the code.
>
> -- Steve
>
> > - if (!event)
> > - return 0;
> > - entry = ring_buffer_event_data(event);
> > - entry->graph_ent.func = trace->func;
> > - entry->graph_ent.depth = trace->depth;
> > - entry->graph_ent.retaddr = retaddr;
> > - trace_buffer_unlock_commit_nostack(buffer, event);
> > -
> > - return 1;
> > -}
> > -#else
> > -int __trace_graph_retaddr_entry(struct trace_array *tr,
> > - struct ftrace_graph_ent *trace,
> > - unsigned int trace_ctx,
> > - unsigned long retaddr)
> > -{
> > - return 1;
> > -}
> > -#endif
> > -
> > static inline int ftrace_graph_ignore_irqs(void)
> > {
> > if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
