Messages in this thread
 Date Wed, 8 Jun 2011 00:12:17 -0400 From Stephen Wilson <> Subject Re: [PATCH v4 3.0-rc2-tip 3/22] 3: uprobes: Adding and remove a uprobe in a rb tree.
`Hi Srikar,On Tue, Jun 07, 2011 at 06:28:50PM +0530, Srikar Dronamraju wrote:> +/* Called with uprobes_treelock held */> +static struct uprobe *__find_uprobe(struct inode * inode,> +			 loff_t offset, struct rb_node **close_match)> +{> +	struct uprobe r = { .inode = inode, .offset = offset };> +	struct rb_node *n = uprobes_tree.rb_node;> +	struct uprobe *uprobe;> +	int match, match_inode;> +> +	while (n) {> +		uprobe = rb_entry(n, struct uprobe, rb_node);> +		match = match_uprobe(uprobe, &r, &match_inode);> +		if (close_match && match_inode)> +			*close_match = n;> +> +		if (!match) {> +			atomic_inc(&uprobe->ref);> +			return uprobe;> +		}> +		if (match < 0)> +			n = n->rb_left;> +		else> +			n = n->rb_right;> +> +	}> +	return NULL;> +}> +I think there is a simple mistake in the search logic here.  In particular, Ithink the arguments to match_uprobe() should be swapped to give:	match = match_uprobe(&r, uprobe, NULL)Otherwise, when we do not have an exact match, the next node to be consideredis the left child of 'uprobe' even though 'uprobe' is "smaller" than r (andvice versa for the "larger" case).> +static struct uprobe *__insert_uprobe(struct uprobe *uprobe)> +{> +	struct rb_node **p = &uprobes_tree.rb_node;> +	struct rb_node *parent = NULL;> +	struct uprobe *u;> +	int match;> +> +	while (*p) {> +		parent = *p;> +		u = rb_entry(parent, struct uprobe, rb_node);> +		match = match_uprobe(u, uprobe, NULL);> +		if (!match) {> +			atomic_inc(&u->ref);> +			return u;> +		}> +> +		if (match < 0)> +			p = &parent->rb_left;> +		else> +			p = &parent->rb_right;> +> +	}I think the match_uprobe() arguments should be swapped here as well forsimilar reasons as above.Also, changing the argument order seems to solve the issue reported byJosh Stone where only the uprobe with the lowest address was responding(thou I did not test with perf, just lightly with the trace_eventinterface).  In particular, iteration using rb_next() appears to work asexpected, thus allowing all breakpoints to be registered inmmap_uprobe().> +	u = NULL;> +	rb_link_node(&uprobe->rb_node, parent, p);> +	rb_insert_color(&uprobe->rb_node, &uprobes_tree);> +	/* get access + drop ref */> +	atomic_set(&uprobe->ref, 2);> +	return u;> +}-- steve`

Last update: 2011-06-08 06:17    [W:0.382 / U:0.880 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site