cgroup: mark subsystems with broken hierarchy support and whine if cgroups are nested for them

mm/memcontrol.c +8 -3

@@ -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();

mm/memcontrol.c +0 -0

 		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);

net/core/netprio_cgroup.c +1 -1

@@ -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,

Review mem_cgroup_create(struct cgroup *cont) · line 4961

Quoted 6 lines

(quoted diff) +1 -0

 	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.

Quoted 15 lines

(quoted diff) +0 -0

 		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);

net/core/netprio_cgroup.c +5 -1

@@ -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

net/core/netprio_cgroup.c struct cgroup_subsys net_prio_subsys = { · line 343

Quoted 6 lines

net/core/netprio_cgroup.c +6 -0

+	 * 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 = trye,

typo

Hello, Michal.

Quoted 14 lines · Michal Hocko

Review mem_cgroup_create(struct cgroup *cont) · line 4961

Quoted 6 lines

(quoted diff) +1 -0

 	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

Quoted 2 lines

which is unfortunate because it will add a noise to a reasonable configuration.

I suppose you're talking about having root group not performing any accounting and/or control?

I suppose such could be a valid use case
(is it really necessary tho?) but I don't think .use_hierarchy is the right interface for that.

If it's absolutely necessary, I think it should be a root-only flag (even if that ends up using the same code path). Eventually, we really want to kill .use_hierarchy, or at least make it to RO 1. As it's currently defined, it's just way too confusing.

Quoted 4 lines

If it's absolutely necessary, I think it should be a root-only flag (even if that ends up using the same code path). Eventually, we really want to kill .use_hierarchy, or at least make it to RO 1. As it's currently defined, it's just way too confusing.

Agreed on that, definitely.

Hello, Michal.

Quoted 14 lines · Michal Hocko

Review mem_cgroup_create(struct cgroup *cont) · line 4961

Quoted 6 lines

(quoted diff) +1 -0

 	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

Quoted 2 lines

which is unfortunate because it will add a noise to a reasonable configuration.

I suppose you're talking about having root group not performing any accounting and/or control?

I suppose such could be a valid use case
(is it really necessary tho?) but I don't think .use_hierarchy is the right interface for that.

If it's absolutely necessary, I think it should be a root-only flag (even if that ends up using the same code path). Eventually, we really want to kill .use_hierarchy, or at least make it to RO 1. As it's currently defined, it's just way too confusing.

Review mem_cgroup_create(struct cgroup *cont) · line 4961

Quoted 6 lines

(quoted diff) +1 -0

 	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.

Quoted 15 lines

(quoted diff) +0 -0

 		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);

net/core/netprio_cgroup.c +5 -1

@@ -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

net/core/netprio_cgroup.c struct cgroup_subsys net_prio_subsys = { · line 343

Quoted 6 lines

net/core/netprio_cgroup.c +6 -0

+	 * 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 = trye,

typo

I like the approach in general but see the comments bellow:

On Mon 10-09-12 15:33:55, Tejun Heo wrote:
[...]

Quoted 24 lines

mm/memcontrol.c +8 -3

@@ -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();

mm/memcontrol.c mem_cgroup_create(struct cgroup *cont) · line 4961

Quoted 4 lines

mm/memcontrol.c +1 -0

@@ -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:

mm/memcontrol.c +7 -0

index 795e525..d5c93ab 100644
@@ -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?

Quoted 66 lines · Michal Hocko

I like the approach in general but see the comments bellow:

On Mon 10-09-12 15:33:55, Tejun Heo wrote:
[...]

Quoted 24 lines

mm/memcontrol.c +8 -3

@@ -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();

mm/memcontrol.c mem_cgroup_create(struct cgroup *cont) · line 4961

Quoted 4 lines

mm/memcontrol.c +1 -0

@@ -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:

mm/memcontrol.c +7 -0

index 795e525..d5c93ab 100644
@@ -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);

Quoted 2 lines

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.

Quoted 5 lines

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?

Hello,

Quoted 4 lines · Michal Hocko

Quoted 4 lines

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.

Quoted 3 lines

Quoted 1 line

Everything aside from it should be warned.

Defintely. And that what the above guarantess, doesn't it?

I'm getting a bit worried that I might not be fully understanding what your concern is. Can you please elaborate what your worries are and the transition plan that you have in your mind regarding .use_hierarchy?

This is getting confusing for me as well, because I don't know if your reply was targeted towards me or Michal. As for me, I am in agreement with what you did, and I merely replied to Michal's concern and suggestion of not warning in the special 1-st level only setups saying I side with you.

Hello,

Quoted 4 lines · Michal Hocko

Quoted 4 lines

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.

Quoted 3 lines

Quoted 1 line

Everything aside from it should be warned.

Defintely. And that what the above guarantess, doesn't it?

I'm getting a bit worried that I might not be fully understanding what your concern is. Can you please elaborate what your worries are and the transition plan that you have in your mind regarding .use_hierarchy?

I would like to see use_hierarchy go away. The only concern I have is to warn only if somebody is doing something wrong (aka flat hierarchies). Or better put it this way. Do not warn in cases which do not change if use_hierarchy is gone or default changes to 1. An example:
root (use_hierarchy=0)

 | \
 |  A (use_hierarchy=0)
 |

B (use_hierarachy=1)
|\
C D

is a perfectly sane configuration and I do not see any reason to fill logs with some scary warnings when A is created. There will be no semantical change in this setup When use_hierchy is gone.

So the only thing I am proposing here is to warn only if something should be fixed in the configuration in order to be prepared for fully hierarchical (and that is a second level of children from root with use_hierachy==0).

Does it make more sense now?

Hello, Michal.

Quoted 22 lines · Michal Hocko

I would like to see use_hierarchy go away. The only concern I have is to warn only if somebody is doing something wrong (aka flat hierarchies). Or better put it this way. Do not warn in cases which do not change if use_hierarchy is gone or default changes to 1. An example:
root (use_hierarchy=0)

 | \
 |  A (use_hierarchy=0)
 |

B (use_hierarachy=1)
|\
C D

is a perfectly sane configuration and I do not see any reason to fill logs with some scary warnings when A is created. There will be no semantical change in this setup When use_hierchy is gone.

So the only thing I am proposing here is to warn only if something should be fixed in the configuration in order to be prepared for fully hierarchical (and that is a second level of children from root with use_hierachy==0).

Quoted 1 line

Does it make more sense now?

Ah, okay, so what you're saying is that we shouldn't warn if 0 .use_hierarchys don't make any behavior difference from when they're all 1, right?

If so, I have no objection. Will incorporate your updated version.

Quoted 25 lines · Tejun Heo

Hello, Michal.

Quoted 22 lines · Michal Hocko

I would like to see use_hierarchy go away. The only concern I have is to warn only if somebody is doing something wrong (aka flat hierarchies). Or better put it this way. Do not warn in cases which do not change if use_hierarchy is gone or default changes to 1. An example:
root (use_hierarchy=0)

 | \
 |  A (use_hierarchy=0)
 |

B (use_hierarachy=1)
|\
C D

is a perfectly sane configuration and I do not see any reason to fill logs with some scary warnings when A is created. There will be no semantical change in this setup When use_hierchy is gone.

So the only thing I am proposing here is to warn only if something should be fixed in the configuration in order to be prepared for fully hierarchical (and that is a second level of children from root with use_hierachy==0).

Quoted 5 lines

Quoted 1 line

Does it make more sense now?

Ah, okay, so what you're saying is that we shouldn't warn if 0 .use_hierarchys don't make any behavior difference from when they're all 1, right?

Exactly. 1st level of children under the root is exactly this kind of setup.

Quoted 1 line

If so, I have no objection. Will incorporate your updated version.

Thanks!

I want oppose it as well, but I believe part of this exercise is to make the need to have hierarchy widespread. Warning on the case 1st-level-only case helps with that, even if we make more noise than we should.

The reason I supported Tejun's proposal originally, is that I think that if we make the wrong amount of noise, being wrong by a surplus is better than being wrong by a deficit, in this case.

Quoted 8 lines · Glauber Costa

I want oppose it as well, but I believe part of this exercise is to make the need to have hierarchy widespread. Warning on the case 1st-level-only case helps with that, even if we make more noise than we should.

The reason I supported Tejun's proposal originally, is that I think that if we make the wrong amount of noise, being wrong by a surplus is better than being wrong by a deficit, in this case.

I think both are valid points and don't think it makes a lot of difference either way. Michal being the maintainer of the code, I'm taking his approach for this one.

Thanks.

(forgot cc'ing containers / cgroups mailing lists and used the old
address for Li. Reposting. Sorry for the noise.)

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.

(I tried to document what's broken and how it should be fixed. If I
got something wrong, please let me know.)

Quoted 3 lines

(I tried to document what's broken and how it should be fixed. If I
got something wrong, please let me know.)

So isn't cpuset broken too? child cpuset's cpu mask isn't necessary a subset of the parent's if the cpu_exclusive flag is not set.

Quoted 2 lines

So isn't cpuset broken too? child cpuset's cpu mask isn't necessary a subset of the parent's if the cpu_exclusive flag is not set.

Heh, didn't even look at that. Just assumed it was in the same boat w/ cpu{acct}. Will take a look.

Hmm... Looked at it and I don't think hierarchy support is broken w/ or w/o exclusive. Can you please elaborate?

Thanks.

"If a cpuset is cpu or mem exclusive, no other cpuset, other than a direct ancestor or descendant, may share any of the same CPUs or Memory Nodes."

So I think it tricked me as well. I was under the impression that "exclusive" would also disallow the kids.

Hello,

Quoted 6 lines · Glauber Costa

"If a cpuset is cpu or mem exclusive, no other cpuset, other than a direct ancestor or descendant, may share any of the same CPUs or Memory Nodes."

So I think it tricked me as well. I was under the impression that "exclusive" would also disallow the kids.

You two are confusing me even more. AFAICS, the hierarchical properties don't seem to change whether exclusive is set or not. It still ensures children can't have something parent doesn't allow and exclusive applies to whether to share something with siblings, so I don't think anything is broken hierarchy-wise. Am I missing something? If so, please be explicit and elaborate where and how it's broken.

Quoted 3 lines

(I tried to document what's broken and how it should be fixed. If I
got something wrong, please let me know.)

So isn't cpuset broken too? child cpuset's cpu mask isn't necessary a subset of the parent's if the cpu_exclusive flag is not set.

Quoted 32 lines · Tejun Heo

(forgot cc'ing containers / cgroups mailing lists and used the old
address for Li. Reposting. Sorry for the noise.)

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.

Quoted 3 lines

(I tried to document what's broken and how it should be fixed. If I
got something wrong, please let me know.)

So isn't cpuset broken too? child cpuset's cpu mask isn't necessary a subset of the parent's if the cpu_exclusive flag is not set.

(forgot cc'ing containers / cgroups mailing lists and used the old
address for Li. Reposting. Sorry for the noise.)

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.

(I tried to document what's broken and how it should be fixed. If I
got something wrong, please let me know.)

Hi Tejun,

Quoted 1 line · Tejun Heo

+ /* we're fully hierarchical iff root uses hierarchy */

minor nit: s/iff/if/

Hi Tejun,

Quoted 1 line · Tejun Heo

+ /* we're fully hierarchical iff root uses hierarchy */

minor nit: s/iff/if/

(forgot cc'ing containers / cgroups mailing lists and used the old
address for Li. Reposting. Sorry for the noise.)

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.

If you want application developers / users to understand this, then you really need to update the Documentation/cgroups/cgroups.txt file to provide suitable recommendations on use of hierarchies. As it stands today it arguably encourages apps to make use of arbitrary hierarchies. While some of the other docs (blkio-controller.txt) do describe the limitations of their implementation, they don't ever go as far as recommending against usage of hierarchies, which is what you seem to be saying.

Just printing warning messages in the logs with no docs to explain the issues which cause the warnings is not friendly to users, and IMHO will just lead people to ignore the warnings.

If you want application developers / users to understand this, then you really need to update the Documentation/cgroups/cgroups.txt file to provide suitable recommendations on use of hierarchies. As it stands today it arguably encourages apps to make use of arbitrary hierarchies. While some of the other docs (blkio-controller.txt) do describe the limitations of their implementation, they don't ever go as far as recommending against usage of hierarchies, which is what you seem to be saying.

Just printing warning messages in the logs with no docs to explain the issues which cause the warnings is not friendly to users, and IMHO will just lead people to ignore the warnings.

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.

(I tried to document what's broken and how it should be fixed. If I
got something wrong, please let me know.)

Signed-off-by Tejun Heo <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Li Zefan <[email protected]>

Cc Glauber Costa <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Paul Turner <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Thomas Graf <[email protected]>

Cc Serge E. Hallyn <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Neil Horman <[email protected]>

Cc Aneesh Kumar K.V <[email protected]>

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

block/blk-cgroup.c +8 -0

@@ -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);
 

include/linux/cgroup.h +15 -0

@@ -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 diallowed 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;
 

kernel/cgroup.c +10 -1

@@ -4075,8 +4075,17 @@ 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\" is not properly hierarchical yet, do not nest cgroups.\n",
+				   ss->name);
+			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;

kernel/cgroup_freezer.c +8 -0

@@ -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,
 };

kernel/events/core.c +7 -0

@@ -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 */

mm/memcontrol.c +9 -3

@@ -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;
 		hotcpu_notifier(memcg_cpu_hotplug_callback, 0);
 	} else {
 		parent = mem_cgroup_from_cont(cont->parent);

net/core/netprio_cgroup.c +11 -1

@@ -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 = trye,
 };
 
 static int netprio_device_event(struct notifier_block *unused,

net/sched/cls_cgroup.c +9 -0

@@ -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 {

security/device_cgroup.c +9 -0

@@ -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)

Quoted 19 lines · 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.

Quoted 3 lines

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

I know two current/potential users. systemd and libvirt. They are anyway going to create hierarchy irrespective of the fact whether controller supports it or not.

Quoted 3 lines

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

I know two current/potential users. systemd and libvirt. They are anyway going to create hierarchy irrespective of the fact whether controller supports it or not.

So even if we start screaming, nothing is going to change there, I suspect. Just that by default they expect every controller supports hiearchies.

Quoted 2 lines

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

I thought we can easily keep track of this in a simple .txt file and we really don't have to provide explicit warnings.

I think for these controllers it is a known fact that they don't support hiearchy yet. I am skeptical that providing explicit warnings is going to help.

Hello, Vivek.

Quoted 7 lines · Vivek Goyal

Quoted 3 lines

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

I know two current/potential users. systemd and libvirt. They are anyway going to create hierarchy irrespective of the fact whether controller supports it or not.

systemd mounts all controllers by default but it only creates hierarchy for controllers other than cpu, so we're in the clear there.

Quoted 3 lines

So even if we start screaming, nothing is going to change there, I suspect. Just that by default they expect every controller supports hiearchies.

As for libvirt, that's exactly the case where we want to be warning. The problem is that they're currently creating a hierarchy and expecting (or at least experiencing) flat behavior. We want them to be either switch to explicit flat cgroups or at least know very well that they're doing something unsupported and the behavior will change beneath them.

Quoted 7 lines

Quoted 3 lines

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

I thought we can easily keep track of this in a simple .txt file and we really don't have to provide explicit warnings.

Quoted 3 lines

I think for these controllers it is a known fact that they don't support hiearchy yet. I am skeptical that providing explicit warnings is going to help.

We should change blkio to support full hierarchy and soon and the current users of the broken hierarchy should be warned loudly and clearly; otherwise, this whole excercise is pointless,

It is kind of strange. First kernel allows creation of hiearchy for non-hierarchical controllers and it also gives warning for user space to not do that.

If creating hiearchy for flat controllers is wrong then kernel should not allow it in first place and enforce it, instead of just giving a warning to user space to not create the hierarchy.

Initially I had blocked the creation of hierarchy deeper than 1 level but later had to remove it as people wanted libvirt to use blkio controller and seemed to be fine with flat support. In fact there were people who insisted on flat support as they thought that made more sense.

Sure. Just that CFQ code now has become really complicated and messy (especially with that 3 service trees per group) so making it hierarchical is significant amount of effort.

And regarding change of behavior, we can always intoduce a .hierarchy file like cgroup which needs to be explicitly set to make controller truly hiearchical. That way behavior does not change in a subtle manner in future kernel releases. (Not that I am a fan of hierarchy file, just that it is cost we pay for not implementing hierarchical controller to begin with).

Hello, Vivek.

Quoted 8 lines · Vivek Goyal

It is kind of strange. First kernel allows creation of hiearchy for non-hierarchical controllers and it also gives warning for user space to not do that.

If creating hiearchy for flat controllers is wrong then kernel should not allow it in first place and enforce it, instead of just giving a warning to user space to not create the hierarchy.

Quoted 5 lines

Initially I had blocked the creation of hierarchy deeper than 1 level but later had to remove it as people wanted libvirt to use blkio controller and seemed to be fine with flat support. In fact there were people who insisted on flat support as they thought that made more sense.

Yes, it is an ugly situation and we'll have to drag ourselves out of this mess gradually. I hope it hadn't happened like this but what happened already happened and I can't see a better way out. If you can, please share.

Quoted 3 lines

Sure. Just that CFQ code now has become really complicated and messy (especially with that 3 service trees per group) so making it hierarchical is significant amount of effort.

I think the wording for the warning wasn't entirely accurate. The thing that we wanna warn about is that the hierarchy behavior isn't complete yet and it may change in the future. If we can absolutely declare that cfq is and will stay broken in terms of hierarchy support, that could work too but I don't really think that's a good idea. It's something we need to do one way or the other.

Quoted 6 lines

And regarding change of behavior, we can always intoduce a .hierarchy file like cgroup which needs to be explicitly set to make controller truly hiearchical. That way behavior does not change in a subtle manner in future kernel releases. (Not that I am a fan of hierarchy file, just that it is cost we pay for not implementing hierarchical controller to begin with).

I think that way of thinking is what led to this horrible mess in the first place. More flexibility doesn't equal better. We can't keep piling things on top. I'd like to at least have an exit strategy. use_hierarchy drives us further away from where we wanna be.

Ok, so whole point of warning seems to be so that we can change the behavior in future and say to user space they few kernel releases back we had started printing a warning that creating hierarchy is wrong and move to a flat setup. So don't complain to us now.?

Are you planning to get rid of .user_hierarchy file from memory cgroup too? If you are planning not to put such a file in blkio controller, then it will make sense to remove it from mem_cgorup too.

The point I am trying to make is that deep hierarchies (5-6 levels) are /going to be a reality and if accounting overhead is not manageable then enabling hierarchy by default might not be a practical solution even if you implement hierarchy support (like memory cgroup), and in that case retaining .use_hierarchy will make sense.

IIUC, are you saying that now none of the controller will have flat hiearchy support because there is no way to be able to create flat hierarchy. (Any new group is child of root group). So are we moving towards a model where every controller is hierarhical and there is no concept of flat hierarchy.

Quoted 5 lines

The point I am trying to make is that deep hierarchies (5-6 levels) are /going to be a reality and if accounting overhead is not manageable then enabling hierarchy by default might not be a practical solution even if you implement hierarchy support (like memory cgroup), and in that case retaining .use_hierarchy will make sense.

That doesn't make any sense to me. If you don't want feature and overhead of hierarchy, you just need to not create a hierarchy. If hierarchical behavior isn't needed, why create hierarchy at all?

"Being able to provide hierarchical support" is an example of such restrictions. And as Vivek said himself, for the CFQ case should be doable.

Yeah, it's mostly that cfq was already a hairy monster before blkcg was added to it and unfortunately we didn't make it any cleaner in the process and blkcg itself has a lot of other issues including being completely broken w.r.t. writeback writes. In addition there are two sub-controllers - the cfq one and blk-throttle. So, it's just that there are too many scary things to do and not enough man power or maybe interest. I hope we could just declare cgroup isn't supported on block devices but that doesn't seem feasible at this point either.

I might / probably work on it and am hoping to coerce Vivek into it too. If you wanna jump in, please be my guest.

Biggest problem with blkcg CFQ implementation is idling on cgroup. If we don't idle on cgroup, then we don't get the service differentiaton for most of the workloads and if we do idle then performance starts to suck very soon (The moment few cgroups are created). And hierarchy will just exacertbate this problem because then one will try to idle at each group in hierarchy.

This problem is something similar to CFQ's idling on sequential queues and iopriority. Because we never idled on random IO queue, ioprios never worked on random IO queues. And same is true for buffered write queues. Similary, if you don't idle on groups, then for most of the workloads, service differentiation is not visible. Only the one which are highly sequential on nature, one can see service differentiation.

That's one fundamental problem for which we need to have a good answer before we try to do more work on blkcg. Because we can write as much code but at the end of the day it might still not be useful because of the above mentioned issue I faced.

Hey, Vivek.

(cc'ing Rakesh and Chad who work on iosched in google).

Quoted 14 lines · Vivek Goyal

Biggest problem with blkcg CFQ implementation is idling on cgroup. If we don't idle on cgroup, then we don't get the service differentiaton for most of the workloads and if we do idle then performance starts to suck very soon (The moment few cgroups are created). And hierarchy will just exacertbate this problem because then one will try to idle at each group in hierarchy.

This problem is something similar to CFQ's idling on sequential queues and iopriority. Because we never idled on random IO queue, ioprios never worked on random IO queues. And same is true for buffered write queues. Similary, if you don't idle on groups, then for most of the workloads, service differentiation is not visible. Only the one which are highly sequential on nature, one can see service differentiation.

Quoted 4 lines

That's one fundamental problem for which we need to have a good answer before we try to do more work on blkcg. Because we can write as much code but at the end of the day it might still not be useful because of the above mentioned issue I faced.

I talked with Rakesh about this as the modified cfq-iosched used in google supports proper hierarchy and the feature is heavily depended upon. I was told that nesting doesn't really change anything. The only thing which matters is the number of active cgroups and whether they're nested or how deep doesn't matter - IIUC there's no need to idle for internal nodes if they don't have IOs pending.

He draw me some diagrams which made sense for me and the code apparently actually works, so there doesn't seem to be any fundamental issue in implementing hierarchy support in cfq.

Quoted 55 lines · Vivek Goyal

Quoted 31 lines · Tejun Heo

Hey, Vivek.

(cc'ing Rakesh and Chad who work on iosched in google).

Quoted 14 lines · Vivek Goyal

Biggest problem with blkcg CFQ implementation is idling on cgroup. If we don't idle on cgroup, then we don't get the service differentiaton for most of the workloads and if we do idle then performance starts to suck very soon (The moment few cgroups are created). And hierarchy will just exacertbate this problem because then one will try to idle at each group in hierarchy.

This problem is something similar to CFQ's idling on sequential queues and iopriority. Because we never idled on random IO queue, ioprios never worked on random IO queues. And same is true for buffered write queues. Similary, if you don't idle on groups, then for most of the workloads, service differentiation is not visible. Only the one which are highly sequential on nature, one can see service differentiation.

Quoted 4 lines

That's one fundamental problem for which we need to have a good answer before we try to do more work on blkcg. Because we can write as much code but at the end of the day it might still not be useful because of the above mentioned issue I faced.

I talked with Rakesh about this as the modified cfq-iosched used in google supports proper hierarchy and the feature is heavily depended upon. I was told that nesting doesn't really change anything. The only thing which matters is the number of active cgroups and whether they're nested or how deep doesn't matter - IIUC there's no need to idle for internal nodes if they don't have IOs pending.

Quoted 3 lines

He draw me some diagrams which made sense for me and the code apparently actually works, so there doesn't seem to be any fundamental issue in implementing hierarchy support in cfq.

Hmm...., They probably are right. Idling only on leaf groups can make sure that none of the groups loses its fair share of quota. Thinking loud...

				root
                                /  \
                               T1  G1
                                  /  \
                                 T2   G2
                                       \
                                        T3 

So if task T3 finishes and there is no active IO from T3, we will idle on group G2 (in the hope that soon some IO will show up from T3 or from some other task in G2). And that alone should make sure all the group nodes in the path to root (G1) get their fair share at their respective level and no additional idling should be needed.

Quoted 3 lines

So sounds like hierarhcy will not cause additional idling. Idling will solely depend on leaf active groups.

What if the G's also have tasks? That is a valid configuration, after all.

Quoted 2 lines

I might / probably work on it and am hoping to coerce Vivek into it too. If you wanna jump in, please be my guest.

I am devoting part of my time to general cgroup stuff, I can definitely help you with that. Advantage is that you won't have to coerce me.

This is not about being interested anymore
This thing is more pressing than global warming.

Haven't gone through the whole patch yet, and not sure how you actually touch memcg in here. And I absolutely know we have discussed this before, but I still stand that for the memcg case, in which hierarchy can be enabled by a crazy boolean, we should be enabling it somehow. It is fine if we don't want to change the default without warning first, but a Kconfig option to make this default would really help. We should tell everybody with a well defined lifecycle to just enable it.

From upstream, my current plan for .use_hierarchy is

include/linux/cgroup.h struct cgroup_subsys { · line 513

Quoted 1 line

include/linux/cgroup.h +1 -0

+	 * It's now diallowed to create nested cgroups if the subsystem is

typo, disallowed.

include/linux/cgroup.h struct cgroup_subsys { · line 520

Quoted 7 lines

include/linux/cgroup.h +7 -0

+	 * 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;
+

why do we need the extra bool? Isn't WARN_ON_ONCE() suitable here?

Quoted 4 lines

net/core/netprio_cgroup.c +4 -0

+	/*
+	 * 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

net/core/netprio_cgroup.c struct cgroup_subsys net_prio_subsys = { · line 344

Quoted 6 lines

net/core/netprio_cgroup.c +5 -0

+	 * 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 = trye,
 };

"trye" doesn't seem to be a recognized word.

Quoted 14 lines

static int netprio_device_event(struct notifier_block *unused,

net/sched/cls_cgroup.c +7 -0

@@ -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.

net/sched/cls_cgroup.c struct cgroup_subsys net_cls_subsys = { · line 95

Quoted 4 lines

net/sched/cls_cgroup.c +2 -0

+	 */
+	.broken_hierarchy = true,
 };
 

Since all this cgroup provides is a marking, it is not terribly obvious to me what "proper hierarchy" would mean. Input from the authors would be strongly advisable here.

Here is where the Kconfig option comes to play. If we do it in the kernel, userspace doesn't have to do anything. I spoke with Lennart and Kay, and at least from a systemd PoV, they would much rather not provide a hack in userspace for a file that is scheduled to go away in any case
- which I personally believe is a fair request.

It is a default, so the effect for the user is the same: After the machine boots, use_hierarchy = 1, and he can still flip to 0 for some time.

Quoted 2 lines

Setting mark on a parent should be reflected on all its children w/o their own explicit settings.

That is clear, and better behavior than we have today. What I mean, is that by setting its own marking, the child can pretty much "escape" the group.

The ideal solution - from this point of view only - would be to have more than one marking, and mark with all the way down to the root. So if you have an iptables rule to match one marking, it still applies to the kids. And you can still have extra markings.

I am not sure this is feasible, though, in which case your solution could be a good compromise. But please let's aim for it.

Quoted 9 lines · Tejun Heo

Hello, Glauber.

Quoted 6 lines · Glauber Costa

Here is where the Kconfig option comes to play. If we do it in the kernel, userspace doesn't have to do anything. I spoke with Lennart and Kay, and at least from a systemd PoV, they would much rather not provide a hack in userspace for a file that is scheduled to go away in any case
- which I personally believe is a fair request.

Quoted 9 lines

Quoted 2 lines

It is a default, so the effect for the user is the same: After the machine boots, use_hierarchy = 1, and he can still flip to 0 for some time.

Alright, let's go Kconfig. Let's just make sure that the transitional nature is clearly labeled and the fact that the default config will generate a warning when nested cgroups are created in memcg. We can then coordinate the flip with distros. Can you please repost the Kconfig patch?

Just wait a bit. If you are merging your earlier patch, I'd like to take that in consideration. In that case, I'd rebase.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

Setting mark on a parent should be reflected on all its children w/o their own explicit settings.

That is clear, and better behavior than we have today. What I mean, is that by setting its own marking, the child can pretty much "escape" the group.

Quoted 4 lines

Quoted 4 lines

The ideal solution - from this point of view only - would be to have more than one marking, and mark with all the way down to the root. So if you have an iptables rule to match one marking, it still applies to the kids. And you can still have extra markings.

Quoted 6 lines

Quoted 2 lines

I am not sure this is feasible, though, in which case your solution could be a good compromise. But please let's aim for it.

I don't think it supports multiple tags. If that's possible, it would be nice but I don't think it's a must.

It is not about "it supports", but more about "can it support?" But I honestly can't answer this question. net_cls people need to come and tell us about it.