Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

[Oleg Nesterov]

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.

> 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.

> 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.

> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

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".

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

Yes.

Oleg.