[PATCH 3/4] utrace_reap: do not play with lock/unlock/restart

[Oleg Nesterov]

(fix the subject)

>From the previous changelog:
>
> Once we drop utrace->lock, any other caller which takes this lock must
> see both ->exit_state and utrace->reap. This means utrace_control() or
> utrace_set_events() which sets/clears REAP must not succeed.

IOW, nobody change change engine->ops or REAP bit in flags. Nobody can
add the new engine or remove it from ->attached. We can do all work
lockless and without barriers.

To simplify the review, utrace_reap() becomes:

	static void utrace_reap(struct task_struct *target, struct utrace *utrace)
		__releases(utrace->lock)
	{
		struct utrace_engine *engine;

		splice_attaching(utrace);
		spin_unlock(&utrace->lock);
		/*
		 * We were called with utrace->reap set:
		 * nobody can set/clear UTRACE_EVENT(REAP) in engine->flags or
		 * change engine->ops, and nobody can change utrace->attached.
		 */
		list_for_each_entry(engine, &utrace->attached, entry) {
			if (engine->flags & UTRACE_EVENT(REAP))
				engine->ops->report_reap(engine, target);

			engine->ops = NULL;
			engine->flags = 0;

		}

		put_detached_list(&utrace->attached);
	}

Note that I changed the logic a bit, we set ->ops = NULL after
->report_reap(). Is it OK? If not, it is trivial to change.

Signed-off-by: Oleg Nesterov <oleg at redhat.com>
---

 kernel/utrace.c |   39 ++++++++++++---------------------------
 1 file changed, 12 insertions(+), 27 deletions(-)

--- __UTRACE/kernel/utrace.c~3_NO_LOCKING	2009-09-06 15:09:59.000000000 +0200
+++ __UTRACE/kernel/utrace.c	2009-09-06 15:06:23.000000000 +0200
@@ -408,40 +408,25 @@ static void put_detached_list(struct lis
 static void utrace_reap(struct task_struct *target, struct utrace *utrace)
 	__releases(utrace->lock)
 {
-	struct utrace_engine *engine, *next;
-	const struct utrace_engine_ops *ops;
-	unsigned long flags;
-	LIST_HEAD(detached);
+	struct utrace_engine *engine;
 
-restart:
 	splice_attaching(utrace);
-	list_for_each_entry_safe(engine, next, &utrace->attached, entry) {
-		ops = engine->ops;
-		flags = engine->flags;
+	spin_unlock(&utrace->lock);
+	/*
+	 * We were called with utrace->reap set:
+	 * nobody can set/clear UTRACE_EVENT(REAP) in engine->flags or
+	 * change engine->ops, and nobody can change utrace->attached.
+	 */
+	list_for_each_entry(engine, &utrace->attached, entry) {
+		if (engine->flags & UTRACE_EVENT(REAP))
+			engine->ops->report_reap(engine, target);
+
 		engine->ops = NULL;
 		engine->flags = 0;
-		list_move(&engine->entry, &detached);
 
-		/*
-		 * If it didn't need a callback, we don't need to drop
-		 * the lock.  Now nothing else refers to this engine.
-		 */
-		if (!(flags & UTRACE_EVENT(REAP)))
-			continue;
-
-		spin_unlock(&utrace->lock);
-
-		(*ops->report_reap)(engine, target);
-
-		put_detached_list(&detached);
-
-		spin_lock(&utrace->lock);
-		goto restart;
 	}
 
-	spin_unlock(&utrace->lock);
-
-	put_detached_list(&detached);
+	put_detached_list(&utrace->attached);
 }
 
 /*