Re: [RFC PATCH] function_graph: Support recording and printing the function return address

[Donglin Peng]

On Wed, Sep 11, 2024 at 12:50 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Tue, 10 Sep 2024 14:15:39 +0800
> Donglin Peng <dolinux.peng@xxxxxxxxx> wrote:
> > > >
> > > > See below logs with both funcgraph-retval and funcgraph-retaddr enabled.
> > > >
> > > > 4)               |  load_elf_binary() { /* A=bprm_execve+0x249/0x600 */
> > >
> > > I wonder if we should make this look more like the function tracer when it
> > > shows the parent. That is:
> > >
> > >   4)               |  load_elf_binary() { /* <-bprm_execve+0x249/0x600 */
> >
> > Nice, Is it necessary to add a prefix letter for the return value? For
> > example, when
> >  both funcgraph-retval and funcgraph-retaddr are enabled, like this:
> >
> >          avc_policy_seqno(); /* <-selinux_file_permission+0x107/0x140 V=0x0 */
>
> How about:
>
> >          avc_policy_seqno(); /* <-selinux_file_permission+0x107/0x140 ret=0x0 */
>
> As "ret" is more obvious than "V".

I agree, and will modify it, thanks.

>
> >
> > >
> > > > 4)               |    load_elf_phdrs() { /* A=load_elf_binary+0x84/0x1730 */
> > > > 4)               |      __kmalloc_noprof() { /* A=load_elf_phdrs+0x4a/0xb0 */
> > > > 4) + 47.910 us   |        __cond_resched(); /* V=0x0 A=__kmalloc_noprof+0x28c/0x390 */
> > > > 4) ! 204.142 us  |      } /* __kmalloc_noprof V=0xffff888201e32c00 */
> > > > 4)               |      kernel_read() { /* A=load_elf_phdrs+0x6c/0xb0 */
> > > > 4)               |        rw_verify_area() { /* A=kernel_read+0x2b/0x50 */
> > > > 4)               |          security_file_permission() {
> > > > 4)               |            selinux_file_permission() { /* A=security_file_permission+0x26/0x40 */
> > > > 4)               |              __inode_security_revalidate() { /* A=selinux_file_permission+0x6d/0x140 */
> > > > 4)   1.182 us    |                __cond_resched(); /* V=0x0 A=__inode_security_revalidate+0x5f/0x80 */
> > > > 4)   4.138 us    |              } /* __inode_security_revalidate V=0x0 */
> > > > 4)   1.513 us    |              avc_policy_seqno(); /* V=0x0 A=selinux_file_permission+0x107/0x140 */
> > > > 4) + 12.133 us   |            } /* selinux_file_permission V=0x0 */
> > > > 4) + 39.834 us   |          } /* security_file_permission V=0x0 */
> > > > 4) + 42.710 us   |        } /* rw_verify_area V=0x0 */
> > > >
>
> > > > +#ifdef CONFIG_FUNCTION_GRAPH_RETVAL
> > > >  /* Function call entry */
> > > >  FTRACE_ENTRY_PACKED(funcgraph_entry, ftrace_graph_ent_entry,
> > > >
> > > > @@ -79,6 +80,7 @@ FTRACE_ENTRY_PACKED(funcgraph_entry, ftrace_graph_ent_entry,
> > > >       F_STRUCT(
> > > >               __field_struct( struct ftrace_graph_ent,        graph_ent       )
> > > >               __field_packed( unsigned long,  graph_ent,      func            )
> > > > +             __field_packed( unsigned long,  graph_ent,      retaddr         )
> > > >               __field_packed( int,            graph_ent,      depth           )
> > > >       ),
> > > >
> > >
> > > Let's make this a new event, so that when this option is not enabled, we
> > > don't waste the ring buffer. For function tracing, every element added to
> > > the event will add megabytes extra to the ring buffer.
> > >
> > > It should be possible to switch what event gets created at the time of the
> > > trace. Even calling different functions to do it.
> >
> > Sounds good, we may create a new event as follows:
> >
> > struct ftrace_graph_ent {
> > unsigned long func; /* Current function */
> > unsigned long retaddr;
> > int depth;
> > } __packed;
> >
> > FTRACE_ENTRY_PACKED(funcgraph_retaddr_entry, ftrace_graph_ent_readdr_entry,
> >
> > TRACE_GRAPH_RETADDR_ENT,
> >
> > F_STRUCT(
> > __field_struct( struct ftrace_graph_retaddr_ent, graph_retaddr_ent )
> > __field_packed( unsigned long, graph_retaddr_ent, func )
> > __field_packed( unsigned long, graph_retaddr_ent, retaddr )
> > __field_packed( int, graph_retaddr_ent, depth )
> > ),
> >
> > F_printk("--> %ps (%d)", (void *)__entry->func, __entry->depth)
>
> Why not add the ret in the printk?
>
>   F_printk("--> %ps (%d) <-%ps", (void *)__entry->func, __entry->depth, (void *)__entry->retaddr)
>
> Then user space parsing tools could use this by default, when it doesn't
> know how to parse it.

Thanks, I will modify it.

>
> > );
> >
> > If we were to introduce another new feature such as funcgraph-args which
> > adds a `ftrace_regs` to the `ftrace_graph_ent`, would it be necessary to
> > create yet another new event?
>
> Probably yes.
>
> >
> >  In the scenario where both funcgrah-retaddr and funcgraph-args are enabled,
> > it is possible that an additional new event may also be required.
>
> Yes, that would likely be another event.
>
> -- Steve