[PATCH] set_cpus_allowed() atomicity fix

[PATCH] set_cpus_allowed() atomicity fix

[Robert Love]

L-Team Captain,

In set_cpus_allowed(), we hold the preempt_disable() over a
wait_for_completition() so it triggers the atomicity debugging.

There is (was) a kernel preemption race here which the preemption
disable is fixing.  I do not understand it; if this uncovers it there
should be some light shed on the subject.

Anyhow, attached patch fixes the atomicity debugging error.

Patch is against current BK.  Please, apply.

	Robert Love

 sched.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff -urN linux-bk/kernel/sched.c linux/kernel/sched.c
--- linux-bk/kernel/sched.c	2002-10-09 15:46:43.000000000 -0400
+++ linux/kernel/sched.c	2002-10-09 18:52:06.000000000 -0400
@@ -1953,7 +1953,6 @@
 		BUG();
 #endif
 
-	preempt_disable();
 	rq = task_rq_lock(p, &flags);
 	p->cpus_allowed = new_mask;
 	/*
@@ -1962,7 +1961,7 @@
 	 */
 	if (new_mask & (1UL << task_cpu(p))) {
 		task_rq_unlock(rq, &flags);
-		goto out;
+		return;
 	}
 	/*
 	 * If the task is not on a runqueue (and not running), then
@@ -1971,17 +1970,21 @@
 	if (!p->array && !task_running(rq, p)) {
 		set_task_cpu(p, __ffs(p->cpus_allowed));
 		task_rq_unlock(rq, &flags);
-		goto out;
+		return;
 	}
 	init_completion(&req.done);
 	req.task = p;
 	list_add(&req.list, &rq->migration_queue);
+
+	/*
+	 * counter the subsequent unlock - we do not want to preempt yet
+	 */
+	preempt_disable();
 	task_rq_unlock(rq, &flags);
 	wake_up_process(rq->migration_thread);
+	preempt_enable();
 
 	wait_for_completion(&req.done);
-out:
-	preempt_enable();
 }
 
 /*

Re: [PATCH] set_cpus_allowed() atomicity fix

[Linus Torvalds]

On 9 Oct 2002, Robert Love wrote:
> 
> Anyhow, attached patch fixes the atomicity debugging error.

I don't think this is right. You have to be preempt safe over the whole 
time you're holding the "rq" pointer, I think. Otherwise you might move to 
another CPU, at which point the rq state isn't valid any more. Or maybe I 
misunderstood.

		Linus

Re: [PATCH] set_cpus_allowed() atomicity fix

[Robert Love]

On Wed, 2002-10-09 at 19:17, Linus Torvalds wrote:

> On 9 Oct 2002, Robert Love wrote:
> > 
> > Anyhow, attached patch fixes the atomicity debugging error.
> 
> I don't think this is right. You have to be preempt safe over the whole 
> time you're holding the "rq" pointer, I think. Otherwise you might move to 
> another CPU, at which point the rq state isn't valid any more. Or maybe I 
> misunderstood.

I agree.  But aren't we?

We are preempt-safe through the entire function (starting at the top
with task_rq_lock()) until the preempt_disable().  The only instruction
outside of the critical section is the wait_for_completion() which
sleeps anyhow.

Or maybe _I_ misunderstood?

	Robert Love