Re: [patch] O(1) scheduler, -G1, 2.5.2-pre10, 2.4.17 (fwd)

[Robert Love <rml@tech9.net>]

On Thu, 2002-01-10 at 09:19, Ingo Molnar wrote:

>  - Kevin O'Connor, Robert Love: fix locking order bug in set_cpus_allowed()
>    which bug is able to cause boot-time lockups on SMP systems.

Along the same lines as the above, note this code snippet from
try_to_wake_up:

	lock_task_rq(rq, p, flags);
	p->state = TASK_RUNNING;
	if (!p->array) {
		if (0 && !rt_task(p) && synchronous && (smp_processor_id() < p->cpu)) {
			spin_lock(&this_rq()->lock);
			p->cpu = smp_processor_id();
			activate_task(p, this_rq());
			spin_unlock(&this_rq()->lock);
		} else { ... }

First, H1 added the 0 there ... ???

Second, prior to the change, I wonder whether the
(smp_processor_id() < p->cpu)) is inverted.  We've already locked p's
runqueue above.  So this branch, if taken, will lock the current
runqueue -- but it checks if current's cpu id is _less_ then p's!  Thus
we lock greater to lesser.  Would a proper change be:

diff -urN linux-2.5.2-pre10-H1/kernel/sched.c linux/kernel/sched.c
--- linux-2.5.2-pre10-H1/kernel/sched.c	Thu Jan 10 14:56:06 2002
+++ linux/kernel/sched.c	Thu Jan 10 14:56:21 2002
@@ -383,7 +383,7 @@
 	lock_task_rq(rq, p, flags);
 	p->state = TASK_RUNNING;
 	if (!p->array) {
-		if (0 && !rt_task(p) && synchronous && (smp_processor_id() < p->cpu)) {
+		if (!rt_task(p) && synchronous && (smp_processor_id() > p->cpu)) {
 			spin_lock(&this_rq()->lock);
 			p->cpu = smp_processor_id();
 			activate_task(p, this_rq());

Note I removed the 0, which may or not be your intention (do you want
that code branch there at all?)  My point is swapping the
lesser-than-sign for a greater-than so that we now lock this_rq only if
it is _greater_ than p's.

Keep 'em coming ;)

	Robert Love