| Message ID | 20240730123457.GA9108@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 12026d2034dfeb575e0eb28f33431cbf03dc732c |
| Delegated to: | Masami Hiramatsu |
| Headers | show |
| Series | uprobes: simplify _unregister paths | expand |
On Tue, Jul 30, 2024 at 5:35 AM Oleg Nesterov <oleg@redhat.com> wrote: > > Kill the extra get_uprobe() + put_uprobe() in uprobe_unregister() and > move the possibly final put_uprobe() from delete_uprobe() to its only > caller, uprobe_unregister(). > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > kernel/events/uprobes.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > LGTM as well. 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. I can do that with a simple refcount_t and refcount_inc_not_zero(), no fancy custom refcounting. This schema makes most sense to me, it will also simplify registration by avoiding that annoying is_uprobe_active() check + retry. Thoughts? All that would be additional on top of your change. 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 :) 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! Acked-by: Andrii Nakryiko <andrii@kernel.org> > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index c06e1a5f1783..f88b7ff20587 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -939,7 +939,6 @@ static void delete_uprobe(struct uprobe *uprobe) > rb_erase(&uprobe->rb_node, &uprobes_tree); > write_unlock(&uprobes_treelock); > RB_CLEAR_NODE(&uprobe->rb_node); /* for uprobe_is_active() */ > - put_uprobe(uprobe); > } > > struct map_info { > @@ -1094,7 +1093,6 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) > { > int err; > > - get_uprobe(uprobe); > down_write(&uprobe->register_rwsem); > if (WARN_ON(!consumer_del(uprobe, uc))) > err = -ENOENT; > @@ -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. With my proposed change to refcounting schema this would be unnecessary, but let's see what you think. > + } > up_write(&uprobe->register_rwsem); > - put_uprobe(uprobe); > + > + if (!err) > + put_uprobe(uprobe); > } > EXPORT_SYMBOL_GPL(uprobe_unregister); > > -- > 2.25.1.362.g51ebf55 >
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! > > @@ -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.
On Tue, Jul 30, 2024 at 10:17 AM Oleg Nesterov <oleg@redhat.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@kernel.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. >
On Tue, Jul 30, 2024 at 08:08:49AM -0700, Andrii Nakryiko wrote: > 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! I can stick then in tip/perf/core if you want.
On Tue, Jul 30, 2024 at 1:55 PM Peter Zijlstra <peterz@infradead.org> wrote: > > On Tue, Jul 30, 2024 at 08:08:49AM -0700, Andrii Nakryiko wrote: > > 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! > > I can stick then in tip/perf/core if you want. Whichever tree you and Masami ultimately agreed on, I don't particularly care. But yeah, this would be great, thanks!
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index c06e1a5f1783..f88b7ff20587 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -939,7 +939,6 @@ static void delete_uprobe(struct uprobe *uprobe) rb_erase(&uprobe->rb_node, &uprobes_tree); write_unlock(&uprobes_treelock); RB_CLEAR_NODE(&uprobe->rb_node); /* for uprobe_is_active() */ - put_uprobe(uprobe); } struct map_info { @@ -1094,7 +1093,6 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc) { int err; - get_uprobe(uprobe); down_write(&uprobe->register_rwsem); if (WARN_ON(!consumer_del(uprobe, uc))) err = -ENOENT; @@ -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; + } up_write(&uprobe->register_rwsem); - put_uprobe(uprobe); + + if (!err) + put_uprobe(uprobe); } EXPORT_SYMBOL_GPL(uprobe_unregister);
Kill the extra get_uprobe() + put_uprobe() in uprobe_unregister() and move the possibly final put_uprobe() from delete_uprobe() to its only caller, uprobe_unregister(). Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- kernel/events/uprobes.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)