[PATCH] Remove down_write() from taskstats code invoked on the exit() path

In send_cpu_listeners(), which is called on the exit path, a down_write() was protecting operations like skb_clone() and genlmsg_unicast() that do GFP_KERNEL allocations. If the oom-killer decides to kill tasks to satisfy the allocations,the exit of those tasks could block on the same semphore. The down_write() was only needed to allow removal of invalid listeners from the listener list. The patch converts the down_write to a down_read and defers the removal to a separate critical region. This ensures that even if the oom-killer is called, no other task's exit is blocked as it can still acquire another down_read. Thanks to Andrew Morton & Herbert Xu for pointing out the oom related pitfalls, and to Chandra Seetharaman for suggesting this fix instead of using something more complex like RCU. Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com> Signed-off-by: Shailabh Nagar <nagar@watson.ibm.com> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>

[PATCH] Remove down_write() from taskstats code invoked on the exit() path
In send_cpu_listeners(), which is called on the exit path, a down_write() was protecting operations like skb_clone() and genlmsg_unicast() that do GFP_KERNEL allocations. If the oom-killer decides to kill tasks to satisfy the allocations,the exit of those tasks could block on the same semphore. The down_write() was only needed to allow removal of invalid listeners from the listener list. The patch converts the down_write to a down_read and defers the removal to a separate critical region. This ensures that even if the oom-killer is called, no other task's exit is blocked as it can still acquire another down_read. Thanks to Andrew Morton & Herbert Xu for pointing out the oom related pitfalls, and to Chandra Seetharaman for suggesting this fix instead of using something more complex like RCU. Signed-off-by: Chandra Seetharaman <sekharan@us.ibm.com> Signed-off-by: Shailabh Nagar <nagar@watson.ibm.com> Signed-off-by: Andrew Morton <akpm@osdl.org> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
Shailabh Nagar · Linus Torvalds
1 parent f9fd8914c1
Showing 1 changed file with 19 additions and 5 deletions Side-by-side Diff
kernel/taskstats.c
@@ -51,6 +51,7 @@
 struct listener {
 	struct list_head list;
 	pid_t pid;
+	char valid;
 };
  
 struct listener_list {
@@ -127,7 +128,7 @@
 	struct listener *s, *tmp;
 	struct sk_buff *skb_next, *skb_cur = skb;
 	void *reply = genlmsg_data(genlhdr);
-	int rc, ret;
+	int rc, ret, delcount = 0;
  
 	rc = genlmsg_end(skb, reply);
 	if (rc < 0) {
@@ -137,7 +138,7 @@
  
 	rc = 0;
 	listeners = &per_cpu(listener_array, cpu);
-	down_write(&listeners->sem);
+	down_read(&listeners->sem);
 	list_for_each_entry_safe(s, tmp, &listeners->list, list) {
 		skb_next = NULL;
 		if (!list_is_last(&s->list, &listeners->list)) {
  
  
@@ -150,14 +151,26 @@
 		}
 		ret = genlmsg_unicast(skb_cur, s->pid);
 		if (ret == -ECONNREFUSED) {
-			list_del(&s->list);
-			kfree(s);
+			s->valid = 0;
+			delcount++;
 			rc = ret;
 		}
 		skb_cur = skb_next;
 	}
-	up_write(&listeners->sem);
+	up_read(&listeners->sem);
  
+	if (!delcount)
+		return rc;
+
+	/* Delete invalidated entries */
+	down_write(&listeners->sem);
+	list_for_each_entry_safe(s, tmp, &listeners->list, list) {
+		if (!s->valid) {
+			list_del(&s->list);
+			kfree(s);
+		}
+	}
+	up_write(&listeners->sem);
 	return rc;
 }
  
@@ -290,6 +303,7 @@
 				goto cleanup;
 			s->pid = pid;
 			INIT_LIST_HEAD(&s->list);
+			s->valid = 1;
  
 			listeners = &per_cpu(listener_array, cpu);
 			down_write(&listeners->sem);
...	...	@@ -51,6 +51,7 @@
51	51	struct listener {
52	52	struct list_head list;
53	53	pid_t pid;
	54	+ char valid;
54	55	};
55	56
56	57	struct listener_list {
...	...	@@ -127,7 +128,7 @@
127	128	struct listener s, tmp;
128	129	struct sk_buff skb_next, skb_cur = skb;
129	130	void *reply = genlmsg_data(genlhdr);
130		- int rc, ret;
	131	+ int rc, ret, delcount = 0;
131	132
132	133	rc = genlmsg_end(skb, reply);
133	134	if (rc < 0) {
...	...	@@ -137,7 +138,7 @@
137	138
138	139	rc = 0;
139	140	listeners = &per_cpu(listener_array, cpu);
140		- down_write(&listeners->sem);
	141	+ down_read(&listeners->sem);
141	142	list_for_each_entry_safe(s, tmp, &listeners->list, list) {
142	143	skb_next = NULL;
143	144	if (!list_is_last(&s->list, &listeners->list)) {
144	145
145	146
...	...	@@ -150,14 +151,26 @@
150	151	}
151	152	ret = genlmsg_unicast(skb_cur, s->pid);
152	153	if (ret == -ECONNREFUSED) {
153		- list_del(&s->list);
154		- kfree(s);
	154	+ s->valid = 0;
	155	+ delcount++;
155	156	rc = ret;
156	157	}
157	158	skb_cur = skb_next;
158	159	}
159		- up_write(&listeners->sem);
	160	+ up_read(&listeners->sem);
160	161
	162	+ if (!delcount)
	163	+ return rc;
	164	+
	165	+ /* Delete invalidated entries */
	166	+ down_write(&listeners->sem);
	167	+ list_for_each_entry_safe(s, tmp, &listeners->list, list) {
	168	+ if (!s->valid) {
	169	+ list_del(&s->list);
	170	+ kfree(s);
	171	+ }
	172	+ }
	173	+ up_write(&listeners->sem);
161	174	return rc;
162	175	}
163	176
...	...	@@ -290,6 +303,7 @@
290	303	goto cleanup;
291	304	s->pid = pid;
292	305	INIT_LIST_HEAD(&s->list);
	306	+ s->valid = 1;
293	307
294	308	listeners = &per_cpu(listener_array, cpu);
295	309	down_write(&listeners->sem);