Re: [PATCH v14 00/19] tracing: fprobe: function_graph: Multi-function graph and fprobe on fgraph

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

On Fri, 13 Sep 2024 14:16:47 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> >
> >  - Change bpf_kprobe_multi_addrs_cmp() to call
> >    ftrace_call_adjust() before comparing. This may take a
> >    cost but find actual entry address.
> 
> too expensive, unnecessary runtime overhead

Indeed.

> >
> >  - (Cached method) when making link->addrs, make a link->adj_addrs
> >    array too, where adj_addrs[i] == ftrace_call_adjust(addrs[i]).
> >
> > Let me try the 3rd one. It may consume more memory but the
> > fastest solution.
> 
> I like the third one as well, but I'm not following why we'd need more memory.
> 
> We can store adjusted addresses in existing link->addrs, and we'll
> need to adjust them to originals (potentially expensively) only in
> bpf_kprobe_multi_link_fill_link_info(). But that's not a hot path, so
> it doesn't matter.

OK, that works well, but let me check it has any side effects.

BTW, I worry about what the user expects for `bpf_get_func_ip()`.

 * u64 bpf_get_func_ip(void *ctx)
 * 	Description
 * 		Get address of the traced function (for tracing and kprobe programs).
 *
 * 		When called for kprobe program attached as uprobe it returns
 * 		probe address for both entry and return uprobe.
 *
 * 	Return
 * 		Address of the traced function for kprobe.
 * 		0 for kprobes placed within the function (not at the entry).
 * 		Address of the probe for uprobe and return uprobe.

This seems expecting to get the function address, not ftrace entry
address. If user expects it returns actual function entry address,
we need to change `get_entry_ip()`(*) as the patch I sent.

If they can accept the address a bit shifted from the entry address,
I think we can change link->addrs.

Jiri, and other BPF users, what you expect and what you want to
have? (what the reason to use this API?)

Thank you,

(*) bpf_get_func_ip() seems to read `run_ctx->entry_ip` which is
set by `get_entry_ip(fentry_ip)`.

> 
> >
> > Thank you,
> >
> > >
> > > which is then checked as
> > >
> > > if ((const void *) addr == &bpf_testmod_fentry_test3)
> > >
> > >
> > > With your patches this doesn't match anymore.
> >
> > It actually enables the fprobe on those architectures, so
> > without my series, those test should be skipped.
> 
> Ok, cool, still, let's fix the issue.
> 
> >
> > Thank you,
> >
> > >
> > >
> > > Hopefully the above gives you some pointers, let me know if you run
> > > into any problems.
> > >
> > > >
> > > > Thank you,
> > > >
> > > > >
> > > > > >
> > > > > > Please note: this email is coming from an unmonitored mailbox. If you have
> > > > > > questions or feedback, please reach out to the Meta Kernel CI team at
> > > > > > kernel-ci@meta.com.
> > > >
> > > >
> > > > --
> > > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>


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