From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Hocko Subject: Re: [PATCH cgroup/for-3.7] cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them Date: Thu, 13 Sep 2012 20:46:53 +0200 Message-ID: <20120913184653.GA22529@dhcp22.suse.cz> References: <20120913183402.GG7677@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20120913183402.GG7677-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Tejun Heo Cc: Neil Horman , "Serge E. Hallyn" , containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Paul Mackerras , "Aneesh Kumar K.V" , Arnaldo Carvalho de Melo , Johannes Weiner , Thomas Graf , cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Paul Turner , Ingo Molnar , Vivek Goyal On Thu 13-09-12 11:34:02, Tejun Heo wrote: [...] > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -4075,8 +4075,18 @@ static long cgroup_create(struct cgroup > set_bit(CGRP_CLONE_CHILDREN, &cgrp->flags); > > for_each_subsys(root, ss) { > - struct cgroup_subsys_state *css = ss->create(cgrp); > + struct cgroup_subsys_state *css; > + > + if (ss->broken_hierarchy && !ss->warned_broken_hierarchy && > + parent->parent) { > + pr_warning("cgroup: %s (%d) created nested cgroup for controller \"%s\" which has incomplete hierarchy support. Nested cgroups may change behavior in the future.\n", > + current->comm, current->pid, ss->name); > + if (!strcmp(ss->name, "memory")) > + pr_warning("cgroup: \"memory\" requires setting use_hierarchy to 1 on the root.\n"); > + ss->warned_broken_hierarchy = true; > + } > > + css = ss->create(cgrp); As I said in other email. We have to move the warning after the create callback or you will not give any chance to the controller to say whether broken_hierarchy is really the case here. [...] > --- 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; This is not necessary because mem_cgroup_create will take care of the proper broken_hierarchy setting. > + } else { > retval = -EBUSY; > - } else > + } > + } else { > retval = -EINVAL; > + } > > out: > cgroup_unlock(); > @@ -4973,6 +4978,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; This should be sufficient. > } > memcg->last_scanned_node = MAX_NUMNODES; > INIT_LIST_HEAD(&memcg->oom_notify); -- Michal Hocko SUSE Labs