rbtree: Undo augmented trees performance damage and regression

Reimplement augmented RB-trees without sprinkling extra branches all over the RB-tree code (which lives in the scheduler hot path). This approach is 'borrowed' from Fabio's BFQ implementation and relies on traversing the rebalance path after the RB-tree-op to correct the heap property for insertion/removal and make up for the damage done by the tree rotations. For insertion the rebalance path is trivially that from the new node upwards to the root, for removal it is that from the deepest node in the path from the to be removed node that will still be around after the removal. [ This patch also fixes a video driver regression reported by Ali Gholami Rudi - the memtype->subtree_max_end was updated incorrectly. ] Acked-by: Suresh Siddha <suresh.b.siddha@intel.com> Acked-by: Venkatesh Pallipadi <venki@google.com> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Tested-by: Ali Gholami Rudi <ali@rudi.ir> Cc: Fabio Checconi <fabio@gandalf.sssup.it> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> LKML-Reference: <1275414172.27810.27961.camel@twins> Signed-off-by: Ingo Molnar <mingo@elte.hu>

rbtree: Undo augmented trees performance damage and regression
Reimplement augmented RB-trees without sprinkling extra branches all over the RB-tree code (which lives in the scheduler hot path). This approach is 'borrowed' from Fabio's BFQ implementation and relies on traversing the rebalance path after the RB-tree-op to correct the heap property for insertion/removal and make up for the damage done by the tree rotations. For insertion the rebalance path is trivially that from the new node upwards to the root, for removal it is that from the deepest node in the path from the to be removed node that will still be around after the removal. [ This patch also fixes a video driver regression reported by Ali Gholami Rudi - the memtype->subtree_max_end was updated incorrectly. ] Acked-by: Suresh Siddha <suresh.b.siddha@intel.com> Acked-by: Venkatesh Pallipadi <venki@google.com> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Tested-by: Ali Gholami Rudi <ali@rudi.ir> Cc: Fabio Checconi <fabio@gandalf.sssup.it> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> LKML-Reference: <1275414172.27810.27961.camel@twins> Signed-off-by: Ingo Molnar <mingo@elte.hu>
Peter Zijlstra · Ingo Molnar
1 parent d596043d71
Showing 3 changed files with 87 additions and 76 deletions Side-by-side Diff
arch/x86/mm/pat_rbtree.c
include/linux/rbtree.h
lib/rbtree.c
@@ -34,8 +34,7 @@
  * memtype_lock protects the rbtree.
  */
  
-static void memtype_rb_augment_cb(struct rb_node *node);
-static struct rb_root memtype_rbroot = RB_AUGMENT_ROOT(&memtype_rb_augment_cb);
+static struct rb_root memtype_rbroot = RB_ROOT;
  
 static int is_node_overlap(struct memtype *node, u64 start, u64 end)
 {
@@ -56,7 +55,7 @@
 }
  
 /* Update 'subtree_max_end' for a node, based on node and its children */
-static void update_node_max_end(struct rb_node *node)
+static void memtype_rb_augment_cb(struct rb_node *node, void *__unused)
 {
 	struct memtype *data;
 	u64 max_end, child_max_end;
@@ -78,25 +77,6 @@
 	data->subtree_max_end = max_end;
 }
  
-/* Update 'subtree_max_end' for a node and all its ancestors */
-static void update_path_max_end(struct rb_node *node)
-{
-	u64 old_max_end, new_max_end;
-
-	while (node) {
-		struct memtype *data = container_of(node, struct memtype, rb);
-
-		old_max_end = data->subtree_max_end;
-		update_node_max_end(node);
-		new_max_end = data->subtree_max_end;
-
-		if (new_max_end == old_max_end)
-			break;
-
-		node = rb_parent(node);
-	}
-}
-
 /* Find the first (lowest start addr) overlapping range from rb tree */
 static struct memtype *memtype_rb_lowest_match(struct rb_root *root,
 				u64 start, u64 end)
@@ -190,12 +170,6 @@
 	return -EBUSY;
 }
  
-static void memtype_rb_augment_cb(struct rb_node *node)
-{
-	if (node)
-		update_path_max_end(node);
-}
-
 static void memtype_rb_insert(struct rb_root *root, struct memtype *newdata)
 {
 	struct rb_node **node = &(root->rb_node);
@@ -213,6 +187,7 @@
  
 	rb_link_node(&newdata->rb, parent, node);
 	rb_insert_color(&newdata->rb, root);
+	rb_augment_insert(&newdata->rb, memtype_rb_augment_cb, NULL);
 }
  
 int rbt_memtype_check_insert(struct memtype *new, unsigned long *ret_type)
  
  
@@ -234,13 +209,16 @@
  
 struct memtype *rbt_memtype_erase(u64 start, u64 end)
 {
+	struct rb_node *deepest;
 	struct memtype *data;
  
 	data = memtype_rb_exact_match(&memtype_rbroot, start, end);
 	if (!data)
 		goto out;
  
+	deepest = rb_augment_erase_begin(&data->rb);
 	rb_erase(&data->rb, &memtype_rbroot);
+	rb_augment_erase_end(deepest, memtype_rb_augment_cb, NULL);
 out:
 	return data;
 }
@@ -110,7 +110,6 @@
 struct rb_root
 {
 	struct rb_node *rb_node;
-	void (*augment_cb)(struct rb_node *node);
 };
  
  
@@ -130,9 +129,7 @@
 	rb->rb_parent_color = (rb->rb_parent_color & ~1) | color;
 }
  
-#define RB_ROOT	(struct rb_root) { NULL, NULL, }
-#define RB_AUGMENT_ROOT(x)	(struct rb_root) { NULL, x}
-
+#define RB_ROOT	(struct rb_root) { NULL, }
 #define	rb_entry(ptr, type, member) container_of(ptr, type, member)
  
 #define RB_EMPTY_ROOT(root)	((root)->rb_node == NULL)
@@ -141,6 +138,14 @@
  
 extern void rb_insert_color(struct rb_node *, struct rb_root *);
 extern void rb_erase(struct rb_node *, struct rb_root *);
+
+typedef void (*rb_augment_f)(struct rb_node *node, void *data);
+
+extern void rb_augment_insert(struct rb_node *node,
+			      rb_augment_f func, void *data);
+extern struct rb_node *rb_augment_erase_begin(struct rb_node *node);
+extern void rb_augment_erase_end(struct rb_node *node,
+				 rb_augment_f func, void *data);
  
 /* Find logical next and previous nodes in a tree */
 extern struct rb_node *rb_next(const struct rb_node *);
@@ -44,11 +44,6 @@
 	else
 		root->rb_node = right;
 	rb_set_parent(node, right);
-
-	if (root->augment_cb) {
-		root->augment_cb(node);
-		root->augment_cb(right);
-	}
 }
  
 static void __rb_rotate_right(struct rb_node *node, struct rb_root *root)
  
@@ -72,20 +67,12 @@
 	else
 		root->rb_node = left;
 	rb_set_parent(node, left);
-
-	if (root->augment_cb) {
-		root->augment_cb(node);
-		root->augment_cb(left);
-	}
 }
  
 void rb_insert_color(struct rb_node *node, struct rb_root *root)
 {
 	struct rb_node *parent, *gparent;
  
-	if (root->augment_cb)
-		root->augment_cb(node);
-
 	while ((parent = rb_parent(node)) && rb_is_red(parent))
 	{
 		gparent = rb_parent(parent);
  
@@ -240,15 +227,12 @@
 	else
 	{
 		struct rb_node *old = node, *left;
-		int old_parent_cb = 0;
-		int successor_parent_cb = 0;
  
 		node = node->rb_right;
 		while ((left = node->rb_left) != NULL)
 			node = left;
  
 		if (rb_parent(old)) {
-			old_parent_cb = 1;
 			if (rb_parent(old)->rb_left == old)
 				rb_parent(old)->rb_left = node;
 			else
  
@@ -263,10 +247,8 @@
 		if (parent == old) {
 			parent = node;
 		} else {
-			successor_parent_cb = 1;
 			if (child)
 				rb_set_parent(child, parent);
-
 			parent->rb_left = child;
  
 			node->rb_right = old->rb_right;
@@ -277,24 +259,6 @@
 		node->rb_left = old->rb_left;
 		rb_set_parent(old->rb_left, node);
  
-		if (root->augment_cb) {
-			/*
-			 * Here, three different nodes can have new children.
-			 * The parent of the successor node that was selected
-			 * to replace the node to be erased.
-			 * The node that is getting erased and is now replaced
-			 * by its successor.
-			 * The parent of the node getting erased-replaced.
-			 */
-			if (successor_parent_cb)
-				root->augment_cb(parent);
-
-			root->augment_cb(node);
-
-			if (old_parent_cb)
-				root->augment_cb(rb_parent(old));
-		}
-
 		goto color;
 	}
  
  
  
  
@@ -303,25 +267,89 @@
  
 	if (child)
 		rb_set_parent(child, parent);
-
-	if (parent) {
+	if (parent)
+	{
 		if (parent->rb_left == node)
 			parent->rb_left = child;
 		else
 			parent->rb_right = child;
-
-		if (root->augment_cb)
-			root->augment_cb(parent);
-
-	} else {
-		root->rb_node = child;
 	}
+	else
+		root->rb_node = child;
  
  color:
 	if (color == RB_BLACK)
 		__rb_erase_color(child, parent, root);
 }
 EXPORT_SYMBOL(rb_erase);
+
+static void rb_augment_path(struct rb_node *node, rb_augment_f func, void *data)
+{
+	struct rb_node *parent;
+
+up:
+	func(node, data);
+	parent = rb_parent(node);
+	if (!parent)
+		return;
+
+	if (node == parent->rb_left && parent->rb_right)
+		func(parent->rb_right, data);
+	else if (parent->rb_left)
+		func(parent->rb_left, data);
+
+	node = parent;
+	goto up;
+}
+
+/*
+ * after inserting @node into the tree, update the tree to account for
+ * both the new entry and any damage done by rebalance
+ */
+void rb_augment_insert(struct rb_node *node, rb_augment_f func, void *data)
+{
+	if (node->rb_left)
+		node = node->rb_left;
+	else if (node->rb_right)
+		node = node->rb_right;
+
+	rb_augment_path(node, func, data);
+}
+
+/*
+ * before removing the node, find the deepest node on the rebalance path
+ * that will still be there after @node gets removed
+ */
+struct rb_node *rb_augment_erase_begin(struct rb_node *node)
+{
+	struct rb_node *deepest;
+
+	if (!node->rb_right && !node->rb_left)
+		deepest = rb_parent(node);
+	else if (!node->rb_right)
+		deepest = node->rb_left;
+	else if (!node->rb_left)
+		deepest = node->rb_right;
+	else {
+		deepest = rb_next(node);
+		if (deepest->rb_right)
+			deepest = deepest->rb_right;
+		else if (rb_parent(deepest) != node)
+			deepest = rb_parent(deepest);
+	}
+
+	return deepest;
+}
+
+/*
+ * after removal, update the tree to account for the removed entry
+ * and any rebalance damage.
+ */
+void rb_augment_erase_end(struct rb_node *node, rb_augment_f func, void *data)
+{
+	if (node)
+		rb_augment_path(node, func, data);
+}
  
 /*
  * This function returns the first node (in sort order) of the tree.
...	...	@@ -34,8 +34,7 @@
34	34	* memtype_lock protects the rbtree.
35	35	*/
36	36
37		-static void memtype_rb_augment_cb(struct rb_node *node);
38		-static struct rb_root memtype_rbroot = RB_AUGMENT_ROOT(&memtype_rb_augment_cb);
	37	+static struct rb_root memtype_rbroot = RB_ROOT;
39	38
40	39	static int is_node_overlap(struct memtype *node, u64 start, u64 end)
41	40	{
...	...	@@ -56,7 +55,7 @@
56	55	}
57	56
58	57	/* Update 'subtree_max_end' for a node, based on node and its children */
59		-static void update_node_max_end(struct rb_node *node)
	58	+static void memtype_rb_augment_cb(struct rb_node node, void __unused)
60	59	{
61	60	struct memtype *data;
62	61	u64 max_end, child_max_end;
...	...	@@ -78,25 +77,6 @@
78	77	data->subtree_max_end = max_end;
79	78	}
80	79
81		-/* Update 'subtree_max_end' for a node and all its ancestors */
82		-static void update_path_max_end(struct rb_node *node)
83		-{
84		- u64 old_max_end, new_max_end;
85		-
86		- while (node) {
87		- struct memtype *data = container_of(node, struct memtype, rb);
88		-
89		- old_max_end = data->subtree_max_end;
90		- update_node_max_end(node);
91		- new_max_end = data->subtree_max_end;
92		-
93		- if (new_max_end == old_max_end)
94		- break;
95		-
96		- node = rb_parent(node);
97		- }
98		-}
99		-
100	80	/* Find the first (lowest start addr) overlapping range from rb tree */
101	81	static struct memtype memtype_rb_lowest_match(struct rb_root root,
102	82	u64 start, u64 end)
...	...	@@ -190,12 +170,6 @@
190	170	return -EBUSY;
191	171	}
192	172
193		-static void memtype_rb_augment_cb(struct rb_node *node)
194		-{
195		- if (node)
196		- update_path_max_end(node);
197		-}
198		-
199	173	static void memtype_rb_insert(struct rb_root root, struct memtype newdata)
200	174	{
201	175	struct rb_node **node = &(root->rb_node);
...	...	@@ -213,6 +187,7 @@
213	187
214	188	rb_link_node(&newdata->rb, parent, node);
215	189	rb_insert_color(&newdata->rb, root);
	190	+ rb_augment_insert(&newdata->rb, memtype_rb_augment_cb, NULL);
216	191	}
217	192
218	193	int rbt_memtype_check_insert(struct memtype new, unsigned long ret_type)
219	194
220	195
...	...	@@ -234,13 +209,16 @@
234	209
235	210	struct memtype *rbt_memtype_erase(u64 start, u64 end)
236	211	{
	212	+ struct rb_node *deepest;
237	213	struct memtype *data;
238	214
239	215	data = memtype_rb_exact_match(&memtype_rbroot, start, end);
240	216	if (!data)
241	217	goto out;
242	218
	219	+ deepest = rb_augment_erase_begin(&data->rb);
243	220	rb_erase(&data->rb, &memtype_rbroot);
	221	+ rb_augment_erase_end(deepest, memtype_rb_augment_cb, NULL);
244	222	out:
245	223	return data;
246	224	}
...	...	@@ -110,7 +110,6 @@
110	110	struct rb_root
111	111	{
112	112	struct rb_node *rb_node;
113		- void (augment_cb)(struct rb_node node);
114	113	};
115	114
116	115
...	...	@@ -130,9 +129,7 @@
130	129	rb->rb_parent_color = (rb->rb_parent_color & ~1) \| color;
131	130	}
132	131
133		-#define RB_ROOT (struct rb_root) { NULL, NULL, }
134		-#define RB_AUGMENT_ROOT(x) (struct rb_root) { NULL, x}
135		-
	132	+#define RB_ROOT (struct rb_root) { NULL, }
136	133	#define rb_entry(ptr, type, member) container_of(ptr, type, member)
137	134
138	135	#define RB_EMPTY_ROOT(root) ((root)->rb_node == NULL)
...	...	@@ -141,6 +138,14 @@
141	138
142	139	extern void rb_insert_color(struct rb_node , struct rb_root );
143	140	extern void rb_erase(struct rb_node , struct rb_root );
	141	+
	142	+typedef void (rb_augment_f)(struct rb_node node, void *data);
	143	+
	144	+extern void rb_augment_insert(struct rb_node *node,
	145	+ rb_augment_f func, void *data);
	146	+extern struct rb_node rb_augment_erase_begin(struct rb_node node);
	147	+extern void rb_augment_erase_end(struct rb_node *node,
	148	+ rb_augment_f func, void *data);
144	149
145	150	/* Find logical next and previous nodes in a tree */
146	151	extern struct rb_node rb_next(const struct rb_node );
...	...	@@ -44,11 +44,6 @@
44	44	else
45	45	root->rb_node = right;
46	46	rb_set_parent(node, right);
47		-
48		- if (root->augment_cb) {
49		- root->augment_cb(node);
50		- root->augment_cb(right);
51		- }
52	47	}
53	48
54	49	static void __rb_rotate_right(struct rb_node node, struct rb_root root)
55	50
...	...	@@ -72,20 +67,12 @@
72	67	else
73	68	root->rb_node = left;
74	69	rb_set_parent(node, left);
75		-
76		- if (root->augment_cb) {
77		- root->augment_cb(node);
78		- root->augment_cb(left);
79		- }
80	70	}
81	71
82	72	void rb_insert_color(struct rb_node node, struct rb_root root)
83	73	{
84	74	struct rb_node parent, gparent;
85	75
86		- if (root->augment_cb)
87		- root->augment_cb(node);
88		-
89	76	while ((parent = rb_parent(node)) && rb_is_red(parent))
90	77	{
91	78	gparent = rb_parent(parent);
92	79
...	...	@@ -240,15 +227,12 @@
240	227	else
241	228	{
242	229	struct rb_node old = node, left;
243		- int old_parent_cb = 0;
244		- int successor_parent_cb = 0;
245	230
246	231	node = node->rb_right;
247	232	while ((left = node->rb_left) != NULL)
248	233	node = left;
249	234
250	235	if (rb_parent(old)) {
251		- old_parent_cb = 1;
252	236	if (rb_parent(old)->rb_left == old)
253	237	rb_parent(old)->rb_left = node;
254	238	else
255	239
...	...	@@ -263,10 +247,8 @@
263	247	if (parent == old) {
264	248	parent = node;
265	249	} else {
266		- successor_parent_cb = 1;
267	250	if (child)
268	251	rb_set_parent(child, parent);
269		-
270	252	parent->rb_left = child;
271	253
272	254	node->rb_right = old->rb_right;
...	...	@@ -277,24 +259,6 @@
277	259	node->rb_left = old->rb_left;
278	260	rb_set_parent(old->rb_left, node);
279	261
280		- if (root->augment_cb) {
281		- /*
282		- * Here, three different nodes can have new children.
283		- * The parent of the successor node that was selected
284		- * to replace the node to be erased.
285		- * The node that is getting erased and is now replaced
286		- * by its successor.
287		- * The parent of the node getting erased-replaced.
288		- */
289		- if (successor_parent_cb)
290		- root->augment_cb(parent);
291		-
292		- root->augment_cb(node);
293		-
294		- if (old_parent_cb)
295		- root->augment_cb(rb_parent(old));
296		- }
297		-
298	262	goto color;
299	263	}
300	264
301	265
302	266
303	267
...	...	@@ -303,25 +267,89 @@
303	267
304	268	if (child)
305	269	rb_set_parent(child, parent);
306		-
307		- if (parent) {
	270	+ if (parent)
	271	+ {
308	272	if (parent->rb_left == node)
309	273	parent->rb_left = child;
310	274	else
311	275	parent->rb_right = child;
312		-
313		- if (root->augment_cb)
314		- root->augment_cb(parent);
315		-
316		- } else {
317		- root->rb_node = child;
318	276	}
	277	+ else
	278	+ root->rb_node = child;
319	279
320	280	color:
321	281	if (color == RB_BLACK)
322	282	__rb_erase_color(child, parent, root);
323	283	}
324	284	EXPORT_SYMBOL(rb_erase);
	285	+
	286	+static void rb_augment_path(struct rb_node node, rb_augment_f func, void data)
	287	+{
	288	+ struct rb_node *parent;
	289	+
	290	+up:
	291	+ func(node, data);
	292	+ parent = rb_parent(node);
	293	+ if (!parent)
	294	+ return;
	295	+
	296	+ if (node == parent->rb_left && parent->rb_right)
	297	+ func(parent->rb_right, data);
	298	+ else if (parent->rb_left)
	299	+ func(parent->rb_left, data);
	300	+
	301	+ node = parent;
	302	+ goto up;
	303	+}
	304	+
	305	+/*
	306	+ * after inserting @node into the tree, update the tree to account for
	307	+ * both the new entry and any damage done by rebalance
	308	+ */
	309	+void rb_augment_insert(struct rb_node node, rb_augment_f func, void data)
	310	+{
	311	+ if (node->rb_left)
	312	+ node = node->rb_left;
	313	+ else if (node->rb_right)
	314	+ node = node->rb_right;
	315	+
	316	+ rb_augment_path(node, func, data);
	317	+}
	318	+
	319	+/*
	320	+ * before removing the node, find the deepest node on the rebalance path
	321	+ * that will still be there after @node gets removed
	322	+ */
	323	+struct rb_node rb_augment_erase_begin(struct rb_node node)
	324	+{
	325	+ struct rb_node *deepest;
	326	+
	327	+ if (!node->rb_right && !node->rb_left)
	328	+ deepest = rb_parent(node);
	329	+ else if (!node->rb_right)
	330	+ deepest = node->rb_left;
	331	+ else if (!node->rb_left)
	332	+ deepest = node->rb_right;
	333	+ else {
	334	+ deepest = rb_next(node);
	335	+ if (deepest->rb_right)
	336	+ deepest = deepest->rb_right;
	337	+ else if (rb_parent(deepest) != node)
	338	+ deepest = rb_parent(deepest);
	339	+ }
	340	+
	341	+ return deepest;
	342	+}
	343	+
	344	+/*
	345	+ * after removal, update the tree to account for the removed entry
	346	+ * and any rebalance damage.
	347	+ */
	348	+void rb_augment_erase_end(struct rb_node node, rb_augment_f func, void data)
	349	+{
	350	+ if (node)
	351	+ rb_augment_path(node, func, data);
	352	+}
325	353
326	354	/*
327	355	* This function returns the first node (in sort order) of the tree.