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

[PATCH 2/2] change attach/release to avoid unnecessary utrace_reap()



While this is not documented explicitely (afaik), the kernel depends
on store-mb-load behavior. Just for example,

	CPU_0:

		current->state = TASK_INTERRUPTIBLE;
		mb();
		if (!COND)
			schedule();
vs
	CPU_1:
		COND = 1;
		wake_up_process(task);
		
			mb();	// <---- implicit
			if (task->state == TASK_RUNNING)
				return;
			...
			task->state = TASK_RUNNING;

should always work. Either CPU_0 must see the condition, or wakeup
must see task->state.

IOW, if we have A == B == 0, then

	CPU_0				CPU_1

	A = 1;				B = 1
	mb();				mb();
	if (B)				if (A)
		printk("OK");			printk("OK");

"OK" should be printed at least once when both CPUs pass this code.

This means we can avoid utrace_release_task() when there are no attached
engines. With this patch we have:

	attach:

		task->utrace_flags = REAP;
		mb();
		if (task->exit_state == EXIT_DEAD)
			abort;

		add the new engine

	release:

		task->exit_state = exit_DEAD;
		mb();
		if (task->utrace_flags)
			utrace_release_task();

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

 include/linux/tracehook.h |    5 ++++-
 kernel/utrace.c           |   26 ++++++++++++++++++++------
 2 files changed, 24 insertions(+), 7 deletions(-)

--- __UTRACE/include/linux/tracehook.h~2_DONT_REAP_UNCONDITIONALLY	2009-08-04 19:48:28.000000000 +0200
+++ __UTRACE/include/linux/tracehook.h	2009-09-08 20:58:53.000000000 +0200
@@ -362,7 +362,10 @@ static inline void tracehook_report_vfor
  */
 static inline void tracehook_prepare_release_task(struct task_struct *task)
 {
-	utrace_release_task(task);
+	/* see utrace_add_engine() about this barrier */
+	smp_mb();
+	if (task_utrace_flags(task))
+		utrace_release_task(task);
 }
 
 /**
--- __UTRACE/kernel/utrace.c~2_DONT_REAP_UNCONDITIONALLY	2009-09-08 20:48:56.000000000 +0200
+++ __UTRACE/kernel/utrace.c	2009-09-08 21:10:23.000000000 +0200
@@ -139,7 +139,7 @@ static int utrace_add_engine(struct task
 			     const struct utrace_engine_ops *ops,
 			     void *data)
 {
-	int ret;
+	int ret = 0;
 
 	spin_lock(&utrace->lock);
 
@@ -149,7 +149,7 @@ static int utrace_add_engine(struct task
 		 */
 		ret = -ESRCH;
 	} else if ((flags & UTRACE_ATTACH_EXCLUSIVE) &&
-	    unlikely(matching_engine(utrace, flags, ops, data))) {
+		   unlikely(matching_engine(utrace, flags, ops, data))) {
 		ret = -EEXIST;
 	} else {
 		/*
@@ -168,10 +168,24 @@ static int utrace_add_engine(struct task
 		 * In case we had no engines before, make sure that
 		 * utrace_flags is not zero.
 		 */
-		target->utrace_flags |= UTRACE_EVENT(REAP);
-		list_add_tail(&engine->entry, &utrace->attaching);
-		utrace->pending_attach = 1;
-		ret = 0;
+		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;
+		}
 	}
 
 	spin_unlock(&utrace->lock);


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