From: Max Krasnyanskiy <maxk@qualcomm.com>
I do not really like the current solution of dropping cgroup lock

but it shows what I have in mind in general.
Basically as Paul J. pointed out rebuild_sched_domains() is the

only way to rebuild sched domains correctly based on the current

cpuset settings. What this means is that we need to be able to

call it from different contexts, like cpuhotplug for example.
In order to get there we need to redo the lock nesting a bit.

This patch tries to do that. New lock nesting is explained in the

comments.

Paul M has some patches for fine grain cgroup locking. That should

simplify things in the future.
This passes light testing (building stuff and creating/removing

domains and bring cpus off/online at the same time) and keeps lockdep

mostly happy. The only thing that I haven't resolved yet is some

complains from lockdep and preemtable RCU is enabled.
Signed-off-by: Max Krasnyanskiy <maxk@qualcomm.com>

Cc: a.p.zijlstra@chello.nl

Cc: pj@sgi.com

Cc: mingo@elte.hu

Cc: menage@google.com

---

 kernel/cpuset.c |   91 ++++++++++++++++++++++++++++--------------------------

 1 files changed, 47 insertions(+), 44 deletions(-)
diff --git a/kernel/cpuset.c b/kernel/cpuset.c

index 3c3ef02..7b9fa17 100644

--- a/kernel/cpuset.c

+++ b/kernel/cpuset.c

@@ -522,13 +522,8 @@ update_domain_attr(struct sched_domain_attr *dattr, struct cpuset *c)

  * domains when operating in the severe memory shortage situations

  * that could cause allocation failures below.

  *

- * Call with cgroup_mutex held.  May take callback_mutex during

- * call due to the kfifo_alloc() and kmalloc() calls.  May nest

- * a call to the get_online_cpus()/put_online_cpus() pair.

- * Must not be called holding callback_mutex, because we must not

- * call get_online_cpus() while holding callback_mutex.  Elsewhere

- * the kernel nests callback_mutex inside get_online_cpus() calls.

- * So the reverse nesting would risk an ABBA deadlock.

+ * Call under get_online_cpus().

+ *...
[ message continues ]

I think that dropping the cgroup lock will open up races for cpusets.

The idea of a separate workqueue/thread to do the sched domain

rebuilding is simplest.
Paul

--

Actually I think we do not have to make it super strict "only rebuilt

from that thread rule". I'd only off-load cpuset_write64(),

update_flag() to the thread. It'd be nice to keep hotplug path clean

synchronous. It's synchronous without cpusets so there is really no good

reason when it needs to be async without them. And the toughest part is

not even hotplug where lock nesting is pretty clear

get_online_cpus() ->

	rebuild_sched_domains() ->

		cgroup_lock();

		// Build cpumaps

			cpuset_callback_lock();

			...

			cpuset_callback_unlock();

		cgroup_unlock();
		partition_sched_domains() ->

			mutex_unlock(&sched_domains_mutex);

			// Rebuild sched domains

			mutex_unlock(&sched_domains_mutex);

put_online_cpus()
It's the other paths where cgroup_lock() is taken by cgroups before even

calling into cpusets, like cgroup destroy case.

So I think we should just off-load those.
Max

--

Actually I audited (to the best of my knowledge) all the paths in

cpusets and rebuild_sched_domains() is the last action. ie We drop the

lock right after it anyways. It's just it's embedded deep in the call

stack and therefor I cannot drop it at the higher level.
The only path where I think it's not safe is the cgroup destroy thing

where we do
cgroup.c

	cgroup_lock();

	for_each_cgroups(...)

		cg->destroy();

	cgroup_unlock();
So in theory it's just that one patch that really needs the workqueue

trick. But I do agree that it'll make it less tricky across the board.

So I'll pick up you work queue based patch, convert it to single

threaded, bang on a bit later today and send a patch on top of this one.
Max
--

The following is just a wild idea.
On systems using cpusets to setup scheduler domains (anything more

elaborate than the default of a single, system-wide, domain) have a

special purpose thread sit around, that does nothing except rebuild

sched domains on demand.
If this rebuild thread was the -only- way that sched domains were

allowed to be rebuilt, and if this rebuild was done -asynchronously-

sometime shortly after requested, without any error or status feedback,

then it would seem to simplify the locking issues.
I would guess that any system big enough to have a non-default, cpuset

determined, sched domain configuration is big enough to not mind one

more thread sitting around, doing nothing most of the time.
If the above is a really stupid idea, have fun taking out your

agression on it ;).
--

                  I won't rest till it's the best ...

                  Programmer, Linux Scalability

                  Paul Jackson <pj@sgi.com> 1.940.382.4214

--

I sent a patch that was similar a couple of weeks ago, that used a

workqueue to do the rebuild. It didn't quite work then since it wasn't

safe to call get_online_cpus() from a multi-threaded workqueue then,

but I believe there's been a patch since then that makes this safe.

And if not, we could always have a single-threaded workqueue that

wasn't bound to any particular CPU.
Paul

--

Actually I think we do not have to make it super strict "only rebuilt

from that thread rule". I'd only off-load cpuset_write64(),

update_flag() to the thread. It'd be nice to keep hotplug path clean

synchronous. It's synchronous without cpusets so there is really no good

reason when it needs to be async without them. And the toughest part is

not even hotplug where lock nesting is pretty clear

get_online_cpus() ->

	rebuild_sched_domains() ->

		cgroup_lock();

		// Build cpumaps

			cpuset_callback_lock();

			...

			cpuset_callback_unlock();

		cgroup_unlock();
		partition_sched_domains() ->

			mutex_unlock(&sched_domains_mutex);

			// Rebuild sched domains

			mutex_unlock(&sched_domains_mutex);

put_online_cpus()
It's the other paths where cgroup_lock() is taken by cgroups before even

calling into cpusets, like cgroup destroy case.

So I think we should just off-load those.
Max

--