set_cpus_allowed() atomicity fix

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
@@ -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();
 }
 
 /*

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

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