Re: [PATCH] SMP signal latency fix up.

[Mark Gross <mgross@linux.co.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5841 bytes --]

On Thu, 2003-11-06 at 17:39, Mark Gross wrote:
> On Thu, 2003-11-06 at 15:20, Linus Torvalds wrote:
> > On 6 Nov 2003, Mark Gross wrote:
> > >
> > > Running on SMP and the 2.6.0-test9 kernel, it takes about 10000 * 1/HZ seconds.  Running this 
> > > command with maxcpus=1 the command finishes in fraction of a second.  Under SMP the 
> > > signal delivery isn't kicking the task if its in the run state on the other CPU.
> > 
> > It looks like the "wake_up_process_kick()" interface is just broken. As it 
> > stands now, it's literally _designed_ to kick the process only when it 
> > wakes it up, which is silly and wrong. It makes no sense to kick a process 
> > that we just woke up, because it _will_ react immediately anyway.
> > 
> > We literally want to kick only processes that didn't need waking up, and 
> > the current interface is totally unsuitable for that.
> > 
> > > The following patch has been tested and seems to fix the problem.  
> > > I'm confident about the change to sched.c actualy fixes a cut and paste bug.
> > 
> > Naah, it's a thinko, the code shouldn't be like that at all.
> > 
> > There's only one user of the "wake_up_process_kick()" thing, and that one 
> > user really wants to kick the process totally independently of waking it 
> > up. Which just implies that we should just have a _regular_ 
> > "wake_up_process()" there, and a _separate_ "kick_process()" thing.
> > 
> > So I've got a feeling that 
> >  - we should remove the "kick" argument from "try_to_wake_up()"
> >  - the signal wakeup case should instead do a _regular_ wakeup.
> >  - we should kick the process if the wakeup _fails_.
> > 
> > Ie signal wakeup should most likely look something like
> > 
> > 
> > 	inline void signal_wake_up(struct task_struct *t, int resume)
> > 	{
> > 		int woken;
> > 		unsigned int mask;
> > 
> > 		set_tsk_thread_flag(t,TIF_SIGPENDING);
> > 		mask = TASK_INTERRUPTIBLE;
> > 		if (resume)
> > 			mask |= TASK_STOPPED;
> > 		woken = 0;
> > 		if (t->state & mask)
> > 			woken = wake_up_state(p, mask);
> > 		if (!woken)
> > 			kick_process(p);
> > 	}
> > 
> > where the "kick_process()" thing does the "is the task running on some 
> > other CPU and if so send it a reschedule event to make it react" thing.
> > 
> > Ingo?
> > 
> > 		Linus
> 
> 
> Are you thinking about something like this?
> 
> It seems to work. I dropped the "task_running" test from
> smp_process_kick intentionaly.  As well as the movement of the success
> flag.  I hope its not too wrong.
> 
> It seems to work on a HT P4 desktop and a dual PIII box.
> 
> --mgross
Evolution messed up my patch.  Retry:

diff -urN -X dontdiff linux-2.6.0-test9/include/linux/sched.h /opt/linux-2.6.0-test9/include/linux/sched.h
--- linux-2.6.0-test9/include/linux/sched.h	2003-10-25 11:42:56.000000000 -0700
+++ /opt/linux-2.6.0-test9/include/linux/sched.h	2003-11-06 14:44:22.000000000 -0800
@@ -573,7 +573,7 @@
 
 extern int FASTCALL(wake_up_state(struct task_struct * tsk, unsigned int state));
 extern int FASTCALL(wake_up_process(struct task_struct * tsk));
-extern int FASTCALL(wake_up_process_kick(struct task_struct * tsk));
+extern void FASTCALL(smp_process_kick(struct task_struct * tsk));
 extern void FASTCALL(wake_up_forked_process(struct task_struct * tsk));
 extern void FASTCALL(sched_exit(task_t * p));
 
diff -urN -X dontdiff linux-2.6.0-test9/kernel/sched.c /opt/linux-2.6.0-test9/kernel/sched.c
--- linux-2.6.0-test9/kernel/sched.c	2003-10-25 11:44:29.000000000 -0700
+++ /opt/linux-2.6.0-test9/kernel/sched.c	2003-11-06 14:58:29.000000000 -0800
@@ -585,7 +585,7 @@
  *
  * returns failure only if the task is already active.
  */
-static int try_to_wake_up(task_t * p, unsigned int state, int sync, int kick)
+static int try_to_wake_up(task_t * p, unsigned int state, int sync)
 {
 	unsigned long flags;
 	int success = 0;
@@ -624,13 +624,8 @@
 				if (TASK_PREEMPTS_CURR(p, rq))
 					resched_task(rq->curr);
 			}
-			success = 1;
 		}
-#ifdef CONFIG_SMP
-	       	else
-			if (unlikely(kick) && task_running(rq, p) && (task_cpu(p) != smp_processor_id()))
-				smp_send_reschedule(task_cpu(p));
-#endif
+		success = 1;
 		p->state = TASK_RUNNING;
 	}
 	task_rq_unlock(rq, &flags);
@@ -640,19 +635,22 @@
 
 int wake_up_process(task_t * p)
 {
-	return try_to_wake_up(p, TASK_STOPPED | TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE, 0, 0);
+	return try_to_wake_up(p, TASK_STOPPED | TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE, 0);
 }
 
 EXPORT_SYMBOL(wake_up_process);
 
-int wake_up_process_kick(task_t * p)
+void smp_process_kick(task_t * p)
 {
-	return try_to_wake_up(p, TASK_STOPPED | TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE, 0, 1);
+#ifdef CONFIG_SMP
+	if (task_cpu(p) != smp_processor_id())
+			smp_send_reschedule(task_cpu(p));
+#endif
 }
 
 int wake_up_state(task_t *p, unsigned int state)
 {
-	return try_to_wake_up(p, state, 0, 0);
+	return try_to_wake_up(p, state, 0);
 }
 
 /*
@@ -1624,7 +1622,7 @@
 int default_wake_function(wait_queue_t *curr, unsigned mode, int sync)
 {
 	task_t *p = curr->task;
-	return try_to_wake_up(p, mode, sync, 0);
+	return try_to_wake_up(p, mode, sync);
 }
 
 EXPORT_SYMBOL(default_wake_function);
diff -urN -X dontdiff linux-2.6.0-test9/kernel/signal.c /opt/linux-2.6.0-test9/kernel/signal.c
--- linux-2.6.0-test9/kernel/signal.c	2003-10-25 11:43:27.000000000 -0700
+++ /opt/linux-2.6.0-test9/kernel/signal.c	2003-11-06 15:05:41.945237624 -0800
@@ -538,6 +538,7 @@
 inline void signal_wake_up(struct task_struct *t, int resume)
 {
 	unsigned int mask;
+	int woken;
 
 	set_tsk_thread_flag(t,TIF_SIGPENDING);
 
@@ -551,10 +552,14 @@
 	mask = TASK_INTERRUPTIBLE;
 	if (resume)
 		mask |= TASK_STOPPED;
+	woken = 0;
 	if (t->state & mask) {
-		wake_up_process_kick(t);
-		return;
+		woken = wake_up_process(t);
 	}
+#ifdef CONFIG_SMP
+	if (!woken) 
+		smp_process_kick(t);
+#endif	
 }
 
 /*



[-- Attachment #2: signal_smp_fix.patch --]
[-- Type: text/x-patch, Size: 3225 bytes --]

diff -urN -X dontdiff linux-2.6.0-test9/include/linux/sched.h /opt/linux-2.6.0-test9/include/linux/sched.h
--- linux-2.6.0-test9/include/linux/sched.h	2003-10-25 11:42:56.000000000 -0700
+++ /opt/linux-2.6.0-test9/include/linux/sched.h	2003-11-06 14:44:22.000000000 -0800
@@ -573,7 +573,7 @@
 
 extern int FASTCALL(wake_up_state(struct task_struct * tsk, unsigned int state));
 extern int FASTCALL(wake_up_process(struct task_struct * tsk));
-extern int FASTCALL(wake_up_process_kick(struct task_struct * tsk));
+extern void FASTCALL(smp_process_kick(struct task_struct * tsk));
 extern void FASTCALL(wake_up_forked_process(struct task_struct * tsk));
 extern void FASTCALL(sched_exit(task_t * p));
 
diff -urN -X dontdiff linux-2.6.0-test9/kernel/sched.c /opt/linux-2.6.0-test9/kernel/sched.c
--- linux-2.6.0-test9/kernel/sched.c	2003-10-25 11:44:29.000000000 -0700
+++ /opt/linux-2.6.0-test9/kernel/sched.c	2003-11-06 14:58:29.000000000 -0800
@@ -585,7 +585,7 @@
  *
  * returns failure only if the task is already active.
  */
-static int try_to_wake_up(task_t * p, unsigned int state, int sync, int kick)
+static int try_to_wake_up(task_t * p, unsigned int state, int sync)
 {
 	unsigned long flags;
 	int success = 0;
@@ -624,13 +624,8 @@
 				if (TASK_PREEMPTS_CURR(p, rq))
 					resched_task(rq->curr);
 			}
-			success = 1;
 		}
-#ifdef CONFIG_SMP
-	       	else
-			if (unlikely(kick) && task_running(rq, p) && (task_cpu(p) != smp_processor_id()))
-				smp_send_reschedule(task_cpu(p));
-#endif
+		success = 1;
 		p->state = TASK_RUNNING;
 	}
 	task_rq_unlock(rq, &flags);
@@ -640,19 +635,22 @@
 
 int wake_up_process(task_t * p)
 {
-	return try_to_wake_up(p, TASK_STOPPED | TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE, 0, 0);
+	return try_to_wake_up(p, TASK_STOPPED | TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE, 0);
 }
 
 EXPORT_SYMBOL(wake_up_process);
 
-int wake_up_process_kick(task_t * p)
+void smp_process_kick(task_t * p)
 {
-	return try_to_wake_up(p, TASK_STOPPED | TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE, 0, 1);
+#ifdef CONFIG_SMP
+	if (task_cpu(p) != smp_processor_id())
+			smp_send_reschedule(task_cpu(p));
+#endif
 }
 
 int wake_up_state(task_t *p, unsigned int state)
 {
-	return try_to_wake_up(p, state, 0, 0);
+	return try_to_wake_up(p, state, 0);
 }
 
 /*
@@ -1624,7 +1622,7 @@
 int default_wake_function(wait_queue_t *curr, unsigned mode, int sync)
 {
 	task_t *p = curr->task;
-	return try_to_wake_up(p, mode, sync, 0);
+	return try_to_wake_up(p, mode, sync);
 }
 
 EXPORT_SYMBOL(default_wake_function);
diff -urN -X dontdiff linux-2.6.0-test9/kernel/signal.c /opt/linux-2.6.0-test9/kernel/signal.c
--- linux-2.6.0-test9/kernel/signal.c	2003-10-25 11:43:27.000000000 -0700
+++ /opt/linux-2.6.0-test9/kernel/signal.c	2003-11-06 15:05:41.945237624 -0800
@@ -538,6 +538,7 @@
 inline void signal_wake_up(struct task_struct *t, int resume)
 {
 	unsigned int mask;
+	int woken;
 
 	set_tsk_thread_flag(t,TIF_SIGPENDING);
 
@@ -551,10 +552,14 @@
 	mask = TASK_INTERRUPTIBLE;
 	if (resume)
 		mask |= TASK_STOPPED;
+	woken = 0;
 	if (t->state & mask) {
-		wake_up_process_kick(t);
-		return;
+		woken = wake_up_process(t);
 	}
+#ifdef CONFIG_SMP
+	if (!woken) 
+		smp_process_kick(t);
+#endif	
 }
 
 /*