[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[PATCH 4/2] utrace_add_engine: cleanup



On 09/08, Oleg Nesterov wrote:
>
> I am starting to think we are going to add too much subtle complications
> to avoid lock/unlock in release path :/

Or not, I dunno. up to you.

But if we do this change, I'd like to re-factor utrace_add_engine().
Mostly because "if (utrace->reap)" check should be near ->exit_state
check, they both are the same thing: protect against release.

OTOH, if we change utrace_reap() to clear ->utrace_flags, then
utrace_add_engine() do not need to check ->reap at all.

If you don't like it - just ignore, I don't have a strong feeling.
To simplify the review:

	static int utrace_add_engine(struct task_struct *target,
				     struct utrace *utrace,
				     struct utrace_engine *engine,
				     int flags,
				     const struct utrace_engine_ops *ops,
				     void *data)
	{
		int ret;

		spin_lock(&utrace->lock);

		ret = -EEXIST;
		if ((flags & UTRACE_ATTACH_EXCLUSIVE) &&
		     unlikely(matching_engine(utrace, flags, ops, data)))
			goto unlock;

		ret = -ESRCH;
		/* can't attach after utrace_release_task() */
		if (utrace->reap)
			goto unlock;

		/*
		 * In case we had no engines before, make sure that
		 * utrace_flags is not zero.
		 */
		if (!target->utrace_flags) {
			target->utrace_flags = UTRACE_EVENT(REAP);
			/*
			 * If we race with tracehook_prepare_release_task()
			 * make sure that either it sees utrace_flags != 0
			 * or we see exit_state == EXIT_DEAD.
			 */
			smp_mb();
			if (unlikely(target->exit_state == EXIT_DEAD)) {
				target->utrace_flags = 0;
				goto unlock;
			}
		}

		/*
		 * Put the new engine on the pending ->attaching list.
		 * Make sure it gets onto the ->attached list by the next
		 * time it's examined.  Setting ->pending_attach ensures
		 * that start_report() takes the lock and splices the lists
		 * before the next new reporting pass.
		 *
		 * When target == current, it would be safe just to call
		 * splice_attaching() right here.  But if we're inside a
		 * callback, that would mean the new engine also gets
		 * notified about the event that precipitated its own
		 * creation.  This is not what the user wants.
		 */
		list_add_tail(&engine->entry, &utrace->attaching);
		utrace->pending_attach = 1;
		ret = 0;
	unlock:
		spin_unlock(&utrace->lock);

		return ret;
	}

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

 kernel/utrace.c |   81 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 40 insertions(+), 41 deletions(-)

--- __UTRACE/kernel/utrace.c~4_CLEANUP_ADD_ENGINE	2009-09-08 22:32:44.000000000 +0200
+++ __UTRACE/kernel/utrace.c	2009-09-08 23:46:10.000000000 +0200
@@ -139,55 +139,54 @@ static int utrace_add_engine(struct task
 			     const struct utrace_engine_ops *ops,
 			     void *data)
 {
-	int ret = 0;
+	int ret;
 
 	spin_lock(&utrace->lock);
 
-	if (utrace->reap) {
-		/*
-		 * Already entered utrace_release_task(), cannot attach now.
-		 */
-		ret = -ESRCH;
-	} else if ((flags & UTRACE_ATTACH_EXCLUSIVE) &&
-		   unlikely(matching_engine(utrace, flags, ops, data))) {
-		ret = -EEXIST;
-	} else {
+	ret = -EEXIST;
+	if ((flags & UTRACE_ATTACH_EXCLUSIVE) &&
+	     unlikely(matching_engine(utrace, flags, ops, data)))
+		goto unlock;
+
+	ret = -ESRCH;
+	/* can't attach after utrace_release_task() */
+	if (utrace->reap)
+		goto unlock;
+	/*
+	 * In case we had no engines before, make sure that
+	 * utrace_flags is not zero.
+	 */
+	if (!target->utrace_flags) {
+		target->utrace_flags = UTRACE_EVENT(REAP);
 		/*
-		 * Put the new engine on the pending ->attaching list.
-		 * Make sure it gets onto the ->attached list by the next
-		 * time it's examined.  Setting ->pending_attach ensures
-		 * that start_report() takes the lock and splices the lists
-		 * before the next new reporting pass.
-		 *
-		 * When target == current, it would be safe just to call
-		 * splice_attaching() right here.  But if we're inside a
-		 * callback, that would mean the new engine also gets
-		 * notified about the event that precipitated its own
-		 * creation.  This is not what the user wants.
-		 *
-		 * In case we had no engines before, make sure that
-		 * utrace_flags is not zero.
+		 * If we race with tracehook_prepare_release_task()
+		 * make sure that either it sees utrace_flags != 0
+		 * or we see exit_state == EXIT_DEAD.
 		 */
-		if (!target->utrace_flags) {
-			target->utrace_flags = UTRACE_EVENT(REAP);
-			/*
-			 * If we race with tracehook_prepare_release_task()
-			 * make sure that either it sees utrace_flags != 0
-			 * or we see exit_state == EXIT_DEAD.
-			 */
-			smp_mb();
-			if (unlikely(target->exit_state == EXIT_DEAD)) {
-				target->utrace_flags = 0;
-				ret = -ESRCH;
-			}
-		}
-
-		if (!ret) {
-			list_add_tail(&engine->entry, &utrace->attaching);
-			utrace->pending_attach = 1;
+		smp_mb();
+		if (unlikely(target->exit_state == EXIT_DEAD)) {
+			target->utrace_flags = 0;
+			goto unlock;
 		}
 	}
 
+	/*
+	 * Put the new engine on the pending ->attaching list.
+	 * Make sure it gets onto the ->attached list by the next
+	 * time it's examined.  Setting ->pending_attach ensures
+	 * that start_report() takes the lock and splices the lists
+	 * before the next new reporting pass.
+	 *
+	 * When target == current, it would be safe just to call
+	 * splice_attaching() right here.  But if we're inside a
+	 * callback, that would mean the new engine also gets
+	 * notified about the event that precipitated its own
+	 * creation.  This is not what the user wants.
+	 */
+	list_add_tail(&engine->entry, &utrace->attaching);
+	utrace->pending_attach = 1;
+	ret = 0;
+unlock:
 	spin_unlock(&utrace->lock);
 
 	return ret;


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]