Re: [PATCH] cpuset semaphore depth check optimize

Roman Zippel
2005-09-14T15:57:10Z

Hi,

On Tue, 13 Sep 2005, Paul Jackson wrote:

> > If I read the source correctly, a cpuset cannot be removed or moved while
> > it's attached to a task, which makes it a lot simpler.
>
> Yes - a cpuset cannot be removed while attached (count > 0). And there
> is no 'move' that I know of. A rename(2) system call on a cpuset in
> the cpuset filesystem gets the vfs default -EINVAL response.
>
> So, yes, if I can pin a cpuset with a per-cpuset spinlock on it, then
> its parent chain (and whatever else I take care to guard with that
> spinlock) is held as well (the cpuset->parent chain is pinned). I
> guess this some of what you meant by your phrase "makes it a lot
> simpler".

I don't think a per-cpuset spinlock will be necessary (at least
initially).
The complete active condition is actually (atomic_read(&cs->count) ||
!list_empty(&cs->children)). These means if any child is possibly active
so is the parent.
Modifications in the cpuset hierarchy require the cpuset_sem and an
inactive cpuset, (de)activating a cpuset requires the cpuset_sem and
(let's call it) cpuset_tasklock.
Callbacks from the allocator now only need cpuset_tasklock to access the
cpuset via tsk->cpuset and to keep it active and an active cpuset can't be
removed from the hierarchy.

> You also wrote:
> > You can BTW avoid locking in cpuset_exit() completely in the common case:
> >
> > tsk->cpuset = NULL;
> > if (atomic_dec_and_test(&cs->count) && notify_on_release(cs)) {
>
> I don't think that works. And I suspect you are proposing the same bug
> that I had, and fixed with the following patch:

You're right, it should better look like this:

tsk->cpuset = NULL;
if (atomic_read(&cs->count) == 1 && notify_on_release(cs)) {
...
}
atomic_dec(&cs->count);

This way it only may happen that two notifaction are sent.

bye, Roman