Re: [PATCH 6/6] Makes procs file writable to move all threads by tgid at once

[Li Zefan <lizf@cn.fujitsu.com>]

Benjamin Blum wrote:
> On Mon, Aug 3, 2009 at 6:09 PM, Li Zefan<lizf@cn.fujitsu.com> wrote:
>> Benjamin Blum wrote:
>>> On Sun, Aug 2, 2009 at 8:00 PM, Li Zefan<lizf@cn.fujitsu.com> wrote:
>>>> Ben Blum wrote:
>>>>> +     }
>>>>> +     write_unlock(&css_set_lock);
>>>>> +
>>>>> +     /*
>>>>> +      * We just gained a reference on oldcg by taking it from the task. As
>>>> This comment is incorrect, the ref we just got has been dropped by
>>>> the above put_css_set(oldcg).
>>> No, the idea is that even though we had a reference that we already
>>> dropped, we in effect "traded" the newcg to the task for its oldcg,
>>> giving it our reference on newcg and gaining its reference on oldcg. I
>>> believe the cgroup_mutex guarantees that it'll still be there when we
>>> do the trade - perhaps a BUG_ON(tsk->cgroups != oldcg) is wanted
>>> inside the second task_lock section there? At the very least, a
>>> clearer comment.
>>>
>> Maybe my English sucks..
>>
>> By "gained a reference", doesn't it mean get_css_set()? But this
>> put_css_set() is not against the get() just called.
> 
> not in the conventional way, no. the comment there is bad enough that
> this is unclear: before trading pointers, the task had a reference on
> its tsk->cgroups pointer (same as our oldcg pointer), which is what we
> are overwriting with newcg. the task will think that the reference it
> has is still on tsk->cgroups, but since the pointer has changed, its
> reference also changes to a reference on newcg - one that this
> function took care of getting for the task. additionally, now that the
> task's reference is no longer for oldcg, we have to take care of the
> refcount that still thinks it's being used.
> 

Ok.

>> And in fact the ref can be 0 before this put(), because task_exit
>> can drop the last ref, but put_css_set() will check this case,
>> so it's Ok.
> 
> the check for PF_EXITING precludes that case.
> 

No. Note task exiting is not protected by cgroup_lock, so this can
happen:

                               | cgroup_attach_task()
                               |   oldcg = tsk->cgroups;
                               |   (tasks->flags & TASK_EXISING == 0)
                               |   rcu_assign_pointer(tsk->cgroups, newcg);
cgroup_exit()                  |
  oldcg = tsk->cgroups;        |
  put_css_set_taskexit(oldcg); |
  (now ref of olcg is 0)       |
                               |   put_css_set(oldcg);