Re: [PATCHv2 bpf-next 1/9] uprobe: Add support for session consumer

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Jul 03, 2024 at 02:48:28PM -0700, Andrii Nakryiko wrote:
> On Wed, Jul 3, 2024 at 10:13 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Tue, Jul 02, 2024 at 01:51:28PM -0700, Andrii Nakryiko wrote:
> >
> > SNIP
> >
> > > >  #ifdef CONFIG_UPROBES
> > > > @@ -80,6 +83,12 @@ struct uprobe_task {
> > > >         unsigned int                    depth;
> > > >  };
> > > >
> > > > +struct session_consumer {
> > > > +       __u64           cookie;
> > > > +       unsigned int    id;
> > > > +       int             rc;
> > >
> > > you'll be using u64 for ID, right? so this struct will be 24 bytes.
> >
> > yes
> >
> > > Maybe we can just use topmost bit of ID to store whether uretprobe
> > > should run or not? It's trivial to mask out during ID comparisons
> >
> > actually.. I think we could store just consumers that need to be
> > executed in return probe so there will be no need for 'rc' value
> 
> ah, nice idea. NULL would mean we have session uprobe, but for this
> particular run we "disabled" uretprobe part of it. Great. And for
> non-session uprobes we just won't have session_consumer at all, right?

hm, I think we don't need to add both session or non-session consumer
if it's not supposed to run.. let's see

> 
> [...]
> 
> > > > +static struct session_consumer *
> > > > +session_consumer_next(struct return_instance *ri, struct session_consumer *sc,
> > > > +                     int session_id)
> > > > +{
> > > > +       struct session_consumer *next;
> > > > +
> > > > +       next = sc ? sc + 1 : &ri->sessions[0];
> > > > +       next->id = session_id;
> > >
> > > it's kind of unexpected that "session_consumer_next" would actually
> > > set an ID... Maybe drop int session_id as input argument and fill it
> > > out outside of this function, this function being just a simple
> > > iterator?
> >
> > yea, I was going back and forth on what to have in that function
> > or not, to keep the change minimal, but makes sense, will move
> >
> 
> great, thanks
> 
> > >
> > > > +       return next;
> > > > +}
> > > > +
> 
> [...]
> 
> > >
> > > > +               } else if (uc->ret_handler) {
> > > >                         need_prep = true;
> > > > +               }
> > > >
> > > >                 remove &= rc;
> > > >         }
> > > >
> > > > +       /* no removal if there's at least one session consumer */
> > > > +       remove &= !uprobe->sessions_cnt;
> > >
> > > this is counter (not error, not pointer), let's stick to ` == 0`, please
> > >
> > > is this
> > >
> > > if (uprobe->sessions_cnt != 0)
> > >    remove = 0;
> >
> > yes ;-) will change
> >
> 
> Thanks, I feel bad for being the only one to call this out, but I find
> all these '!<some_integer_variable>` constructs extremely unintuitive
> and hard to reason about quickly. It's only pointers and error cases
> that are more or less intuitive. Everything else, including
> !strcmp(...) is just mind bending and exhausting... Perhaps I'm just
> not a kernel engineer enough :)

heh I was going for minimal change.. but it's intrusive enough already,
so let's keep it at least readable

jirka