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@xxxxxxxxx> 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
