Re: [RFC/PATCH bpf-next 00/20] bpf: Add multi uprobe link

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Apr 27, 2023 at 03:24:25PM -0700, Andrii Nakryiko wrote:
> On Thu, Apr 27, 2023 at 5:44 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Apr 26, 2023 at 12:09:59PM -0700, Andrii Nakryiko wrote:
> > > On Mon, Apr 24, 2023 at 9:04 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > hi,
> > > > this patchset is adding support to attach multiple uprobes and usdt probes
> > > > through new uprobe_multi link.
> > > >
> > > > The current uprobe is attached through the perf event and attaching many
> > > > uprobes takes a lot of time because of that.
> > > >
> > > > The main reason is that we need to install perf event for each probed function
> > > > and profile shows perf event installation (perf_install_in_context) as culprit.
> > > >
> > > > The new uprobe_multi link just creates raw uprobes and attaches the bpf
> > > > program to them without perf event being involved.
> > > >
> > > > In addition to being faster we also save file descriptors. For the current
> > > > uprobe attach we use extra perf event fd for each probed function. The new
> > > > link just need one fd that covers all the functions we are attaching to.
> > >
> > > All of the above are good reasons and thanks for tackling multi-uprobe!
> > >
> > > >
> > > > By dropping perf we lose the ability to attach uprobe to specific pid.
> > > > We can workaround that by having pid check directly in the bpf program,
> > > > but we might need to check for another solution if that will turn out
> > > > to be a problem.
> > > >
> > >
> > > I think this is a big deal, because it makes multi-uprobe not a
> > > drop-in replacement for normal uprobes even for typical scenarios. It
> > > might be why you couldn't do transparent use of uprobe.multi in USDT?
> >
> > yes
> >
> > >
> > > But I'm not sure why this is a problem? How does perf handle this?
> > > Does it do runtime filtering or something more efficient that prevents
> > > uprobe to be triggered for other PIDs in the first place? If it's the
> > > former, then why can't we do the same simple check ourselves if pid
> > > filter is specified?
> >
> > so the standard uprobe is basically a perf event and as such it can be
> > created with 'pid' as a target.. and such perf event will get installed
> > only when the process with that pid is scheduled in and uninstalled
> > when it's scheduled out
> >
> > >
> > > I also see that uprobe_consumer has filter callback, not sure if it's
> > > a better solution just for pid filtering, but might be another way to
> > > do this?
> >
> > yes, that's probably how we will have to do that, will check
> 
> callback seems like overkill as we'll be paying indirect call price.
> So a simple if statement in either uprobe_prog_run or in
> uprobe_multi_link_ret_handler/uprobe_multi_link_handler seems like
> better solution, IMO.

it looks like the consumer->filter is checked/executed before installing
the breakpoint for uprobe, so it could be actually faster than current
uprobe pid filter.. I'll check and have it there in next version

> 
> 
> >
> > >
> > > Another aspect I wanted to discuss (and I don't know the right answer)
> > > was whether we need to support separate binary path for each offset?
> > > It would simplify (and trim down memory usage significantly) a bunch
> > > of internals if we knew we are dealing with single inode for each
> > > multi-uprobe link. I'm trying to think if it would be limiting in
> > > practice to have to create link per each binary, and so far it seems
> > > like usually user-space code will do symbol resolution per ELF file
> > > anyways, so doesn't seem limiting to have single path + multiple
> > > offsets/cookies within that file. For USDTs use case even ref_ctr is
> > > probably the same, but I'd keep it 1:1 with offset and cookie anyways.
> > > For uniformity and generality.
> > >
> > > WDYT?
> >
> > right, it's waste for single binary, but I guess it's not a big waste,
> > because when you have single binary you just repeat the same pointer,
> > not the path
> >
> > it's fast enough to be called multiple times for each binary you want
> > to trace, but it'd be also nice to be able to attach all in once ;-)
> >
> > maybe we could have a bit in flags saying paths[0] is valid for all
> 
> No need for extra flags. I was just thinking about having a simpler
> and more straightforward API, where you don't need to create another
> array with tons of duplicated string pointers. No big deal, I'm fine
> either way.

ok

thanks,
jirka