On Tue, Jul 30, 2024 at 5:35 AM Oleg Nesterov <[email protected]> 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 <[email protected]> > --- > 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 <[email protected]> > 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 >
