On Mon, Aug 5, 2024 at 6:44 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > On 07/31, Andrii Nakryiko wrote: > > > > @@ -732,11 +776,13 @@ static struct uprobe *alloc_uprobe(struct inode *inode, loff_t offset, > > uprobe->ref_ctr_offset = ref_ctr_offset; > > init_rwsem(&uprobe->register_rwsem); > > init_rwsem(&uprobe->consumer_rwsem); > > + RB_CLEAR_NODE(&uprobe->rb_node); > > I guess RB_CLEAR_NODE() is not necessary? I definitely needed that with my batch API changes, but it might be that I don't need it anymore. But I'm a bit hesitant to remove it, because if we ever get put_uprobe() on an uprobe that hasn't been inserted into RB-tree yet, this will cause a hard to understand crash. RB_CLEAR_NODE() in __insert_uprobe() is critical to have, this one is kind of optional (but still feels right to initialize the field properly). Let me know if you feel strongly about this, though. > > > @@ -1286,15 +1296,19 @@ static void build_probe_list(struct inode *inode, > > u = rb_entry(t, struct uprobe, rb_node); > > if (u->inode != inode || u->offset < min) > > break; > > + u = try_get_uprobe(u); > > + if (!u) /* uprobe already went away, safe to ignore */ > > + continue; > > list_add(&u->pending_list, head); > > cosmetic nit, feel to ignore, but to me > > if (try_get_uprobe(u)) > list_add(&u->pending_list, head); > > looks more readable. It's not my code base to enforce my preferences, but I'll at least explain why I disagree. To me, something like `if (some condition) <break/continue>;` is a very clear indication that this item (or even the rest of items in case of break) won't be processed anymore. While if (some inverted condition) <do some something useful> <might be some more code> ... is a pattern that requires double-checking that we really are not going to use that item later on. So I'll invert this just to not be PITA, but I disagree :) > > Other than the lack of kfree() in put_uprobe() and WARN() in _unregister() > the patch looks good to me. yep, fixed that locally already. Thanks for the review! > > Oleg. >
