[PATCH] simplify do_signal_stop() && utrace_report_jctl() interaction

[Oleg Nesterov]

On 07/27, Roland McGrath wrote:
>
> > Ah. I forgot signals-tracehook_notify_jctl-change.patch is still pending in
> > -mm.
>
> Perhaps we should just rejigger all these together into one new patch (or
> two, whatever) before akpm submits any of them.
>
> > Or you can just merge these 2 patches, perhaps this would be better.
>
> As long as we have akpm drop/update the any conflicting ones, sure.

OK, please find the patch below. changes: add the comment as you suggested.

> > Confused, signals-tracehook_notify_jctl-change.patch already updated the
> > comment?
>
> I was looking at the Linus tree, not the -mm tree (I always do).
> If that prerequisite patch (or merged rejiggered patch or whatever)
> updates the comments that is fine.

Yes it does (and just in case, this patch is applied to your
linux-2.6-utrace.git).

> I was also just noticing the other
> callers of tracehook_notify_jctl, which should all be rejiggered as makes
> most sense for the locking change (maybe that pending patch already did that).

Yes, it did.


Questions: should I send the change in signal.c to Andrew right now?
It can be applied separetely, it doesn't break utrace_report_jctl().

Should I send utrace part to Andrew too? This is not strictly needed.

Oleg.

---------------------------------------------------------------------
[PATCH] simplify do_signal_stop() && utrace_report_jctl() interaction

do_signal_stop() can call utrace_report_jctl() before decrementing
->group_stop_count and setting TASK_STOPPED/SIGNAL_STOP_STOPPED.
This allows to greatly simplify utrace_report_jctl() and avoid playing
with group-stop bookkeeping twice.

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

 kernel/signal.c |   40 ++++++++++++++++++----------------------
 kernel/utrace.c |   20 --------------------
 2 files changed, 18 insertions(+), 42 deletions(-)

--- __UTRACE/kernel/signal.c~JCTL	2009-07-28 01:37:58.000000000 +0200
+++ __UTRACE/kernel/signal.c	2009-07-28 21:39:31.000000000 +0200
@@ -1682,16 +1682,9 @@ void ptrace_notify(int exit_code)
 static int do_signal_stop(int signr)
 {
 	struct signal_struct *sig = current->signal;
-	int stop_count;
 	int notify;
 
-	if (sig->group_stop_count > 0) {
-		/*
-		 * There is a group stop in progress.  We don't need to
-		 * start another one.
-		 */
-		stop_count = --sig->group_stop_count;
-	} else {
+	if (!sig->group_stop_count) {
 		struct task_struct *t;
 
 		if (!likely(sig->flags & SIGNAL_STOP_DEQUEUED) ||
@@ -1703,7 +1696,7 @@ static int do_signal_stop(int signr)
 		 */
 		sig->group_exit_code = signr;
 
-		stop_count = 0;
+		sig->group_stop_count = 1;
 		for (t = next_thread(current); t != current; t = next_thread(t))
 			/*
 			 * Setting state to TASK_STOPPED for a group
@@ -1712,25 +1705,28 @@ static int do_signal_stop(int signr)
 			 */
 			if (!(t->flags & PF_EXITING) &&
 			    !task_is_stopped_or_traced(t)) {
-				stop_count++;
+				sig->group_stop_count++;
 				signal_wake_up(t, 0);
 			}
-		sig->group_stop_count = stop_count;
 	}
-
-	if (stop_count == 0)
-		sig->flags = SIGNAL_STOP_STOPPED;
-	current->exit_code = sig->group_exit_code;
-	__set_current_state(TASK_STOPPED);
-
 	/*
 	 * If there are no other threads in the group, or if there is
-	 * a group stop in progress and we are the last to stop,
-	 * report to the parent.  When ptraced, every thread reports itself.
+	 * a group stop in progress and we are the last to stop, report
+	 * to the parent.  When ptraced, every thread reports itself.
 	 */
-	notify = tracehook_notify_jctl(stop_count == 0 ? CLD_STOPPED : 0,
-				       CLD_STOPPED);
-
+	notify = sig->group_stop_count == 1 ? CLD_STOPPED : 0;
+	notify = tracehook_notify_jctl(notify, CLD_STOPPED);
+	/*
+	 * tracehook_notify_jctl() can drop and reacquire siglock, so
+	 * we keep ->group_stop_count != 0 before the call. If SIGCONT
+	 * or SIGKILL comes in between ->group_stop_count == 0.
+	 */
+	if (sig->group_stop_count) {
+		if (!--sig->group_stop_count)
+			sig->flags = SIGNAL_STOP_STOPPED;
+		current->exit_code = sig->group_exit_code;
+		__set_current_state(TASK_STOPPED);
+	}
 	spin_unlock_irq(&current->sighand->siglock);
 
 	if (notify) {
--- __UTRACE/kernel/utrace.c~JCTL	2009-07-28 20:55:01.000000000 +0200
+++ __UTRACE/kernel/utrace.c	2009-07-28 20:56:32.000000000 +0200
@@ -1607,18 +1607,7 @@ void utrace_report_jctl(int notify, int 
 	struct task_struct *task = current;
 	struct utrace *utrace = task_utrace_struct(task);
 	INIT_REPORT(report);
-	bool stop = task_is_stopped(task);
 
-	/*
-	 * We have to come out of TASK_STOPPED in case the event report
-	 * hooks might block.  Since we held the siglock throughout, it's
-	 * as if we were never in TASK_STOPPED yet at all.
-	 */
-	if (stop) {
-		__set_current_state(TASK_RUNNING);
-		task->signal->flags &= ~SIGNAL_STOP_STOPPED;
-		++task->signal->group_stop_count;
-	}
 	spin_unlock_irq(&task->sighand->siglock);
 
 	/*
@@ -1647,16 +1636,7 @@ void utrace_report_jctl(int notify, int 
 	REPORT(task, utrace, &report, UTRACE_EVENT(JCTL),
 	       report_jctl, what, notify);
 
-	/*
-	 * Retake the lock, and go back into TASK_STOPPED
-	 * unless the stop was just cleared.
-	 */
 	spin_lock_irq(&task->sighand->siglock);
-	if (stop && task->signal->group_stop_count > 0) {
-		__set_current_state(TASK_STOPPED);
-		if (--task->signal->group_stop_count == 0)
-			task->signal->flags |= SIGNAL_STOP_STOPPED;
-	}
 }
 
 /*