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

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

[Tejun Heo]

Currently, cgroup hierarchy support is a mess.  cpu related subsystems
behave correctly - configuration, accounting and control on a parent
properly cover its children.  blkio and freezer completely ignore
hierarchy and treat all cgroups as if they're directly under the root
cgroup.  Others show yet different behaviors.

These differing interpretations of cgroup hierarchy make using cgroup
confusing and it impossible to co-mount controllers into the same
hierarchy and obtain sane behavior.

Eventually, we want full hierarchy support from all subsystems and
probably a unified hierarchy.  Users using separate hierarchies
expecting completely different behaviors depending on the mounted
subsystem is deterimental to making any progress on this front.

This patch adds cgroup_subsys.broken_hierarchy and sets it to %true
for controllers which are lacking in hierarchy support.  The goal of
this patch is two-fold.

* Move users away from using hierarchy on currently non-hierarchical
  subsystems, so that implementing proper hierarchy support on those
  doesn't surprise them.

* Keep track of which controllers are broken how and nudge the
  subsystems to implement proper hierarchy support.

For now, start with a single warning message.  We can whine louder
later on.

v2: Fixed a typo spotted by Michal. Warning message updated.

v3: Updated memcg part so that it doesn't generate warning in the
    cases where .use_hierarchy=false doesn't make the behavior
    different from root.use_hierarchy=true.  Fixed a typo spotted by
    Glauber.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
Cc: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
Cc: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Cc: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Arnaldo Carvalho de Melo <acme-f8uhVLnGfZaxAyOMLChx1axOck334EZe@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Aneesh Kumar K.V <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
Okay, I'm gonna push this through for for-3.7.  Please scream soon if
you disagree with the patch or the direction we're headed; otherwise,
we're going this way.  Li, Michal, Glauber, if you're okay with this,
please ack.

Thanks guys.

 block/blk-cgroup.c        |    8 ++++++++
 include/linux/cgroup.h    |   15 +++++++++++++++
 kernel/cgroup.c           |   12 +++++++++++-
 kernel/cgroup_freezer.c   |    8 ++++++++
 kernel/events/core.c      |    7 +++++++
 mm/memcontrol.c           |   18 +++++++++++++++---
 net/core/netprio_cgroup.c |   12 +++++++++++-
 net/sched/cls_cgroup.c    |    9 +++++++++
 security/device_cgroup.c  |    9 +++++++++
 9 files changed, 93 insertions(+), 5 deletions(-)

--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -737,6 +737,14 @@ struct cgroup_subsys blkio_subsys = {
 	.subsys_id = blkio_subsys_id,
 	.base_cftypes = blkcg_files,
 	.module = THIS_MODULE,
+
+	/*
+	 * blkio subsystem is utterly broken in terms of hierarchy support.
+	 * It treats all cgroups equally regardless of where they're
+	 * located in the hierarchy - all cgroups are treated as if they're
+	 * right below the root.  Fix it and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 EXPORT_SYMBOL_GPL(blkio_subsys);
 
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -503,6 +503,21 @@ struct cgroup_subsys {
 	 */
 	bool __DEPRECATED_clear_css_refs;
 
+	/*
+	 * If %false, this subsystem is properly hierarchical -
+	 * configuration, resource accounting and restriction on a parent
+	 * cgroup cover those of its children.  If %true, hierarchy support
+	 * is broken in some ways - some subsystems ignore hierarchy
+	 * completely while others are only implemented half-way.
+	 *
+	 * It's now disallowed to create nested cgroups if the subsystem is
+	 * broken and cgroup core will emit a warning message on such
+	 * cases.  Eventually, all subsystems will be made properly
+	 * hierarchical and this will go away.
+	 */
+	bool broken_hierarchy;
+	bool warned_broken_hierarchy;
+
 #define MAX_CGROUP_TYPE_NAMELEN 32
 	const char *name;
 
--- 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);
 		if (IS_ERR(css)) {
 			err = PTR_ERR(css);
 			goto err_destroy;
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -373,4 +373,12 @@ struct cgroup_subsys freezer_subsys = {
 	.can_attach	= freezer_can_attach,
 	.fork		= freezer_fork,
 	.base_cftypes	= files,
+
+	/*
+	 * freezer subsys doesn't handle hierarchy at all.  Frozen state
+	 * should be inherited through the hierarchy - if a parent is
+	 * frozen, all its children should be frozen.  Fix it and remove
+	 * the following.
+	 */
+	.broken_hierarchy = true,
 };
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7285,5 +7285,12 @@ struct cgroup_subsys perf_subsys = {
 	.destroy	= perf_cgroup_destroy,
 	.exit		= perf_cgroup_exit,
 	.attach		= perf_cgroup_attach,
+
+	/*
+	 * perf_event cgroup doesn't handle nesting correctly.
+	 * ctx->nr_cgroups adjustments should be propagated through the
+	 * cgroup hierarchy.  Fix it and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 #endif /* CONFIG_CGROUP_PERF */
--- 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();
@@ -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;
 	}
 	memcg->last_scanned_node = MAX_NUMNODES;
 	INIT_LIST_HEAD(&memcg->oom_notify);
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -330,7 +330,17 @@ struct cgroup_subsys net_prio_subsys = {
 	.subsys_id	= net_prio_subsys_id,
 #endif
 	.base_cftypes	= ss_files,
-	.module		= THIS_MODULE
+	.module		= THIS_MODULE,
+
+	/*
+	 * net_prio has artificial limit on the number of cgroups and
+	 * disallows nesting making it impossible to co-mount it with other
+	 * hierarchical subsystems.  Remove the artificially low PRIOIDX_SZ
+	 * limit and properly nest configuration such that children follow
+	 * their parents' configurations by default and are allowed to
+	 * override and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 
 static int netprio_device_event(struct notifier_block *unused,
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -82,6 +82,15 @@ struct cgroup_subsys net_cls_subsys = {
 #endif
 	.base_cftypes	= ss_files,
 	.module		= THIS_MODULE,
+
+	/*
+	 * While net_cls cgroup has the rudimentary hierarchy support of
+	 * inheriting the parent's classid on cgroup creation, it doesn't
+	 * properly propagates config changes in ancestors to their
+	 * descendents.  A child should follow the parent's configuration
+	 * but be allowed to override it.  Fix it and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 
 struct cls_cgroup_head {
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -457,6 +457,15 @@ struct cgroup_subsys devices_subsys = {
 	.destroy = devcgroup_destroy,
 	.subsys_id = devices_subsys_id,
 	.base_cftypes = dev_cgroup_files,
+
+	/*
+	 * While devices cgroup has the rudimentary hierarchy support which
+	 * checks the parent's restriction, it doesn't properly propagates
+	 * config changes in ancestors to their descendents.  A child
+	 * should only be allowed to add more restrictions to the parent's
+	 * configuration.  Fix it and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 
 int __devcgroup_inode_permission(struct inode *inode, int mask)

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

[Michal Hocko]

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

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

[Tejun Heo]

Currently, cgroup hierarchy support is a mess.  cpu related subsystems
behave correctly - configuration, accounting and control on a parent
properly cover its children.  blkio and freezer completely ignore
hierarchy and treat all cgroups as if they're directly under the root
cgroup.  Others show yet different behaviors.

These differing interpretations of cgroup hierarchy make using cgroup
confusing and it impossible to co-mount controllers into the same
hierarchy and obtain sane behavior.

Eventually, we want full hierarchy support from all subsystems and
probably a unified hierarchy.  Users using separate hierarchies
expecting completely different behaviors depending on the mounted
subsystem is deterimental to making any progress on this front.

This patch adds cgroup_subsys.broken_hierarchy and sets it to %true
for controllers which are lacking in hierarchy support.  The goal of
this patch is two-fold.

* Move users away from using hierarchy on currently non-hierarchical
  subsystems, so that implementing proper hierarchy support on those
  doesn't surprise them.

* Keep track of which controllers are broken how and nudge the
  subsystems to implement proper hierarchy support.

For now, start with a single warning message.  We can whine louder
later on.

v2: Fixed a typo spotted by Michal. Warning message updated.

v3: Updated memcg part so that it doesn't generate warning in the
    cases where .use_hierarchy=false doesn't make the behavior
    different from root.use_hierarchy=true.  Fixed a typo spotted by
    Glauber.

v4: Check ->broken_hierarchy after cgroup creation is complete so that
    ->create() can affect the result per Michal.  Dropped unnecessary
    memcg root handling per Michal.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
Cc: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
Cc: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Cc: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Arnaldo Carvalho de Melo <acme-f8uhVLnGfZaxAyOMLChx1axOck334EZe@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: Aneesh Kumar K.V <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 block/blk-cgroup.c        |    8 ++++++++
 include/linux/cgroup.h    |   15 +++++++++++++++
 kernel/cgroup.c           |   12 +++++++++++-
 kernel/cgroup_freezer.c   |    8 ++++++++
 kernel/events/core.c      |    7 +++++++
 mm/memcontrol.c           |    7 +++++++
 net/core/netprio_cgroup.c |   12 +++++++++++-
 net/sched/cls_cgroup.c    |    9 +++++++++
 security/device_cgroup.c  |    9 +++++++++
 9 files changed, 85 insertions(+), 2 deletions(-)

--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -737,6 +737,14 @@ struct cgroup_subsys blkio_subsys = {
 	.subsys_id = blkio_subsys_id,
 	.base_cftypes = blkcg_files,
 	.module = THIS_MODULE,
+
+	/*
+	 * blkio subsystem is utterly broken in terms of hierarchy support.
+	 * It treats all cgroups equally regardless of where they're
+	 * located in the hierarchy - all cgroups are treated as if they're
+	 * right below the root.  Fix it and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 EXPORT_SYMBOL_GPL(blkio_subsys);
 
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -503,6 +503,21 @@ struct cgroup_subsys {
 	 */
 	bool __DEPRECATED_clear_css_refs;
 
+	/*
+	 * If %false, this subsystem is properly hierarchical -
+	 * configuration, resource accounting and restriction on a parent
+	 * cgroup cover those of its children.  If %true, hierarchy support
+	 * is broken in some ways - some subsystems ignore hierarchy
+	 * completely while others are only implemented half-way.
+	 *
+	 * It's now disallowed to create nested cgroups if the subsystem is
+	 * broken and cgroup core will emit a warning message on such
+	 * cases.  Eventually, all subsystems will be made properly
+	 * hierarchical and this will go away.
+	 */
+	bool broken_hierarchy;
+	bool warned_broken_hierarchy;
+
 #define MAX_CGROUP_TYPE_NAMELEN 32
 	const char *name;
 
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4075,8 +4075,9 @@ 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;
 
+		css = ss->create(cgrp);
 		if (IS_ERR(css)) {
 			err = PTR_ERR(css);
 			goto err_destroy;
@@ -4090,6 +4091,15 @@ static long cgroup_create(struct cgroup 
 		/* At error, ->destroy() callback has to free assigned ID. */
 		if (clone_children(parent) && ss->post_clone)
 			ss->post_clone(cgrp);
+
+		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;
+		}
 	}
 
 	list_add(&cgrp->sibling, &cgrp->parent->children);
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -373,4 +373,12 @@ struct cgroup_subsys freezer_subsys = {
 	.can_attach	= freezer_can_attach,
 	.fork		= freezer_fork,
 	.base_cftypes	= files,
+
+	/*
+	 * freezer subsys doesn't handle hierarchy at all.  Frozen state
+	 * should be inherited through the hierarchy - if a parent is
+	 * frozen, all its children should be frozen.  Fix it and remove
+	 * the following.
+	 */
+	.broken_hierarchy = true,
 };
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7285,5 +7285,12 @@ struct cgroup_subsys perf_subsys = {
 	.destroy	= perf_cgroup_destroy,
 	.exit		= perf_cgroup_exit,
 	.attach		= perf_cgroup_attach,
+
+	/*
+	 * perf_event cgroup doesn't handle nesting correctly.
+	 * ctx->nr_cgroups adjustments should be propagated through the
+	 * cgroup hierarchy.  Fix it and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 #endif /* CONFIG_CGROUP_PERF */
--- 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);
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -330,7 +330,17 @@ struct cgroup_subsys net_prio_subsys = {
 	.subsys_id	= net_prio_subsys_id,
 #endif
 	.base_cftypes	= ss_files,
-	.module		= THIS_MODULE
+	.module		= THIS_MODULE,
+
+	/*
+	 * net_prio has artificial limit on the number of cgroups and
+	 * disallows nesting making it impossible to co-mount it with other
+	 * hierarchical subsystems.  Remove the artificially low PRIOIDX_SZ
+	 * limit and properly nest configuration such that children follow
+	 * their parents' configurations by default and are allowed to
+	 * override and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 
 static int netprio_device_event(struct notifier_block *unused,
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -82,6 +82,15 @@ struct cgroup_subsys net_cls_subsys = {
 #endif
 	.base_cftypes	= ss_files,
 	.module		= THIS_MODULE,
+
+	/*
+	 * While net_cls cgroup has the rudimentary hierarchy support of
+	 * inheriting the parent's classid on cgroup creation, it doesn't
+	 * properly propagates config changes in ancestors to their
+	 * descendents.  A child should follow the parent's configuration
+	 * but be allowed to override it.  Fix it and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 
 struct cls_cgroup_head {
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -457,6 +457,15 @@ struct cgroup_subsys devices_subsys = {
 	.destroy = devcgroup_destroy,
 	.subsys_id = devices_subsys_id,
 	.base_cftypes = dev_cgroup_files,
+
+	/*
+	 * While devices cgroup has the rudimentary hierarchy support which
+	 * checks the parent's restriction, it doesn't properly propagates
+	 * config changes in ancestors to their descendents.  A child
+	 * should only be allowed to add more restrictions to the parent's
+	 * configuration.  Fix it and remove the following.
+	 */
+	.broken_hierarchy = true,
 };
 
 int __devcgroup_inode_permission(struct inode *inode, int mask)

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

[Michal Hocko]

On Thu 13-09-12 12:20:58, Tejun Heo wrote:
> Currently, cgroup hierarchy support is a mess.  cpu related subsystems
> behave correctly - configuration, accounting and control on a parent
> properly cover its children.  blkio and freezer completely ignore
> hierarchy and treat all cgroups as if they're directly under the root
> cgroup.  Others show yet different behaviors.
> 
> These differing interpretations of cgroup hierarchy make using cgroup
> confusing and it impossible to co-mount controllers into the same
> hierarchy and obtain sane behavior.
> 
> Eventually, we want full hierarchy support from all subsystems and
> probably a unified hierarchy.  Users using separate hierarchies
> expecting completely different behaviors depending on the mounted
> subsystem is deterimental to making any progress on this front.
> 
> This patch adds cgroup_subsys.broken_hierarchy and sets it to %true
> for controllers which are lacking in hierarchy support.  The goal of
> this patch is two-fold.
> 
> * Move users away from using hierarchy on currently non-hierarchical
>   subsystems, so that implementing proper hierarchy support on those
>   doesn't surprise them.
> 
> * Keep track of which controllers are broken how and nudge the
>   subsystems to implement proper hierarchy support.
> 
> For now, start with a single warning message.  We can whine louder
> later on.
> 
> v2: Fixed a typo spotted by Michal. Warning message updated.
> 
> v3: Updated memcg part so that it doesn't generate warning in the
>     cases where .use_hierarchy=false doesn't make the behavior
>     different from root.use_hierarchy=true.  Fixed a typo spotted by
>     Glauber.
> 
> v4: Check ->broken_hierarchy after cgroup creation is complete so that
>     ->create() can affect the result per Michal.  Dropped unnecessary
>     memcg root handling per Michal.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
> Cc: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
> Cc: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Cc: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Arnaldo Carvalho de Melo <acme-f8uhVLnGfZaxAyOMLChx1axOck334EZe@public.gmane.org>
> Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Aneesh Kumar K.V <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Acked-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>

Thanks!

> ---
>  block/blk-cgroup.c        |    8 ++++++++
>  include/linux/cgroup.h    |   15 +++++++++++++++
>  kernel/cgroup.c           |   12 +++++++++++-
>  kernel/cgroup_freezer.c   |    8 ++++++++
>  kernel/events/core.c      |    7 +++++++
>  mm/memcontrol.c           |    7 +++++++
>  net/core/netprio_cgroup.c |   12 +++++++++++-
>  net/sched/cls_cgroup.c    |    9 +++++++++
>  security/device_cgroup.c  |    9 +++++++++
>  9 files changed, 85 insertions(+), 2 deletions(-)
> 
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -737,6 +737,14 @@ struct cgroup_subsys blkio_subsys = {
>  	.subsys_id = blkio_subsys_id,
>  	.base_cftypes = blkcg_files,
>  	.module = THIS_MODULE,
> +
> +	/*
> +	 * blkio subsystem is utterly broken in terms of hierarchy support.
> +	 * It treats all cgroups equally regardless of where they're
> +	 * located in the hierarchy - all cgroups are treated as if they're
> +	 * right below the root.  Fix it and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  EXPORT_SYMBOL_GPL(blkio_subsys);
>  
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -503,6 +503,21 @@ struct cgroup_subsys {
>  	 */
>  	bool __DEPRECATED_clear_css_refs;
>  
> +	/*
> +	 * If %false, this subsystem is properly hierarchical -
> +	 * configuration, resource accounting and restriction on a parent
> +	 * cgroup cover those of its children.  If %true, hierarchy support
> +	 * is broken in some ways - some subsystems ignore hierarchy
> +	 * completely while others are only implemented half-way.
> +	 *
> +	 * It's now disallowed to create nested cgroups if the subsystem is
> +	 * broken and cgroup core will emit a warning message on such
> +	 * cases.  Eventually, all subsystems will be made properly
> +	 * hierarchical and this will go away.
> +	 */
> +	bool broken_hierarchy;
> +	bool warned_broken_hierarchy;
> +
>  #define MAX_CGROUP_TYPE_NAMELEN 32
>  	const char *name;
>  
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -4075,8 +4075,9 @@ 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;
>  
> +		css = ss->create(cgrp);
>  		if (IS_ERR(css)) {
>  			err = PTR_ERR(css);
>  			goto err_destroy;
> @@ -4090,6 +4091,15 @@ static long cgroup_create(struct cgroup 
>  		/* At error, ->destroy() callback has to free assigned ID. */
>  		if (clone_children(parent) && ss->post_clone)
>  			ss->post_clone(cgrp);
> +
> +		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;
> +		}
>  	}
>  
>  	list_add(&cgrp->sibling, &cgrp->parent->children);
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -373,4 +373,12 @@ struct cgroup_subsys freezer_subsys = {
>  	.can_attach	= freezer_can_attach,
>  	.fork		= freezer_fork,
>  	.base_cftypes	= files,
> +
> +	/*
> +	 * freezer subsys doesn't handle hierarchy at all.  Frozen state
> +	 * should be inherited through the hierarchy - if a parent is
> +	 * frozen, all its children should be frozen.  Fix it and remove
> +	 * the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7285,5 +7285,12 @@ struct cgroup_subsys perf_subsys = {
>  	.destroy	= perf_cgroup_destroy,
>  	.exit		= perf_cgroup_exit,
>  	.attach		= perf_cgroup_attach,
> +
> +	/*
> +	 * perf_event cgroup doesn't handle nesting correctly.
> +	 * ctx->nr_cgroups adjustments should be propagated through the
> +	 * cgroup hierarchy.  Fix it and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  #endif /* CONFIG_CGROUP_PERF */
> --- 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);
> --- a/net/core/netprio_cgroup.c
> +++ b/net/core/netprio_cgroup.c
> @@ -330,7 +330,17 @@ struct cgroup_subsys net_prio_subsys = {
>  	.subsys_id	= net_prio_subsys_id,
>  #endif
>  	.base_cftypes	= ss_files,
> -	.module		= THIS_MODULE
> +	.module		= THIS_MODULE,
> +
> +	/*
> +	 * net_prio has artificial limit on the number of cgroups and
> +	 * disallows nesting making it impossible to co-mount it with other
> +	 * hierarchical subsystems.  Remove the artificially low PRIOIDX_SZ
> +	 * limit and properly nest configuration such that children follow
> +	 * their parents' configurations by default and are allowed to
> +	 * override and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  
>  static int netprio_device_event(struct notifier_block *unused,
> --- a/net/sched/cls_cgroup.c
> +++ b/net/sched/cls_cgroup.c
> @@ -82,6 +82,15 @@ struct cgroup_subsys net_cls_subsys = {
>  #endif
>  	.base_cftypes	= ss_files,
>  	.module		= THIS_MODULE,
> +
> +	/*
> +	 * While net_cls cgroup has the rudimentary hierarchy support of
> +	 * inheriting the parent's classid on cgroup creation, it doesn't
> +	 * properly propagates config changes in ancestors to their
> +	 * descendents.  A child should follow the parent's configuration
> +	 * but be allowed to override it.  Fix it and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  
>  struct cls_cgroup_head {
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -457,6 +457,15 @@ struct cgroup_subsys devices_subsys = {
>  	.destroy = devcgroup_destroy,
>  	.subsys_id = devices_subsys_id,
>  	.base_cftypes = dev_cgroup_files,
> +
> +	/*
> +	 * While devices cgroup has the rudimentary hierarchy support which
> +	 * checks the parent's restriction, it doesn't properly propagates
> +	 * config changes in ancestors to their descendents.  A child
> +	 * should only be allowed to add more restrictions to the parent's
> +	 * configuration.  Fix it and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  
>  int __devcgroup_inode_permission(struct inode *inode, int mask)

-- 
Michal Hocko
SUSE Labs

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

[Vivek Goyal]

On Thu, Sep 13, 2012 at 12:20:58PM -0700, Tejun Heo wrote:

[..]
> +
> +		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;
> +		}

Hi Tejun,

Had one more question. So this warning pops up if parent->parent is set.
So first level of cgroup creation is allowed but not the second level
one?

I think for blkio controller, first level of creation itself is wrong if
hierarchy is not supported. 

				root
			       / | \ 
                              T1 T2 G1

So T1 and T2 are tasks and G1 is the created cgroup. Now if we are
expecting hierarchical controller then like cpu controller, T1, T2 and
G1 should be considered at same level and disk bandwidth should be
divided equally between T1, T2 and G1.

But currently blkio controller looks at them as follows.

				pivot
				/  \
			      root G1
			      / \
			     T1  T2
and divides bandwidth between root and G1 equally (if weights are equal).

Now if you don't warn about first level creation and later change to default
hierarchical controller, anyway you will change the behavior underneath
(even if user space creates only one level of cgroup).

Point being, cgroup fs does not allow us to create flat hierarchy. So
warning about second level creation and beyond does not help much and
behavior underneath will change anyway(even for first level of creation).

Thanks
Vivek

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

[Tejun Heo]

Hello, Vivek.

On Thu, Sep 13, 2012 at 03:41:11PM -0400, Vivek Goyal wrote:
> I think for blkio controller, first level of creation itself is wrong if
> hierarchy is not supported. 
> 
> 				root
> 			       / | \ 
>                               T1 T2 G1
> 
> So T1 and T2 are tasks and G1 is the created cgroup. Now if we are
> expecting hierarchical controller then like cpu controller, T1, T2 and
> G1 should be considered at same level and disk bandwidth should be
> divided equally between T1, T2 and G1.
> 
> But currently blkio controller looks at them as follows.
> 
> 				pivot
> 				/  \
> 			      root G1
> 			      / \
> 			     T1  T2
> and divides bandwidth between root and G1 equally (if weights are equal).

Yeah, how to handle tasks and groups under the same cgroup is a
difficult question.  I don't think what blkcg does now is necessarily
wrong and am not particularly fond of the idea of dealing tasks and
groups as equals.  It isn't clear how resource should be distributed
across them - we don't have per-task knobs.

I think this is a problem only for controllers which implement
proportional control - cpu and blkio, am I right?

Thanks.

-- 
tejun

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

[Vivek Goyal]

On Thu, Sep 13, 2012 at 01:03:54PM -0700, Tejun Heo wrote:
> Hello, Vivek.
> 
> On Thu, Sep 13, 2012 at 03:41:11PM -0400, Vivek Goyal wrote:
> > I think for blkio controller, first level of creation itself is wrong if
> > hierarchy is not supported. 
> > 
> > 				root
> > 			       / | \ 
> >                               T1 T2 G1
> > 
> > So T1 and T2 are tasks and G1 is the created cgroup. Now if we are
> > expecting hierarchical controller then like cpu controller, T1, T2 and
> > G1 should be considered at same level and disk bandwidth should be
> > divided equally between T1, T2 and G1.
> > 
> > But currently blkio controller looks at them as follows.
> > 
> > 				pivot
> > 				/  \
> > 			      root G1
> > 			      / \
> > 			     T1  T2
> > and divides bandwidth between root and G1 equally (if weights are equal).
> 
> Yeah, how to handle tasks and groups under the same cgroup is a
> difficult question.  I don't think what blkcg does now is necessarily
> wrong and am not particularly fond of the idea of dealing tasks and
> groups as equals.  It isn't clear how resource should be distributed
> across them - we don't have per-task knobs.

IIUC cpu controller tied together the notion of task priority and
cgroup weight. We could do something similar where we map the task
io priority to some weight and then divide bandwidth according to
that. That give us some control on tasks.

In the past we had this discussion and consensus seemed to be go
the cpu controller way where task and groups are treated at same
level when it comes to bandwidth division. One can always move all
the tasks to one of the children groups if this behavior is not
desired. The only problem is that kernel does not seem to offer
any fool proof way of moving existing process to a cgroup (making
sure none of the threads escaped the movement). Possibly freezer
controller could help here but I am not sure how it would freeze
threads in root cgroup which might require to be moved out to a
children group.

> 
> I think this is a problem only for controllers which implement
> proportional control - cpu and blkio, am I right?

I think this might become a problem with throttling too if somebody was
throttling root group. Throttling root group is not recommended so that
might not be a big issue. (Though there is nothing in the code which
prevents it).

So you are right. Looks like cpu and block proportional control have this
issue. Memory controller might have handled it already using .use_hiearchy
i am assuming.

Thanks
Vivek

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

[Glauber Costa]

On 09/14/2012 12:03 AM, Tejun Heo wrote:
> Hello, Vivek.
> 
> On Thu, Sep 13, 2012 at 03:41:11PM -0400, Vivek Goyal wrote:
>> I think for blkio controller, first level of creation itself is wrong if
>> hierarchy is not supported. 
>>
>> 				root
>> 			       / | \ 
>>                               T1 T2 G1
>>

I side with Vivek, and I think we should warn first level. It just tends
to be wrong. If memcg is a special case, just special case it.
_but_, I see no reason not to do it.

While I agree with Michal point that this is not necessary, I don't
think being noisier than we need to be is a crime. Less noisy certainly
can be.

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

[Glauber Costa]

On 09/14/2012 01:14 PM, Glauber Costa wrote:
> On 09/14/2012 12:03 AM, Tejun Heo wrote:
>> Hello, Vivek.
>>
>> On Thu, Sep 13, 2012 at 03:41:11PM -0400, Vivek Goyal wrote:
>>> I think for blkio controller, first level of creation itself is wrong if
>>> hierarchy is not supported. 
>>>
>>> 				root
>>> 			       / | \ 
>>>                               T1 T2 G1
>>>
> 
> I side with Vivek, and I think we should warn first level. It just tends
> to be wrong. If memcg is a special case, just special case it.
> _but_, I see no reason not to do it.
> 
> While I agree with Michal point that this is not necessary, I don't
> think being noisier than we need to be is a crime. Less noisy certainly
> can be.
> 

To add to that: If it is "all the same", hierarchy should be set to 1.
If anything else, this will make people switch it to shut up the
warning, and prevent a potentially broken use case to appear in the future.

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

[Tejun Heo]

Hello, Glauber.

On Fri, Sep 14, 2012 at 01:17:14PM +0400, Glauber Costa wrote:
> > I side with Vivek, and I think we should warn first level. It just tends
> > to be wrong. If memcg is a special case, just special case it.
> > _but_, I see no reason not to do it.

The issue at hand is how to handle tasks vs. sub-cgroups in the same
cgroup.  memcg doesn't factor into this because it doesn't implement
proportional control.  No matter which way we decide, it doesn't
matter for memcg.

Thanks.

-- 
tejun

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

[Serge Hallyn]

Quoting Tejun Heo (tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org):
> Currently, cgroup hierarchy support is a mess.  cpu related subsystems
> behave correctly - configuration, accounting and control on a parent
> properly cover its children.  blkio and freezer completely ignore
> hierarchy and treat all cgroups as if they're directly under the root
> cgroup.  Others show yet different behaviors.
> 
> These differing interpretations of cgroup hierarchy make using cgroup
> confusing and it impossible to co-mount controllers into the same
> hierarchy and obtain sane behavior.
> 
> Eventually, we want full hierarchy support from all subsystems and
> probably a unified hierarchy.  Users using separate hierarchies
> expecting completely different behaviors depending on the mounted
> subsystem is deterimental to making any progress on this front.
> 
> This patch adds cgroup_subsys.broken_hierarchy and sets it to %true
> for controllers which are lacking in hierarchy support.  The goal of
> this patch is two-fold.
> 
> * Move users away from using hierarchy on currently non-hierarchical
>   subsystems, so that implementing proper hierarchy support on those
>   doesn't surprise them.
> 
> * Keep track of which controllers are broken how and nudge the
>   subsystems to implement proper hierarchy support.
> 
> For now, start with a single warning message.  We can whine louder
> later on.
> 
> v2: Fixed a typo spotted by Michal. Warning message updated.
> 
> v3: Updated memcg part so that it doesn't generate warning in the
>     cases where .use_hierarchy=false doesn't make the behavior
>     different from root.use_hierarchy=true.  Fixed a typo spotted by
>     Glauber.
> 
> v4: Check ->broken_hierarchy after cgroup creation is complete so that
>     ->create() can affect the result per Michal.  Dropped unnecessary
>     memcg root handling per Michal.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
> Cc: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
> Cc: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

> Cc: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Arnaldo Carvalho de Melo <acme-f8uhVLnGfZaxAyOMLChx1axOck334EZe@public.gmane.org>
> Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Aneesh Kumar K.V <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  block/blk-cgroup.c        |    8 ++++++++
>  include/linux/cgroup.h    |   15 +++++++++++++++
>  kernel/cgroup.c           |   12 +++++++++++-
>  kernel/cgroup_freezer.c   |    8 ++++++++
>  kernel/events/core.c      |    7 +++++++
>  mm/memcontrol.c           |    7 +++++++
>  net/core/netprio_cgroup.c |   12 +++++++++++-
>  net/sched/cls_cgroup.c    |    9 +++++++++
>  security/device_cgroup.c  |    9 +++++++++
>  9 files changed, 85 insertions(+), 2 deletions(-)
> 
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -737,6 +737,14 @@ struct cgroup_subsys blkio_subsys = {
>  	.subsys_id = blkio_subsys_id,
>  	.base_cftypes = blkcg_files,
>  	.module = THIS_MODULE,
> +
> +	/*
> +	 * blkio subsystem is utterly broken in terms of hierarchy support.
> +	 * It treats all cgroups equally regardless of where they're
> +	 * located in the hierarchy - all cgroups are treated as if they're
> +	 * right below the root.  Fix it and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  EXPORT_SYMBOL_GPL(blkio_subsys);
>  
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -503,6 +503,21 @@ struct cgroup_subsys {
>  	 */
>  	bool __DEPRECATED_clear_css_refs;
>  
> +	/*
> +	 * If %false, this subsystem is properly hierarchical -
> +	 * configuration, resource accounting and restriction on a parent
> +	 * cgroup cover those of its children.  If %true, hierarchy support
> +	 * is broken in some ways - some subsystems ignore hierarchy
> +	 * completely while others are only implemented half-way.
> +	 *
> +	 * It's now disallowed to create nested cgroups if the subsystem is
> +	 * broken and cgroup core will emit a warning message on such
> +	 * cases.  Eventually, all subsystems will be made properly
> +	 * hierarchical and this will go away.
> +	 */
> +	bool broken_hierarchy;
> +	bool warned_broken_hierarchy;
> +
>  #define MAX_CGROUP_TYPE_NAMELEN 32
>  	const char *name;
>  
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -4075,8 +4075,9 @@ 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;
>  
> +		css = ss->create(cgrp);
>  		if (IS_ERR(css)) {
>  			err = PTR_ERR(css);
>  			goto err_destroy;
> @@ -4090,6 +4091,15 @@ static long cgroup_create(struct cgroup 
>  		/* At error, ->destroy() callback has to free assigned ID. */
>  		if (clone_children(parent) && ss->post_clone)
>  			ss->post_clone(cgrp);
> +
> +		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;
> +		}
>  	}
>  
>  	list_add(&cgrp->sibling, &cgrp->parent->children);
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -373,4 +373,12 @@ struct cgroup_subsys freezer_subsys = {
>  	.can_attach	= freezer_can_attach,
>  	.fork		= freezer_fork,
>  	.base_cftypes	= files,
> +
> +	/*
> +	 * freezer subsys doesn't handle hierarchy at all.  Frozen state
> +	 * should be inherited through the hierarchy - if a parent is
> +	 * frozen, all its children should be frozen.  Fix it and remove
> +	 * the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7285,5 +7285,12 @@ struct cgroup_subsys perf_subsys = {
>  	.destroy	= perf_cgroup_destroy,
>  	.exit		= perf_cgroup_exit,
>  	.attach		= perf_cgroup_attach,
> +
> +	/*
> +	 * perf_event cgroup doesn't handle nesting correctly.
> +	 * ctx->nr_cgroups adjustments should be propagated through the
> +	 * cgroup hierarchy.  Fix it and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  #endif /* CONFIG_CGROUP_PERF */
> --- 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);
> --- a/net/core/netprio_cgroup.c
> +++ b/net/core/netprio_cgroup.c
> @@ -330,7 +330,17 @@ struct cgroup_subsys net_prio_subsys = {
>  	.subsys_id	= net_prio_subsys_id,
>  #endif
>  	.base_cftypes	= ss_files,
> -	.module		= THIS_MODULE
> +	.module		= THIS_MODULE,
> +
> +	/*
> +	 * net_prio has artificial limit on the number of cgroups and
> +	 * disallows nesting making it impossible to co-mount it with other
> +	 * hierarchical subsystems.  Remove the artificially low PRIOIDX_SZ
> +	 * limit and properly nest configuration such that children follow
> +	 * their parents' configurations by default and are allowed to
> +	 * override and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  
>  static int netprio_device_event(struct notifier_block *unused,
> --- a/net/sched/cls_cgroup.c
> +++ b/net/sched/cls_cgroup.c
> @@ -82,6 +82,15 @@ struct cgroup_subsys net_cls_subsys = {
>  #endif
>  	.base_cftypes	= ss_files,
>  	.module		= THIS_MODULE,
> +
> +	/*
> +	 * While net_cls cgroup has the rudimentary hierarchy support of
> +	 * inheriting the parent's classid on cgroup creation, it doesn't
> +	 * properly propagates config changes in ancestors to their
> +	 * descendents.  A child should follow the parent's configuration
> +	 * but be allowed to override it.  Fix it and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  
>  struct cls_cgroup_head {
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -457,6 +457,15 @@ struct cgroup_subsys devices_subsys = {
>  	.destroy = devcgroup_destroy,
>  	.subsys_id = devices_subsys_id,
>  	.base_cftypes = dev_cgroup_files,
> +
> +	/*
> +	 * While devices cgroup has the rudimentary hierarchy support which
> +	 * checks the parent's restriction, it doesn't properly propagates
> +	 * config changes in ancestors to their descendents.  A child
> +	 * should only be allowed to add more restrictions to the parent's
> +	 * configuration.  Fix it and remove the following.
> +	 */
> +	.broken_hierarchy = true,
>  };
>  
>  int __devcgroup_inode_permission(struct inode *inode, int mask)
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers

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

[Li Zefan]

On 2012/9/14 3:20, Tejun Heo wrote:
> Currently, cgroup hierarchy support is a mess.  cpu related subsystems
> behave correctly - configuration, accounting and control on a parent
> properly cover its children.  blkio and freezer completely ignore
> hierarchy and treat all cgroups as if they're directly under the root
> cgroup.  Others show yet different behaviors.
> 
> These differing interpretations of cgroup hierarchy make using cgroup
> confusing and it impossible to co-mount controllers into the same
> hierarchy and obtain sane behavior.
> 
> Eventually, we want full hierarchy support from all subsystems and
> probably a unified hierarchy.  Users using separate hierarchies
> expecting completely different behaviors depending on the mounted
> subsystem is deterimental to making any progress on this front.
> 
> This patch adds cgroup_subsys.broken_hierarchy and sets it to %true
> for controllers which are lacking in hierarchy support.  The goal of
> this patch is two-fold.
> 
> * Move users away from using hierarchy on currently non-hierarchical
>   subsystems, so that implementing proper hierarchy support on those
>   doesn't surprise them.
> 
> * Keep track of which controllers are broken how and nudge the
>   subsystems to implement proper hierarchy support.
> 
> For now, start with a single warning message.  We can whine louder
> later on.
> 
> v2: Fixed a typo spotted by Michal. Warning message updated.
> 
> v3: Updated memcg part so that it doesn't generate warning in the
>     cases where .use_hierarchy=false doesn't make the behavior
>     different from root.use_hierarchy=true.  Fixed a typo spotted by
>     Glauber.
> 
> v4: Check ->broken_hierarchy after cgroup creation is complete so that
>     ->create() can affect the result per Michal.  Dropped unnecessary
>     memcg root handling per Michal.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Paul Turner <pjt-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
> Cc: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
> Cc: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Cc: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> Cc: Ingo Molnar <mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Arnaldo Carvalho de Melo <acme-f8uhVLnGfZaxAyOMLChx1axOck334EZe@public.gmane.org>
> Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
> Cc: Aneesh Kumar K.V <aneesh.kumar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

Acked-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

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

[Glauber Costa]

On 09/13/2012 11:20 PM, Tejun Heo wrote:
> v4: Check ->broken_hierarchy after cgroup creation is complete so that
>     ->create() can affect the result per Michal.  Dropped unnecessary
Sorry, "result per Michal" just sounds funny =p

I hope there aren't that many Michals around, or it won't scale!

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

[Tejun Heo]

On Thu, Sep 13, 2012 at 12:20:58PM -0700, Tejun Heo wrote:
> Currently, cgroup hierarchy support is a mess.  cpu related subsystems
> behave correctly - configuration, accounting and control on a parent
> properly cover its children.  blkio and freezer completely ignore
> hierarchy and treat all cgroups as if they're directly under the root
> cgroup.  Others show yet different behaviors.

Applied to cgroup/for-3.7-hierarchy w/ acks added.

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.7-hierarchy

The branch is based on top of the current mainline (fbcbe2b3c 3.6-rc5)
and only contains hierarchy related changes.  The branch will be kept
stable unless I do something completely stupid, so please feel free to
pull into other trees as necessary.

Thanks.

-- 
tejun

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

[Michal Hocko]

[didn't get to this sooner]

On Fri 14-09-12 12:06:08, Tejun Heo wrote:
> On Thu, Sep 13, 2012 at 12:20:58PM -0700, Tejun Heo wrote:
> > Currently, cgroup hierarchy support is a mess.  cpu related subsystems
> > behave correctly - configuration, accounting and control on a parent
> > properly cover its children.  blkio and freezer completely ignore
> > hierarchy and treat all cgroups as if they're directly under the root
> > cgroup.  Others show yet different behaviors.
> 
> Applied to cgroup/for-3.7-hierarchy w/ acks added.
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git for-3.7-hierarchy
> 
> The branch is based on top of the current mainline (fbcbe2b3c 3.6-rc5)
> and only contains hierarchy related changes.  

This is not very much mm git tree friendly because I would have to pull
a lot just to merge a single commit... It would have been much nicer if
you based it on the last major release (it applies cleanly on top of
3.5). mm git tree tries to get as few (unrelated) changes as possible on
top of the last major release.

> The branch will be kept stable unless I do something completely
> stupid, so please feel free to pull into other trees as necessary.

Anyway, keep it this way for now (as we are getting close to the end of
3.6). I have cherry-picked 8c7f6edb for now and use the branch after 3.6
is out.
-- 
Michal Hocko
SUSE Labs

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

[Tejun Heo]

Hello, Michal.

On Tue, Sep 18, 2012 at 8:04 AM, Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org> wrote:
>> The branch is based on top of the current mainline (fbcbe2b3c 3.6-rc5)
>> and only contains hierarchy related changes.
>
> This is not very much mm git tree friendly because I would have to pull
> a lot just to merge a single commit... It would have been much nicer if
> you based it on the last major release (it applies cleanly on top of
> 3.5). mm git tree tries to get as few (unrelated) changes as possible on
> top of the last major release.

Hmmm... I thought you explicitly requested it to be based on -rc5.
Maybe I misread... and yes I did. I read 3.5 as 3.6-rc5.

>> The branch will be kept stable unless I do something completely
>> stupid, so please feel free to pull into other trees as necessary.
>
> Anyway, keep it this way for now (as we are getting close to the end of
> 3.6). I have cherry-picked 8c7f6edb for now and use the branch after 3.6
> is out.

It's single commit.  Rebasing is not a problem.

Thanks.

-- 
tejun

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

[Michal Hocko]

On Tue 18-09-12 09:27:04, Tejun Heo wrote:
> Hello, Michal.
> 
> On Tue, Sep 18, 2012 at 8:04 AM, Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org> wrote:
> >> The branch is based on top of the current mainline (fbcbe2b3c 3.6-rc5)
> >> and only contains hierarchy related changes.
> >
> > This is not very much mm git tree friendly because I would have to pull
> > a lot just to merge a single commit... It would have been much nicer if
> > you based it on the last major release (it applies cleanly on top of
> > 3.5). mm git tree tries to get as few (unrelated) changes as possible on
> > top of the last major release.
> 
> Hmmm... I thought you explicitly requested it to be based on -rc5.
> Maybe I misread... and yes I did. I read 3.5 as 3.6-rc5.

No problem.
 
> >> The branch will be kept stable unless I do something completely
> >> stupid, so please feel free to pull into other trees as necessary.
> >
> > Anyway, keep it this way for now (as we are getting close to the end of
> > 3.6). I have cherry-picked 8c7f6edb for now and use the branch after 3.6
> > is out.
> 
> It's single commit.  Rebasing is not a problem.

Let's keep it this way. Who knows who has already pulled.

-- 
Michal Hocko
SUSE Labs