Messages in this thread Patches in this message | | | Subject | Re: [PATCH] SMP signal latency fix up. | From | Mark Gross <> | Date | 06 Nov 2003 17:42:43 -0800 |
| |
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 } /*
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 } /* | |