Messages in this thread | | | Subject | Re: [PATCH v5 3.1.0-rc4-tip 4/26] uprobes: Define hooks for mmap/munmap. | From | Peter Zijlstra <> | Date | Tue, 27 Sep 2011 13:41:21 +0200 |
| |
On Mon, 2011-09-26 at 21:14 +0530, Srikar Dronamraju wrote: > > Why not something like: > > > > > > +static struct uprobe *__find_uprobe(struct inode * inode, loff_t offset, > > bool inode_only) > > +{ > > struct uprobe u = { .inode = inode, .offset = inode_only ? 0 : offset }; > > + struct rb_node *n = uprobes_tree.rb_node; > > + struct uprobe *uprobe; > > struct uprobe *ret = NULL; > > + int match; > > + > > + while (n) { > > + uprobe = rb_entry(n, struct uprobe, rb_node); > > + match = match_uprobe(&u, uprobe); > > + if (!match) { > > if (!inode_only) > > atomic_inc(&uprobe->ref); > > + return uprobe; > > + } > > if (inode_only && uprobe->inode == inode) > > ret = uprobe; > > + if (match < 0) > > + n = n->rb_left; > > + else > > + n = n->rb_right; > > + > > + } > > return ret; > > +} > > > > I am not comfortable with this change. > find_uprobe() was suppose to return back a uprobe if and only if > the inode and offset match,
And it will, because find_uprobe() will never expose that third argument.
> However with your approach, we end up > returning a uprobe that isnt matching and one that isnt refcounted. > Moreover if even if we have a matching uprobe, we end up sending a > unrefcounted uprobe back.
Because the matching isn't the important part, you want to return the leftmost node matching the specified inode. Also, in that case you explicitly don't want the ref, since the first thing you do on the call-site is drop the ref if there was a match. You don't care about inode:0 in particular, you want a place to start iterating all of inode:*.
| |