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?
[...]
> > > +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 :)
> jirka
>
> >
> > ? I can't tell (honestly), without spending ridiculous amounts of
> > mental resources (for the underlying simplicity of the condition).
>
> SNIP
