Re: [PATCH REPOST RFC cgroup/for-3.7] cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them

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

On 09/11/2012 02:04 PM, Michal Hocko wrote:
> I like the approach in general but see the comments bellow:
> 
> On Mon 10-09-12 15:33:55, Tejun Heo wrote:
> [...]
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3855,12 +3855,17 @@ static int mem_cgroup_hierarchy_write(st
>>  	 */
>>  	if ((!parent_memcg || !parent_memcg->use_hierarchy) &&
>>  				(val == 1 || val == 0)) {
>> -		if (list_empty(&cont->children))
>> +		if (list_empty(&cont->children)) {
>>  			memcg->use_hierarchy = val;
>> -		else
>> +			/* we're fully hierarchical iff root uses hierarchy */
>> +			if (mem_cgroup_is_root(memcg))
>> +				mem_cgroup_subsys.broken_hierarchy = !val;
>> +		} else {
>>  			retval = -EBUSY;
>> -	} else
>> +		}
>> +	} else {
>>  		retval = -EINVAL;
>> +	}
>>  
>>  out:
>>  	cgroup_unlock();
>> @@ -4953,6 +4958,7 @@ mem_cgroup_create(struct cgroup *cont)
>>  						&per_cpu(memcg_stock, cpu);
>>  			INIT_WORK(&stock->work, drain_local_stock);
>>  		}
>> +		mem_cgroup_subsys.broken_hierarchy = !memcg->use_hierarchy;
> 
> Hmmm, this will warn even if we have
> root (default use_hierarchy=0)
>  \
>   A (use_hierarchy=1)
>    \
>     B <- here
> 
> which is unfortunate because it will add a noise to a reasonable
> configuration.
> I think this is fixable if you move the warning after 
> cgroup_subsys_state::create and broken_hierarchy would be set only if
> parent is not root and use_hierarchy==0 in mem_cgroup_create. Something
> like:
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 795e525..d5c93ab 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4973,6 +4973,13 @@ mem_cgroup_create(struct cgroup *cont)
>  	} else {
>  		res_counter_init(&memcg->res, NULL);
>  		res_counter_init(&memcg->memsw, NULL);
> +		/*
> +		 * Deeper hierachy with use_hierarchy == false doesn't make
> +		 * much sense so let cgroup subsystem know about this unfortunate
> +		 * state in our controller.
> +		 */
> +		if (parent && parent != root_mem_cgroup)
> +			mem_cgroup_subsys.broken_hierarchy = true;
>  	}
>  	memcg->last_scanned_node = MAX_NUMNODES;
>  	INIT_LIST_HEAD(&memcg->oom_notify);
> 
> What do you think?
> 
I side with Tejun's original intentions.

While I respect your goal of not warning about any configuration
with max_level = 1, I believe the only sane configuration as soon
as we get any 2nd-level child is use_hierarchy = 1 for everybody.

Everything aside from it should be warned.