Re: [PATCH] cpusets+hotplug+preepmt broken

[Paul Jackson <pj@sgi.com>]

Nathan wrote:
> Hmm, tsk->cpuset will break the build if !CONFIG_CPUSETS, no?  Plus,
> the task's original cpuset->count will be unbalanced and it will never
> be released, methinks.

Ah - yup.

Well, at least I am keeping my mistakes simple enough that others can
spot them quickly ;).

Fixing the tasks cpuset by moving it to top_cpuset to match its
newly extended tsk->cpus_allowed setting probably requires holding
cpuset_sem, which is what we were trying to avoid.  Drat.


> As a short-term (i.e. 2.6.12) solution, would it be terrible to set
> tsk->cpus_allowed to the default value without messing with the
> cpuset? 

No - not terrible at all.  The code that was there before, using the
cpuset_cpus_allowed() and guarantee_online_cpus(), could do just that,
setting tsk->cpus_allowed to a superset of tsk->cpuset->cpus_allowed,
possibly even setting tsk->cpus_allowed to all bits.  No one has noticed
any terrible disasters from this yet.

Granted, no one has pushed this code path very hard yet.


> However, I think making a best effort to honor the task's cpuset is a
> reasonable goal in this context.  But it will require some nontrivial
> changes to the code for migrating tasks off the dead cpu, as well as
> some changes to the cpuset code.

If these nontrivial changes have other merits, such as making some code
simpler or clearer or more elegant, then that's goodness in any case. 

But I am reluctant to complicate either the cpuset or the hotplug code
on this account.  In my view, the kernel reserves the right to blow out
a cpuset, if it is convenient for the kernel to do so in order stay
sane, which includes:

 * insuring every task has a cpu it is allowed to run on,
 * insuring every task has a node is is allowed to allocate on, and
 * no deadlocks, hangs, oops, panics or other disasters.

The mm/page_alloc.c __alloc_pages() code makes similar tradeoffs,
allowing GFP_ATOMIC requests to ignore cpusets under memory pressure,
for the "convenience of the kernel."

A key property of any solution to this is that it is so simple and
robust that people who don't understand this stuff (including obviously
myself after a month's vacation) won't break it in the future.

The following untested patch is probably what you meant by your
"short-term solution", above.  It _just_ reverts the "No more Mr. Nice
Guy" code back to setting all bits in tsk->cpus_allowed.  I see on
another post in my inbox that Srivatsa has just heartily endorsed this
approach as well.

Signed-off-by: Paul Jackson <pj@sgi.com>

diff -Naurp 2.6.12-rc1-mm4.orig/kernel/sched.c 2.6.12-rc1-mm4/kernel/sched.c
--- 2.6.12-rc1-mm4.orig/kernel/sched.c	2005-05-13 18:39:54.000000000 -0700
+++ 2.6.12-rc1-mm4/kernel/sched.c	2005-05-14 09:06:29.000000000 -0700
@@ -4301,7 +4301,7 @@ static void move_task_off_dead_cpu(int d
 
 	/* No more Mr. Nice Guy. */
 	if (dest_cpu == NR_CPUS) {
-		tsk->cpus_allowed = cpuset_cpus_allowed(tsk);
+		cpus_setall(tsk->cpus_allowed);
 		dest_cpu = any_online_cpu(tsk->cpus_allowed);
 
 		/*

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401