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.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
