Re: [RFC][PATCH] CPUSets: Move most calls to rebuild_sched_domains() to the workqueue

[Max Krasnyansky <maxk@qualcomm.com>]

Paul Menage wrote:
> On Thu, Jun 26, 2008 at 10:10 PM, Max Krasnyansky <maxk@qualcomm.com> wrote:
>> Instead of changing cpu_hotplug locking should we maybe try to avoid using
>> cgroup_lock in rebuild_sched_domains() ?
> 
> Yes, that would be good too.
> 
>> There is a comment in cpuset.c that says
>>  * If a task is only holding callback_mutex, then it has read-only
>>  * access to cpusets.
>>
>> I'm not sure if it's still valid. rebuild_sched_domains() only needs read only
>> access, it does not really modify any cpuset structures.
> 
> The comment is still valid, if you interpret it narrowly enough.
> Holding callback_mutex gives you read-only access to structures that
> are under the control of cpusets. But rebuild_sched_domains() needs to
> traverse the hierarchy of cpusets, and that hierarchy is controlled by
> cgroups. 
Yes that's what I meant by "not sure if it's still valid" I looked at 
the code and it did not look like callback_mutex protected overall 
hierarchy. Thanx for confirming that.

> Currently the only synchronization primitives exposed by
> cgroups are:
> 
> - cgroup_lock()/cgroup_unlock() to prevent all cgroup modifications
> (also used as the main synchronization primitive by some subsystems,
> i.e. it's in danger of becoming the cgroups equivalent of the BKL).
> 
> - task_lock()/task_unlock() to prevent a specific task from changing cgroups
> 
> Possible options for richer locking support include:
> 
> - lock/unlock a hierarchy, to prevent creation/deletion of cgroups in
> that hierarchy
Sounds good.

> - lock/unlock a cgroup to prevent deletion of that cgroup
Can that be just an atomic refcount ?

> - lock/unlock a cgroup to prevent task movement in/out of that cgroup
Sounds good.

> For the case of rebuild_sched_domains, we need the first of these
> options. This lock would be taken in cgroup.c at the points where it
> attached and removed cgroups from a cgroup tree, and could be taken by
> something like cpusets that needed to keep the hierarchy stable while
> scanning it. I think it would be fine to make it a mutex rather than a
> spinlock.
Agree

> cpu_hotplug.lock has to nest outside this hierarchy lock due to it
> being taken at the root of the hotplug/unplug path. So as long as we
> can ensure that we can always nest the hierarchy lock inside any
> get_online_cpus()/put_online_cpus() pairs, we should be OK.
Yes. Although that basically means that we always have to take 
cpu_hotplug.lock before hierarchy lock.

I like the proposal in general. Specifically for the 
rebuild_sched_domain() I'm now thinking that maybe we can get away with 
not involving cpuset at all. I think that what Peter meant originally. 
I'll send more thoughts on this separately.

Max