On 01/21/2013 12:34 PM, Michal Hocko wrote:
> On Mon 21-01-13 11:58:49, Glauber Costa wrote:
>> On 01/18/2013 08:06 PM, Michal Hocko wrote:
>>>> + /* bounce at first found */
>>>>> + for_each_mem_cgroup_tree(iter, memcg) {
>>> This will not work. Consider you will see a !online memcg. What happens?
>>> mem_cgroup_iter will css_get group that it returns and css_put it when
>>> it visits another one or finishes the loop. So your poor iter will be
>>> released before it gets born. Not good.
>>>
>> Reading this again, I don't really follow. The iterator is not supposed
>> to put() anything it hasn't get()'d before, so we will never release the
>> group. Note that if it ever appears in here, the css refcnt is expected
>> to be at least 1 already.
>>
>> The online test relies on the memcg refcnt, not on the css refcnt.
>
> Bahh, yeah, sorry about the confusion. Damn, it's not the first time I
> managed to mix those two...
>
You're excused. This time.
>> Actually, now that the value setting is all done in css_online, the css
>> refcnt should be enough to denote if the cgroup already has children,
>> without a memcg-specific test. The css refcnt is bumped somewhere
>> between alloc and online.
>
> Yes, in init_cgroup_css.
Yes, but that should not matter. We should not depend on anything more
general than "between alloc and online".
>
>> Unless Tejun objects it, I think I will just get rid of the online
>> test, and rely on the fact that if the iterator sees any children, we
>> should already online.
>
> Which means that we are back to list_empty(&cgrp->children) test, aren't
> we.
As long as cgroup core keeps using a list yes.
The css itself won't go away, regardless of the infrastructure cgroup
uses internally. So I do believe this is stronger long term.
Note that we have been arguing here about the css_refcnt, but we don't
actually refer to it: we do css_get and let cgroup core do whatever it
pleases it internally.
> If you really insist on not using
> children directly then do something like:
> struct cgroup *pos;
>
> if (!memcg->use_hierarchy)
> cgroup_for_each_child(pos, memcg->css.cgroup)
> return true;
>
> return false;
>
I don't oppose that.
> This still has an issue that a change (e.g. vm_swappiness) that requires
> this check will fail even though the child creation fails after it is
> made visible (e.g. during css_online).
>
Is it a problem ?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>