Re: [PATCH 4/4] memcg: replace cgroup_lock with memcg specific memcg_lock

[Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]

On 12/03/2012 09:30 PM, Michal Hocko wrote:
> On Mon 03-12-12 18:15:32, Michal Hocko wrote:
> [...]
>>> @@ -3915,7 +3926,7 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
>>>  	 */
>>>  	if ((!parent_memcg || !parent_memcg->use_hierarchy) &&
>>>  				(val == 1 || val == 0)) {
>>> -		if (list_empty(&cont->children))
>>> +		if (!memcg_has_children(memcg))
>>>  			memcg->use_hierarchy = val;
>>>  		else
>>>  			retval = -EBUSY;
>>
>> Nothing prevents from a race when a task is on the way to be attached to
>> the group. This means that we might miss some charges up the way to the
>> parent.
>>
>> mem_cgroup_hierarchy_write
>>   					cgroup_attach_task
>> 					  ss->can_attach() = mem_cgroup_can_attach
>> 					    mutex_lock(&memcg_lock)
>> 					    memcg->attach_in_progress++
>> 					    mutex_unlock(&memcg_lock)
>> 					    __mem_cgroup_can_attach
>> 					      mem_cgroup_precharge_mc (*)
>>   mutex_lock(memcg_lock)
>>   memcg_has_children(memcg)==false
> 
> Dohh, retard alert. I obviously mixed tasks and children cgroups here.
> Why I thought we also do check for no tasks in the group? Ahh, because
> we should, at least here otherwise parent could see more uncharges than
> charges.
> But that deserves a separate patch. Sorry, for the confusion.

That is okay, here is a beer for you: http://bit.ly/R3rkML