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@kernel.org>

Thanks!

quoted

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