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 <[email protected]> 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.
