[PATCH rfc 0/3] mm: memcg: deprecate cgroup v1 non-hierarchical mode

[PATCH rfc 0/3] mm: memcg: deprecate cgroup v1 non-hierarchical mode

[Roman Gushchin]

The non-hierarchical cgroup v1 mode is a legacy of early days
of the memory controller and doesn't bring any value today.
However, it complicates the code and creates many edge cases
all over the memory controller code.

It's a good time to deprecate it completely. This patchset removes
the internal logic, adjusts the user interface and updates
the documentation. The alt patch removes some bits of the cgroup
core code, which become obsolete.


Roman Gushchin (3):
  mm: memcg: deprecate the non-hierarchical mode
  docs: cgroup-v1: reflect the deprecation of the non-hierarchical mode
  cgroup: remove obsoleted broken_hierarchy and warned_broken_hierarchy

 .../admin-guide/cgroup-v1/memcg_test.rst      |  8 +-
 .../admin-guide/cgroup-v1/memory.rst          | 40 +++------
 include/linux/cgroup-defs.h                   | 15 ----
 include/linux/memcontrol.h                    |  7 --
 kernel/cgroup/cgroup.c                        | 12 ---
 mm/memcontrol.c                               | 90 +++----------------
 6 files changed, 29 insertions(+), 143 deletions(-)

-- 
2.26.2

[PATCH rfc 1/3] mm: memcg: deprecate the non-hierarchical mode

[Roman Gushchin]

The non-hierarchical cgroup v1 mode is a legacy of early days
of the memory controller and doesn't bring any value today.
However, it complicates the code and creates many edge cases
all over the memory controller code.

It's a good time to deprecate it completely.

Functionally this patch enabled is by default for all cgroups
and forbids switching it off. Nothing changes if cgroup v2 is used:
hierarchical mode was enforced from scratch.

To protect the ABI memory.use_hierarchy interface is preserved
with a limited functionality: reading always returns "1", writing
of "1" passes silently, writing of any other value fails with
-EINVAL and a warning to dmesg (on the first occasion).

Signed-off-by: Roman Gushchin <guro@fb.com>
---
 include/linux/memcontrol.h |  7 ---
 kernel/cgroup/cgroup.c     |  5 ---
 mm/memcontrol.c            | 90 ++++++--------------------------------
 3 files changed, 13 insertions(+), 89 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 0f4dd7829fb2..31000929db4b 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -234,11 +234,6 @@ struct mem_cgroup {
 	/* vmpressure notifications */
 	struct vmpressure vmpressure;
 
-	/*
-	 * Should the accounting and control be hierarchical, per subtree?
-	 */
-	bool use_hierarchy;
-
 	/*
 	 * Should the OOM killer kill all belonging tasks, had it kill one?
 	 */
@@ -758,8 +753,6 @@ static inline bool mem_cgroup_is_descendant(struct mem_cgroup *memcg,
 {
 	if (root == memcg)
 		return true;
-	if (!root->use_hierarchy)
-		return false;
 	return cgroup_is_descendant(memcg->css.cgroup, root->css.cgroup);
 }
 
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index f2eeff74d713..621a586e3529 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -281,9 +281,6 @@ bool cgroup_ssid_enabled(int ssid)
  * - cpuset: a task can be moved into an empty cpuset, and again it takes
  *   masks of ancestors.
  *
- * - memcg: use_hierarchy is on by default and the cgroup file for the flag
- *   is not created.
- *
  * - blkcg: blk-throttle becomes properly hierarchical.
  *
  * - debug: disallowed on the default hierarchy.
@@ -5156,8 +5153,6 @@ static struct cgroup_subsys_state *css_create(struct cgroup *cgrp,
 	    cgroup_parent(parent)) {
 		pr_warn("%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_warn("\"memory\" requires setting use_hierarchy to 1 on the root\n");
 		ss->warned_broken_hierarchy = true;
 	}
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4b0cd4b78d47..2a37785e9abd 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1127,12 +1127,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 	if (prev && !reclaim)
 		pos = prev;
 
-	if (!root->use_hierarchy && root != root_mem_cgroup) {
-		if (prev)
-			goto out;
-		return root;
-	}
-
 	rcu_read_lock();
 
 	if (reclaim) {
@@ -1212,7 +1206,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
 
 out_unlock:
 	rcu_read_unlock();
-out:
 	if (prev && prev != root)
 		css_put(&prev->css);
 
@@ -3440,10 +3433,7 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
 }
 
 /*
- * Test whether @memcg has children, dead or alive.  Note that this
- * function doesn't care whether @memcg has use_hierarchy enabled and
- * returns %true if there are child csses according to the cgroup
- * hierarchy.  Testing use_hierarchy is the caller's responsibility.
+ * Test whether @memcg has children, dead or alive.
  */
 static inline bool memcg_has_children(struct mem_cgroup *memcg)
 {
@@ -3503,37 +3493,20 @@ static ssize_t mem_cgroup_force_empty_write(struct kernfs_open_file *of,
 static u64 mem_cgroup_hierarchy_read(struct cgroup_subsys_state *css,
 				     struct cftype *cft)
 {
-	return mem_cgroup_from_css(css)->use_hierarchy;
+	return 1;
 }
 
 static int mem_cgroup_hierarchy_write(struct cgroup_subsys_state *css,
 				      struct cftype *cft, u64 val)
 {
-	int retval = 0;
-	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
-	struct mem_cgroup *parent_memcg = mem_cgroup_from_css(memcg->css.parent);
-
-	if (memcg->use_hierarchy == val)
+	if (val == 1)
 		return 0;
 
-	/*
-	 * If parent's use_hierarchy is set, we can't make any modifications
-	 * in the child subtrees. If it is unset, then the change can
-	 * occur, provided the current cgroup has no children.
-	 *
-	 * For the root cgroup, parent_mem is NULL, we allow value to be
-	 * set if there are no children.
-	 */
-	if ((!parent_memcg || !parent_memcg->use_hierarchy) &&
-				(val == 1 || val == 0)) {
-		if (!memcg_has_children(memcg))
-			memcg->use_hierarchy = val;
-		else
-			retval = -EBUSY;
-	} else
-		retval = -EINVAL;
+	pr_warn_once("Non-hierarchical mode is deprecated. "
+		     "Please report your usecase to linux-mm@kvack.org if you "
+		     "depend on this functionality.\n");
 
-	return retval;
+	return -EINVAL;
 }
 
 static unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
@@ -3727,8 +3700,6 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
 		child = mem_cgroup_from_css(css);
 		BUG_ON(child->kmemcg_id != kmemcg_id);
 		child->kmemcg_id = parent->kmemcg_id;
-		if (!memcg->use_hierarchy)
-			break;
 	}
 	rcu_read_unlock();
 
@@ -5319,38 +5290,22 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 	if (parent) {
 		memcg->swappiness = mem_cgroup_swappiness(parent);
 		memcg->oom_kill_disable = parent->oom_kill_disable;
-	}
-	if (!parent) {
-		page_counter_init(&memcg->memory, NULL);
-		page_counter_init(&memcg->swap, NULL);
-		page_counter_init(&memcg->kmem, NULL);
-		page_counter_init(&memcg->tcpmem, NULL);
-	} else if (parent->use_hierarchy) {
-		memcg->use_hierarchy = true;
+
 		page_counter_init(&memcg->memory, &parent->memory);
 		page_counter_init(&memcg->swap, &parent->swap);
 		page_counter_init(&memcg->kmem, &parent->kmem);
 		page_counter_init(&memcg->tcpmem, &parent->tcpmem);
 	} else {
-		page_counter_init(&memcg->memory, &root_mem_cgroup->memory);
-		page_counter_init(&memcg->swap, &root_mem_cgroup->swap);
-		page_counter_init(&memcg->kmem, &root_mem_cgroup->kmem);
-		page_counter_init(&memcg->tcpmem, &root_mem_cgroup->tcpmem);
-		/*
-		 * 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 != root_mem_cgroup)
-			memory_cgrp_subsys.broken_hierarchy = true;
-	}
+		page_counter_init(&memcg->memory, NULL);
+		page_counter_init(&memcg->swap, NULL);
+		page_counter_init(&memcg->kmem, NULL);
+		page_counter_init(&memcg->tcpmem, NULL);
 
-	/* The following stuff does not apply to the root */
-	if (!parent) {
 		root_mem_cgroup = memcg;
 		return &memcg->css;
 	}
 
+	/* The following stuff does not apply to the root */
 	error = memcg_online_kmem(memcg);
 	if (error)
 		goto fail;
@@ -6187,24 +6142,6 @@ static void mem_cgroup_move_task(void)
 }
 #endif
 
-/*
- * Cgroup retains root cgroups across [un]mount cycles making it necessary
- * to verify whether we're attached to the default hierarchy on each mount
- * attempt.
- */
-static void mem_cgroup_bind(struct cgroup_subsys_state *root_css)
-{
-	/*
-	 * use_hierarchy is forced on the default hierarchy.  cgroup core
-	 * guarantees that @root doesn't have any children, so turning it
-	 * on for the root memcg is enough.
-	 */
-	if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
-		root_mem_cgroup->use_hierarchy = true;
-	else
-		root_mem_cgroup->use_hierarchy = false;
-}
-
 static int seq_puts_memcg_tunable(struct seq_file *m, unsigned long value)
 {
 	if (value == PAGE_COUNTER_MAX)
@@ -6542,7 +6479,6 @@ struct cgroup_subsys memory_cgrp_subsys = {
 	.can_attach = mem_cgroup_can_attach,
 	.cancel_attach = mem_cgroup_cancel_attach,
 	.post_attach = mem_cgroup_move_task,
-	.bind = mem_cgroup_bind,
 	.dfl_cftypes = memory_files,
 	.legacy_cftypes = mem_cgroup_legacy_files,
 	.early_init = 0,
-- 
2.26.2

[PATCH rfc 2/3] docs: cgroup-v1: reflect the deprecation of the non-hierarchical mode

[Roman Gushchin]

Update cgroup v1 docs after the deprecation of the non-hierarchical
mode of the memory controller.

Signed-off-by: Roman Gushchin <guro@fb.com>
---
 .../admin-guide/cgroup-v1/memcg_test.rst      |  8 ++--
 .../admin-guide/cgroup-v1/memory.rst          | 40 ++++++-------------
 2 files changed, 16 insertions(+), 32 deletions(-)

diff --git a/Documentation/admin-guide/cgroup-v1/memcg_test.rst b/Documentation/admin-guide/cgroup-v1/memcg_test.rst
index 3f7115e07b5d..4f83de2dab6e 100644
--- a/Documentation/admin-guide/cgroup-v1/memcg_test.rst
+++ b/Documentation/admin-guide/cgroup-v1/memcg_test.rst
@@ -219,13 +219,11 @@ Under below explanation, we assume CONFIG_MEM_RES_CTRL_SWAP=y.
 
 	This is an easy way to test page migration, too.
 
-9.5 mkdir/rmdir
----------------
+9.5 nested cgroups
+------------------
 
-	When using hierarchy, mkdir/rmdir test should be done.
-	Use tests like the following::
+	Use tests like the following for testing nested cgroups::
 
-		echo 1 >/opt/cgroup/01/memory/use_hierarchy
 		mkdir /opt/cgroup/01/child_a
 		mkdir /opt/cgroup/01/child_b
 
diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst
index 12757e63b26c..a44cd467d218 100644
--- a/Documentation/admin-guide/cgroup-v1/memory.rst
+++ b/Documentation/admin-guide/cgroup-v1/memory.rst
@@ -77,6 +77,8 @@ Brief summary of control files.
  memory.soft_limit_in_bytes	     set/show soft limit of memory usage
  memory.stat			     show various statistics
  memory.use_hierarchy		     set/show hierarchical account enabled
+                                     This knob is deprecated and shouldn't be
+                                     used.
  memory.force_empty		     trigger forced page reclaim
  memory.pressure_level		     set memory pressure notifications
  memory.swappiness		     set/show swappiness parameter of vmscan
@@ -495,16 +497,13 @@ cgroup might have some charge associated with it, even though all
 tasks have migrated away from it. (because we charge against pages, not
 against tasks.)
 
-We move the stats to root (if use_hierarchy==0) or parent (if
-use_hierarchy==1), and no change on the charge except uncharging
+We move the stats to parent, and no change on the charge except uncharging
 from the child.
 
 Charges recorded in swap information is not updated at removal of cgroup.
 Recorded information is discarded and a cgroup which uses swap (swapcache)
 will be charged as a new owner of it.
 
-About use_hierarchy, see Section 6.
-
 5. Misc. interfaces
 ===================
 
@@ -527,8 +526,6 @@ About use_hierarchy, see Section 6.
   write will still return success. In this case, it is expected that
   memory.kmem.usage_in_bytes == memory.usage_in_bytes.
 
-  About use_hierarchy, see Section 6.
-
 5.2 stat file
 -------------
 
@@ -675,31 +672,20 @@ hierarchy::
 		      d   e
 
 In the diagram above, with hierarchical accounting enabled, all memory
-usage of e, is accounted to its ancestors up until the root (i.e, c and root),
-that has memory.use_hierarchy enabled. If one of the ancestors goes over its
-limit, the reclaim algorithm reclaims from the tasks in the ancestor and the
-children of the ancestor.
-
-6.1 Enabling hierarchical accounting and reclaim
-------------------------------------------------
-
-A memory cgroup by default disables the hierarchy feature. Support
-can be enabled by writing 1 to memory.use_hierarchy file of the root cgroup::
+usage of e, is accounted to its ancestors up until the root (i.e, c and root).
+If one of the ancestors goes over its limit, the reclaim algorithm reclaims
+from the tasks in the ancestor and the children of the ancestor.
 
-	# echo 1 > memory.use_hierarchy
-
-The feature can be disabled by::
+6.1 Hierarchical accounting and reclaim
+---------------------------------------
 
-	# echo 0 > memory.use_hierarchy
+Hierarchical accounting is enabled by default. Disabling the hierarchical
+accounting is deprecated. An attempt to do it will result in a failure
+and a warning printed to dmesg.
 
-NOTE1:
-       Enabling/disabling will fail if either the cgroup already has other
-       cgroups created below it, or if the parent cgroup has use_hierarchy
-       enabled.
+For compatibility reasons writing 1 to memory.use_hierarchy will always pass::
 
-NOTE2:
-       When panic_on_oom is set to "2", the whole system will panic in
-       case of an OOM event in any cgroup.
+	# echo 1 > memory.use_hierarchy
 
 7. Soft limits
 ==============
-- 
2.26.2

[PATCH rfc 3/3] cgroup: remove obsoleted broken_hierarchy and warned_broken_hierarchy

[Roman Gushchin]

With the deprecation of the non-hierarchical mode of the memory
controller there are no more examples of broken hierarchies left.

Let's remove the cgroup core code which was supposed to print
warnings about creating of broken hierarchies.

Signed-off-by: Roman Gushchin <guro@fb.com>
---
 include/linux/cgroup-defs.h | 15 ---------------
 kernel/cgroup/cgroup.c      |  7 -------
 2 files changed, 22 deletions(-)

diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index fee0b5547cd0..559ee05f86b2 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -668,21 +668,6 @@ struct cgroup_subsys {
 	 */
 	bool threaded:1;
 
-	/*
-	 * 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:1;
-	bool warned_broken_hierarchy:1;
-
 	/* the following two fields are initialized automtically during boot */
 	int id;
 	const char *name;
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 621a586e3529..fefa21981027 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5149,13 +5149,6 @@ static struct cgroup_subsys_state *css_create(struct cgroup *cgrp,
 	if (err)
 		goto err_list_del;
 
-	if (ss->broken_hierarchy && !ss->warned_broken_hierarchy &&
-	    cgroup_parent(parent)) {
-		pr_warn("%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);
-		ss->warned_broken_hierarchy = true;
-	}
-
 	return css;
 
 err_list_del:
-- 
2.26.2

Re: [PATCH rfc 1/3] mm: memcg: deprecate the non-hierarchical mode

[Michal Hocko]

On Tue 03-11-20 13:27:23, Roman Gushchin wrote:
> The non-hierarchical cgroup v1 mode is a legacy of early days
> of the memory controller and doesn't bring any value today.

This is a bold statement ;)
All that we know today is that we have a warning in place to complain
loudly when somebody relies on use_hierarchy=0 with a deeper
hierarchy. For all those years we have seen _zero_ reports that would
describe a sensible usecase.
Moreover we (SUSE) have backported this warning into old distribution
kernels (since 3.0 based kernels) to extend the coverage and didn't hear
even for users who adopt new kernels only very slowly. The only report
we have seen so far was a LTP test suite which doesn't really reflect
any real life usecase.

Feel free to reuse the above in the changelog.

> However, it complicates the code and creates many edge cases
> all over the memory controller code.
> 
> It's a good time to deprecate it completely.
> 
> Functionally this patch enabled is by default for all cgroups
> and forbids switching it off. Nothing changes if cgroup v2 is used:
> hierarchical mode was enforced from scratch.
> 
> To protect the ABI memory.use_hierarchy interface is preserved
> with a limited functionality: reading always returns "1", writing
> of "1" passes silently, writing of any other value fails with
> -EINVAL and a warning to dmesg (on the first occasion).

Yes, that makes sense.
 
> Signed-off-by: Roman Gushchin <guro@fb.com>

I do not see any problems with the patch or any left overs behind
(except for the documentation which you handle in the follow up
patches).

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks and let's see whether some last minute usecase show up.

> ---
>  include/linux/memcontrol.h |  7 ---
>  kernel/cgroup/cgroup.c     |  5 ---
>  mm/memcontrol.c            | 90 ++++++--------------------------------
>  3 files changed, 13 insertions(+), 89 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 0f4dd7829fb2..31000929db4b 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -234,11 +234,6 @@ struct mem_cgroup {
>  	/* vmpressure notifications */
>  	struct vmpressure vmpressure;
>  
> -	/*
> -	 * Should the accounting and control be hierarchical, per subtree?
> -	 */
> -	bool use_hierarchy;
> -
>  	/*
>  	 * Should the OOM killer kill all belonging tasks, had it kill one?
>  	 */
> @@ -758,8 +753,6 @@ static inline bool mem_cgroup_is_descendant(struct mem_cgroup *memcg,
>  {
>  	if (root == memcg)
>  		return true;
> -	if (!root->use_hierarchy)
> -		return false;
>  	return cgroup_is_descendant(memcg->css.cgroup, root->css.cgroup);
>  }
>  
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index f2eeff74d713..621a586e3529 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -281,9 +281,6 @@ bool cgroup_ssid_enabled(int ssid)
>   * - cpuset: a task can be moved into an empty cpuset, and again it takes
>   *   masks of ancestors.
>   *
> - * - memcg: use_hierarchy is on by default and the cgroup file for the flag
> - *   is not created.
> - *
>   * - blkcg: blk-throttle becomes properly hierarchical.
>   *
>   * - debug: disallowed on the default hierarchy.
> @@ -5156,8 +5153,6 @@ static struct cgroup_subsys_state *css_create(struct cgroup *cgrp,
>  	    cgroup_parent(parent)) {
>  		pr_warn("%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_warn("\"memory\" requires setting use_hierarchy to 1 on the root\n");
>  		ss->warned_broken_hierarchy = true;
>  	}
>  
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4b0cd4b78d47..2a37785e9abd 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1127,12 +1127,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>  	if (prev && !reclaim)
>  		pos = prev;
>  
> -	if (!root->use_hierarchy && root != root_mem_cgroup) {
> -		if (prev)
> -			goto out;
> -		return root;
> -	}
> -
>  	rcu_read_lock();
>  
>  	if (reclaim) {
> @@ -1212,7 +1206,6 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
>  
>  out_unlock:
>  	rcu_read_unlock();
> -out:
>  	if (prev && prev != root)
>  		css_put(&prev->css);
>  
> @@ -3440,10 +3433,7 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order,
>  }
>  
>  /*
> - * Test whether @memcg has children, dead or alive.  Note that this
> - * function doesn't care whether @memcg has use_hierarchy enabled and
> - * returns %true if there are child csses according to the cgroup
> - * hierarchy.  Testing use_hierarchy is the caller's responsibility.
> + * Test whether @memcg has children, dead or alive.
>   */
>  static inline bool memcg_has_children(struct mem_cgroup *memcg)
>  {
> @@ -3503,37 +3493,20 @@ static ssize_t mem_cgroup_force_empty_write(struct kernfs_open_file *of,
>  static u64 mem_cgroup_hierarchy_read(struct cgroup_subsys_state *css,
>  				     struct cftype *cft)
>  {
> -	return mem_cgroup_from_css(css)->use_hierarchy;
> +	return 1;
>  }
>  
>  static int mem_cgroup_hierarchy_write(struct cgroup_subsys_state *css,
>  				      struct cftype *cft, u64 val)
>  {
> -	int retval = 0;
> -	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> -	struct mem_cgroup *parent_memcg = mem_cgroup_from_css(memcg->css.parent);
> -
> -	if (memcg->use_hierarchy == val)
> +	if (val == 1)
>  		return 0;
>  
> -	/*
> -	 * If parent's use_hierarchy is set, we can't make any modifications
> -	 * in the child subtrees. If it is unset, then the change can
> -	 * occur, provided the current cgroup has no children.
> -	 *
> -	 * For the root cgroup, parent_mem is NULL, we allow value to be
> -	 * set if there are no children.
> -	 */
> -	if ((!parent_memcg || !parent_memcg->use_hierarchy) &&
> -				(val == 1 || val == 0)) {
> -		if (!memcg_has_children(memcg))
> -			memcg->use_hierarchy = val;
> -		else
> -			retval = -EBUSY;
> -	} else
> -		retval = -EINVAL;
> +	pr_warn_once("Non-hierarchical mode is deprecated. "
> +		     "Please report your usecase to linux-mm@kvack.org if you "
> +		     "depend on this functionality.\n");
>  
> -	return retval;
> +	return -EINVAL;
>  }
>  
>  static unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
> @@ -3727,8 +3700,6 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg)
>  		child = mem_cgroup_from_css(css);
>  		BUG_ON(child->kmemcg_id != kmemcg_id);
>  		child->kmemcg_id = parent->kmemcg_id;
> -		if (!memcg->use_hierarchy)
> -			break;
>  	}
>  	rcu_read_unlock();
>  
> @@ -5319,38 +5290,22 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
>  	if (parent) {
>  		memcg->swappiness = mem_cgroup_swappiness(parent);
>  		memcg->oom_kill_disable = parent->oom_kill_disable;
> -	}
> -	if (!parent) {
> -		page_counter_init(&memcg->memory, NULL);
> -		page_counter_init(&memcg->swap, NULL);
> -		page_counter_init(&memcg->kmem, NULL);
> -		page_counter_init(&memcg->tcpmem, NULL);
> -	} else if (parent->use_hierarchy) {
> -		memcg->use_hierarchy = true;
> +
>  		page_counter_init(&memcg->memory, &parent->memory);
>  		page_counter_init(&memcg->swap, &parent->swap);
>  		page_counter_init(&memcg->kmem, &parent->kmem);
>  		page_counter_init(&memcg->tcpmem, &parent->tcpmem);
>  	} else {
> -		page_counter_init(&memcg->memory, &root_mem_cgroup->memory);
> -		page_counter_init(&memcg->swap, &root_mem_cgroup->swap);
> -		page_counter_init(&memcg->kmem, &root_mem_cgroup->kmem);
> -		page_counter_init(&memcg->tcpmem, &root_mem_cgroup->tcpmem);
> -		/*
> -		 * 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 != root_mem_cgroup)
> -			memory_cgrp_subsys.broken_hierarchy = true;
> -	}
> +		page_counter_init(&memcg->memory, NULL);
> +		page_counter_init(&memcg->swap, NULL);
> +		page_counter_init(&memcg->kmem, NULL);
> +		page_counter_init(&memcg->tcpmem, NULL);
>  
> -	/* The following stuff does not apply to the root */
> -	if (!parent) {
>  		root_mem_cgroup = memcg;
>  		return &memcg->css;
>  	}
>  
> +	/* The following stuff does not apply to the root */
>  	error = memcg_online_kmem(memcg);
>  	if (error)
>  		goto fail;
> @@ -6187,24 +6142,6 @@ static void mem_cgroup_move_task(void)
>  }
>  #endif
>  
> -/*
> - * Cgroup retains root cgroups across [un]mount cycles making it necessary
> - * to verify whether we're attached to the default hierarchy on each mount
> - * attempt.
> - */
> -static void mem_cgroup_bind(struct cgroup_subsys_state *root_css)
> -{
> -	/*
> -	 * use_hierarchy is forced on the default hierarchy.  cgroup core
> -	 * guarantees that @root doesn't have any children, so turning it
> -	 * on for the root memcg is enough.
> -	 */
> -	if (cgroup_subsys_on_dfl(memory_cgrp_subsys))
> -		root_mem_cgroup->use_hierarchy = true;
> -	else
> -		root_mem_cgroup->use_hierarchy = false;
> -}
> -
>  static int seq_puts_memcg_tunable(struct seq_file *m, unsigned long value)
>  {
>  	if (value == PAGE_COUNTER_MAX)
> @@ -6542,7 +6479,6 @@ struct cgroup_subsys memory_cgrp_subsys = {
>  	.can_attach = mem_cgroup_can_attach,
>  	.cancel_attach = mem_cgroup_cancel_attach,
>  	.post_attach = mem_cgroup_move_task,
> -	.bind = mem_cgroup_bind,
>  	.dfl_cftypes = memory_files,
>  	.legacy_cftypes = mem_cgroup_legacy_files,
>  	.early_init = 0,
> -- 
> 2.26.2

-- 
Michal Hocko
SUSE Labs

Re: [PATCH rfc 2/3] docs: cgroup-v1: reflect the deprecation of the non-hierarchical mode

[Michal Hocko]

On Tue 03-11-20 13:27:24, Roman Gushchin wrote:
> Update cgroup v1 docs after the deprecation of the non-hierarchical
> mode of the memory controller.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  .../admin-guide/cgroup-v1/memcg_test.rst      |  8 ++--
>  .../admin-guide/cgroup-v1/memory.rst          | 40 ++++++-------------
>  2 files changed, 16 insertions(+), 32 deletions(-)
> 
> diff --git a/Documentation/admin-guide/cgroup-v1/memcg_test.rst b/Documentation/admin-guide/cgroup-v1/memcg_test.rst
> index 3f7115e07b5d..4f83de2dab6e 100644
> --- a/Documentation/admin-guide/cgroup-v1/memcg_test.rst
> +++ b/Documentation/admin-guide/cgroup-v1/memcg_test.rst
> @@ -219,13 +219,11 @@ Under below explanation, we assume CONFIG_MEM_RES_CTRL_SWAP=y.
>  
>  	This is an easy way to test page migration, too.
>  
> -9.5 mkdir/rmdir
> ----------------
> +9.5 nested cgroups
> +------------------
>  
> -	When using hierarchy, mkdir/rmdir test should be done.
> -	Use tests like the following::
> +	Use tests like the following for testing nested cgroups::
>  
> -		echo 1 >/opt/cgroup/01/memory/use_hierarchy
>  		mkdir /opt/cgroup/01/child_a
>  		mkdir /opt/cgroup/01/child_b
>  
> diff --git a/Documentation/admin-guide/cgroup-v1/memory.rst b/Documentation/admin-guide/cgroup-v1/memory.rst
> index 12757e63b26c..a44cd467d218 100644
> --- a/Documentation/admin-guide/cgroup-v1/memory.rst
> +++ b/Documentation/admin-guide/cgroup-v1/memory.rst
> @@ -77,6 +77,8 @@ Brief summary of control files.
>   memory.soft_limit_in_bytes	     set/show soft limit of memory usage
>   memory.stat			     show various statistics
>   memory.use_hierarchy		     set/show hierarchical account enabled
> +                                     This knob is deprecated and shouldn't be
> +                                     used.
>   memory.force_empty		     trigger forced page reclaim
>   memory.pressure_level		     set memory pressure notifications
>   memory.swappiness		     set/show swappiness parameter of vmscan
> @@ -495,16 +497,13 @@ cgroup might have some charge associated with it, even though all
>  tasks have migrated away from it. (because we charge against pages, not
>  against tasks.)
>  
> -We move the stats to root (if use_hierarchy==0) or parent (if
> -use_hierarchy==1), and no change on the charge except uncharging
> +We move the stats to parent, and no change on the charge except uncharging
>  from the child.
>  
>  Charges recorded in swap information is not updated at removal of cgroup.
>  Recorded information is discarded and a cgroup which uses swap (swapcache)
>  will be charged as a new owner of it.
>  
> -About use_hierarchy, see Section 6.
> -
>  5. Misc. interfaces
>  ===================
>  
> @@ -527,8 +526,6 @@ About use_hierarchy, see Section 6.
>    write will still return success. In this case, it is expected that
>    memory.kmem.usage_in_bytes == memory.usage_in_bytes.
>  
> -  About use_hierarchy, see Section 6.
> -
>  5.2 stat file
>  -------------
>  
> @@ -675,31 +672,20 @@ hierarchy::
>  		      d   e
>  
>  In the diagram above, with hierarchical accounting enabled, all memory
> -usage of e, is accounted to its ancestors up until the root (i.e, c and root),
> -that has memory.use_hierarchy enabled. If one of the ancestors goes over its
> -limit, the reclaim algorithm reclaims from the tasks in the ancestor and the
> -children of the ancestor.
> -
> -6.1 Enabling hierarchical accounting and reclaim
> -------------------------------------------------
> -
> -A memory cgroup by default disables the hierarchy feature. Support
> -can be enabled by writing 1 to memory.use_hierarchy file of the root cgroup::
> +usage of e, is accounted to its ancestors up until the root (i.e, c and root).
> +If one of the ancestors goes over its limit, the reclaim algorithm reclaims
> +from the tasks in the ancestor and the children of the ancestor.
>  
> -	# echo 1 > memory.use_hierarchy
> -
> -The feature can be disabled by::
> +6.1 Hierarchical accounting and reclaim
> +---------------------------------------
>  
> -	# echo 0 > memory.use_hierarchy
> +Hierarchical accounting is enabled by default. Disabling the hierarchical
> +accounting is deprecated. An attempt to do it will result in a failure
> +and a warning printed to dmesg.
>  
> -NOTE1:
> -       Enabling/disabling will fail if either the cgroup already has other
> -       cgroups created below it, or if the parent cgroup has use_hierarchy
> -       enabled.
> +For compatibility reasons writing 1 to memory.use_hierarchy will always pass::
>  
> -NOTE2:
> -       When panic_on_oom is set to "2", the whole system will panic in
> -       case of an OOM event in any cgroup.
> +	# echo 1 > memory.use_hierarchy
>  
>  7. Soft limits
>  ==============
> -- 
> 2.26.2

-- 
Michal Hocko
SUSE Labs

Re: [PATCH rfc 1/3] mm: memcg: deprecate the non-hierarchical mode

[Shakeel Butt]

On Tue, Nov 3, 2020 at 1:27 PM Roman Gushchin <guro@fb.com> wrote:
>
> The non-hierarchical cgroup v1 mode is a legacy of early days
> of the memory controller and doesn't bring any value today.
> However, it complicates the code and creates many edge cases
> all over the memory controller code.
>
> It's a good time to deprecate it completely.
>
> Functionally this patch enabled is by default for all cgroups
> and forbids switching it off. Nothing changes if cgroup v2 is used:
> hierarchical mode was enforced from scratch.
>
> To protect the ABI memory.use_hierarchy interface is preserved
> with a limited functionality: reading always returns "1", writing
> of "1" passes silently, writing of any other value fails with
> -EINVAL and a warning to dmesg (on the first occasion).
>
> Signed-off-by: Roman Gushchin <guro@fb.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH rfc 2/3] docs: cgroup-v1: reflect the deprecation of the non-hierarchical mode

[Shakeel Butt]

On Tue, Nov 3, 2020 at 1:27 PM Roman Gushchin <guro@fb.com> wrote:
>
> Update cgroup v1 docs after the deprecation of the non-hierarchical
> mode of the memory controller.
>
> Signed-off-by: Roman Gushchin <guro@fb.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH rfc 3/3] cgroup: remove obsoleted broken_hierarchy and warned_broken_hierarchy

[Shakeel Butt]

On Tue, Nov 3, 2020 at 1:27 PM Roman Gushchin <guro@fb.com> wrote:
>
> With the deprecation of the non-hierarchical mode of the memory
> controller there are no more examples of broken hierarchies left.
>
> Let's remove the cgroup core code which was supposed to print
> warnings about creating of broken hierarchies.
>
> Signed-off-by: Roman Gushchin <guro@fb.com>

Reviewed-by: Shakeel Butt <shakeelb@google.com>

Re: [PATCH rfc 1/3] mm: memcg: deprecate the non-hierarchical mode

[Roman Gushchin]

On Wed, Nov 04, 2020 at 09:37:46AM +0100, Michal Hocko wrote:
> On Tue 03-11-20 13:27:23, Roman Gushchin wrote:
> > The non-hierarchical cgroup v1 mode is a legacy of early days
> > of the memory controller and doesn't bring any value today.
> 
> This is a bold statement ;)
> All that we know today is that we have a warning in place to complain
> loudly when somebody relies on use_hierarchy=0 with a deeper
> hierarchy. For all those years we have seen _zero_ reports that would
> describe a sensible usecase.
> Moreover we (SUSE) have backported this warning into old distribution
> kernels (since 3.0 based kernels) to extend the coverage and didn't hear
> even for users who adopt new kernels only very slowly. The only report
> we have seen so far was a LTP test suite which doesn't really reflect
> any real life usecase.

Good to know, thank you for providing this information. I'm also
not aware of any users of the non-hierarchical mode.

> 
> Feel free to reuse the above in the changelog.
> 
> > However, it complicates the code and creates many edge cases
> > all over the memory controller code.
> > 
> > It's a good time to deprecate it completely.
> > 
> > Functionally this patch enabled is by default for all cgroups
> > and forbids switching it off. Nothing changes if cgroup v2 is used:
> > hierarchical mode was enforced from scratch.
> > 
> > To protect the ABI memory.use_hierarchy interface is preserved
> > with a limited functionality: reading always returns "1", writing
> > of "1" passes silently, writing of any other value fails with
> > -EINVAL and a warning to dmesg (on the first occasion).
> 
> Yes, that makes sense.
>  
> > Signed-off-by: Roman Gushchin <guro@fb.com>
> 
> I do not see any problems with the patch or any left overs behind
> (except for the documentation which you handle in the follow up
> patches).
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks and let's see whether some last minute usecase show up.

Thank you!

Roman

Re: [PATCH rfc 0/3] mm: memcg: deprecate cgroup v1 non-hierarchical mode

[David Rientjes]

On Tue, 3 Nov 2020, Roman Gushchin wrote:

> The non-hierarchical cgroup v1 mode is a legacy of early days
> of the memory controller and doesn't bring any value today.
> However, it complicates the code and creates many edge cases
> all over the memory controller code.
> 
> It's a good time to deprecate it completely. This patchset removes
> the internal logic, adjusts the user interface and updates
> the documentation. The alt patch removes some bits of the cgroup
> core code, which become obsolete.
> 
> 
> Roman Gushchin (3):
>   mm: memcg: deprecate the non-hierarchical mode
>   docs: cgroup-v1: reflect the deprecation of the non-hierarchical mode
>   cgroup: remove obsoleted broken_hierarchy and warned_broken_hierarchy
> 

For all three patches:

Acked-by: David Rientjes <rientjes@google.com>

Very welcome change to see, we've always prevented the non-hierarchical 
mode from being set in our kernel.