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

[Benjamin Blum]

On Mon, Aug 3, 2009 at 6:09 PM, Li Zefan<lizf at cn.fujitsu.com> wrote:
> Benjamin Blum wrote:
>> On Sun, Aug 2, 2009 at 8:00 PM, Li Zefan<lizf at 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.

> 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.

>>>> +static int css_set_check_fetched(struct cgroup *cgrp, struct task_struct *tsk,
>>>> +                              struct css_set *cg,
>>>> +                              struct list_head *newcg_list)
>>>> +{
>>>> +     struct css_set *newcg;
>>>> +     struct cg_list_entry *cg_entry;
>>>> +     struct cgroup_subsys_state *template[CGROUP_SUBSYS_COUNT];
>>>> +     read_lock(&css_set_lock);
>>>> +     newcg = find_existing_css_set(cg, cgrp, template);
>>>> +     if (newcg)
>>>> +             get_css_set(newcg);
>>>> +     read_unlock(&css_set_lock);
>>>> +     /* doesn't exist at all? */
>>>> +     if (!newcg)
>>>> +             return 1;
>>> I think it's more intuitive to return 1 if found and 0 if not found.
>>
>> I was sticking with the convention of nonzero return values indicating
>> failure, as is used everywhere else in this context.
>>
>
> Quoted from Documentation/CodingStyle:
>
> ...Such a value can be represented as an error-code integer
> (-Exxx = failure, 0 = success) or a "succeeded" boolean (0 = failure,
> non-zero = success).

sure. a boolean return value will be better here.
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers