On 08/05, Andrii Nakryiko wrote: > > 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, OK, lets keep it, it doesn't hurt. Just it wasn't clear to me why did you add this initialization in this patch. > > > @@ -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> OK, I won't insist. To me the most confusing part is u = try_get_uprobe(u); if (!u) ... If you read this code for the 1st time (or you are trying to recall it after 10 years ;) it looks as if try_get_uprobe() can return another uprobe. > So I'll invert this just to not be PITA, but I disagree :) If you disagree, then don't change it ;) Oleg.
