Message-ID: <486523E6.4030201@qualcomm.com>
Date: Fri, 27 Jun 2008 10:31:18 -0700
From: Max Krasnyansky <maxk@qualcomm.com>
User-Agent: Thunderbird 2.0.0.14 (X11/20080501)
MIME-Version: 1.0
To: Paul Menage <menage@google.com>
CC: Vegard Nossum <vegard.nossum@gmail.com>, Paul Jackson <pj@sgi.com>,
       a.p.zijlstra@chello.nl, linux-kernel@vger.kernel.org,
       Gautham shenoy <ego@in.ibm.com>
Subject: Re: [RFC][PATCH] CPUSets: Move most calls to rebuild_sched_domains()
 to the workqueue
References: <48634BC1.8@google.com>	 <19f34abd0806260234y7616bab2k54bc019dfb0c6305@mail.gmail.com>	 <6599ad830806260250m39d700a5haf0f32d999cd2129@mail.gmail.com>	 <4863E4C8.9050705@qualcomm.com>	 <6599ad830806261334y6def5f7an57ac8f071a08eb4b@mail.gmail.com>	 <6599ad830806261417u3015a9b2i6318841de866d768@mail.gmail.com>	 <48647652.5050001@qualcomm.com> <6599ad830806262251x3a0747b9q6f143cc2fcd0aa57@mail.gmail.com>
In-Reply-To: <6599ad830806262251x3a0747b9q6f143cc2fcd0aa57@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2981
Lines: 71

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/