Re: [PATCHv5 bpf-next 02/13] uprobe: Add support for session consumer

[Andrii Nakryiko]

On Tue, Oct 1, 2024 at 6:17 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Mon, Sep 30, 2024 at 02:36:03PM -0700, Andrii Nakryiko wrote:
> > On Sun, Sep 29, 2024 at 1:57 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > >
> > > This change allows the uprobe consumer to behave as session which
> > > means that 'handler' and 'ret_handler' callbacks are connected in
> > > a way that allows to:
> > >
> > >   - control execution of 'ret_handler' from 'handler' callback
> > >   - share data between 'handler' and 'ret_handler' callbacks
> > >
> > > The session concept fits to our common use case where we do filtering
> > > on entry uprobe and based on the result we decide to run the return
> > > uprobe (or not).
> > >
> > > It's also convenient to share the data between session callbacks.
> > >
> > > To achive this we are adding new return value the uprobe consumer
> > > can return from 'handler' callback:
> > >
> > >   UPROBE_HANDLER_IGNORE
> > >   - Ignore 'ret_handler' callback for this consumer.
> > >
> > > And store cookie and pass it to 'ret_handler' when consumer has both
> > > 'handler' and 'ret_handler' callbacks defined.
> > >
> > > We store shared data in the return_consumer object array as part of
> > > the return_instance object. This way the handle_uretprobe_chain can
> > > find related return_consumer and its shared data.
> > >
> > > We also store entry handler return value, for cases when there are
> > > multiple consumers on single uprobe and some of them are ignored and
> > > some of them not, in which case the return probe gets installed and
> > > we need to have a way to find out which consumer needs to be ignored.
> > >
> > > The tricky part is when consumer is registered 'after' the uprobe
> > > entry handler is hit. In such case this consumer's 'ret_handler' gets
> > > executed as well, but it won't have the proper data pointer set,
> > > so we can filter it out.
> > >
> > > Suggested-by: Oleg Nesterov <oleg@xxxxxxxxxx>
> > > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > ---
> > >  include/linux/uprobes.h |  21 +++++-
> > >  kernel/events/uprobes.c | 148 +++++++++++++++++++++++++++++++---------
> > >  2 files changed, 137 insertions(+), 32 deletions(-)
> > >
> >
> > LGTM,
> >
> > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >
> >
> > Note also that I just resent the last patch from my patch set ([0]),
> > hopefully it will get applied, in which case you'd need to do a tiny
> > rebase.
> >
> >   [0] https://lore.kernel.org/linux-trace-kernel/20240930212246.1829395-1-andrii@xxxxxxxxxx/
>
> the rebase is fine, but what I'm not clear about is that after yours and
> Oleg's changes get in, my kernel changes will depend on peter's perf/core,
> but bpf selftests changes will need bpf-next/master

Yep, and I was waiting for your next revision to discuss logistics,
but perhaps we could do it right here.

I think uprobe parts should stay in tip/perf/core (if that's where all
uprobe code goes in), as we have a bunch of ongoing work that all will
conflict a bit with each other, if it lands across multiple trees.

So that means that patches #1 and #2 ideally land in tip/perf/core.
But you have a lot of BPF-specific things that would be inconvenient
to route through tip, so I'd say those should go through bpf-next.

What we can do, if Ingo and Peter are OK with that, is to create a
stable (non-rebaseable) branch off of your first two patches (applied
in tip/perf/core), which we'll merge into bpf-next/master and land the
rest of your patch set there. We've done that with recent struct fd
changes, and there were few other similar cases in the past, and that
all worked well.

Peter, Ingo, are you guys OK with that approach?

>
> jirka