Message-ID: <CAEf4BzYiBgqJwmh=rJiL+5gfwbZprEp81wnmq219w5j5WEGAFg@mail.gmail.com>
Date: Tue, 30 Jul 2024 10:27:01 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: andrii@...nel.org, mhiramat@...nel.org, peterz@...radead.org, 
	jolsa@...nel.org, rostedt@...dmis.org, linux-kernel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

On Tue, Jul 30, 2024 at 10:17 AM Oleg Nesterov <oleg@...hat.com> wrote:
>
> Thanks for looking at this!
>
> On 07/30, Andrii Nakryiko wrote:
> >
> > BTW, do you have anything against me changing refcounting so that
> > uprobes_tree itself doesn't hold a refcount, and all the refcounting
> > is done based on consumers holding implicit refcount and whatever
> > temporary get/put uprobe is necessary for runtime uprobe/uretprobe
> > functioning.
>
> No, I have nothing against.
>
> To be honest, I don't really understand the value of this change, but
> a) this is probably because I didn't see a separate patch(es) which
> does this and b) assuming that it doesn't complicate the code too much
> I won't argue in any case ;)
>
> And in fact I had your proposed change in mind when I did these cleanups.
> I think that they can even simplify this change, at least I hope they can
> not complicate it.

I just find this logic to drop refcount if the consumer list is empty
super confusing and hard to intuitively reason about. It's just very
unconventional and seems problematic every time I think about this.

Either way, we can discuss once you see the code, it's not really
complicated if I stick to refcount_t instead of my fancy atomic-based
refcounting.

>
> > BTW, do you plan
> > any more clean ups like this? It's a bit of a moving target because of
> > your refactoring, so I'd appreciate some stability to build upon :)
>
> Well yes... may be this week.
>
> I'd like to (try to) optimize/de-uglify register_for_each_vma() for
> the multiple-consumers case. And, more importantly, the case when perf
> does uprobe_register() + uprobe_apply().
>
> But. All these changes will only touch the register_for_each_vma() paths,
> so this is completely orthogonal to this series and your and/or Peter's
> changes.
>

Ok, sounds good, it would be nice to keep the other part more or less
intact for the time being. Thanks!

> > Also, can you please push this and your previous patch set into some
> > branch somewhere I can pull from, preferably based on the latest
> > linux-trace's probes/for-next? Thanks!
>
> Cough ;)
>
> No, sorry, I can't. I lost my kernel.org account years ago (and this is
> the second time this has happened!), but since I am a lazy dog I didn't
> even bother to try to restore it.

It doesn't have to be kernel.org repo :-P Github would work just fine,
but ok, if it's too much trouble I'll just go download emails one by
one and apply them locally.

>
> > Acked-by: Andrii Nakryiko <andrii@...nel.org>
>
> Thanks!
>
> > > @@ -1102,10 +1100,16 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> > >                 err = register_for_each_vma(uprobe, NULL);
> > >
> > >         /* TODO : cant unregister? schedule a worker thread */
> > > -       if (!err && !uprobe->consumers)
> > > -               delete_uprobe(uprobe);
> > > +       if (!err) {
> > > +               if (!uprobe->consumers)
> > > +                       delete_uprobe(uprobe);
> > > +               else
> > > +                       err = -EBUSY;
> >
> > This bit is weird because really it's not an error... but this makes
> > this change simpler and moves put_uprobe outside of rwsem.
>
> Agreed, uprobe->consumers != NULL is not an error. But we don't return
> this error code, just we need to ensure that "err == 0" means that
> "delete_uprobe() was actually called".
>

yep

> > With my
> > proposed change to refcounting schema this would be unnecessary,
>
> Yes.
>
> Oleg.
>

Powered by blists - more mailing lists