On Wed 12-09-12 13:31:55, Glauber Costa wrote:
> 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.
Defintely. And that what the above guarantess, doesn't it?
--
Michal Hocko
SUSE Labs
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
