Re: [PATCH 2/2] memcg: first step towards hierarchical controller

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

On 06/26/2012 08:15 PM, Michal Hocko wrote:
> On Tue 26-06-12 19:47:14, Glauber Costa wrote:
>> Okay, so after recent discussions, I am proposing the following
>> patch. It won't remove hierarchy, or anything like that. Just default
>> to true in the root cgroup, and print a warning once if you try
>> to set it back to 0.
>>
>> I am not adding it to feature-removal-schedule.txt because I don't
>> view it as a consensus. Rather, changing the default would allow us
>> to give it a time around in the open, and see if people complain
>> and what we can learn about that.
>>
>> Signed-off-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> Acked-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
>> CC: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
>> CC: Kamezawa Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
>> CC: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>>   mm/memcontrol.c |    5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 85f7790..c37e4c1 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3993,6 +3993,10 @@ static int mem_cgroup_hierarchy_write(struct cgroup *cont, struct cftype *cft,
>>   	if (memcg->use_hierarchy == val)
>>   		goto out;
>>
>> +	WARN_ONCE(!parent_memcg && memcg->use_hierarchy,
>
> Do you have to test anything here at all? The test above will get you
> out without doing anything if you are not trying to change anything.
> The default is true so you have to be trying to disable it.
>
> If you omit !parent_memcg test as well you will get a bonus of the early
> warning even if somebody has cgconfig.conf like this:
>
> 	group a/b/c {
> 		memory {
> 			memory.use_hierarchy = 0;
> 			[...]
> 		}
> 	}
>
> which worked previously...
> True there is a risk of a "false warning" when somebody just tries to
> set disable hierarchy when it is (and never was) allowed but I do not
> think this is that bad.


Well, a false warning is not that bad.
It is better to be vocal.

I will wait for Kame to put his comments, and I can resend with that change.