mm, memcg: fix inconsistent oom event behavior

PATCH In review Yafang Shao 4 participants 7 messages Apr 22, 2020

Subsystems (2)

Memory management memcg

lore

Share & tools

lore

Yafang Shao

author

2020-04-22 11:07 UTC #0 lore raw

PATCH v2 1/1 mm, memcg: fix inconsistent oom event behavior

A recent commit 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") changes the behavior of memcg events, which will consider subtrees in memory.events. But oom_kill event is a special one as it is used in both cgroup1 and cgroup2. In cgroup1, it is displayed in memory.oom_control. The file memory.oom_control is in both root memcg and non root memcg, that is different with memory.event as it only in non-root memcg. That commit is okay for cgroup2, but it is not okay for cgroup1 as it will cause inconsistent behavior between root memcg and non-root memcg.

Here's an example on why this behavior is inconsistent in cgroup1.

     root memcg
     /

memcg foo
/
memcg bar

Suppose there's an oom_kill in memcg bar, then the oon_kill will be

root memcg : memory.oom_control(oom_kill) 0
/
memcg foo : memory.oom_control(oom_kill) 1
/
memcg bar : memory.oom_control(oom_kill) 1

For the non-root memcg, its memory.oom_control(oom_kill) includes its descendants' oom_kill, but for root memcg, it doesn't include its descendants' oom_kill. That means, memory.oom_control(oom_kill) has different meanings in different memcgs. That is inconsistent. Then the user has to know whether the memcg is root or not.

If we can't fully support it in cgroup1, for example by adding memory.events.local into cgroup1 as well, then let's don't touch its original behavior.

Setting CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy by default rather than special casing it somewhere quite deep in the code would be better, per discussion with Michal.

Fixes 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")

Cc Chris Down <[email protected]>

Cc Shakeel Butt <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Johannes Weiner <[email protected]>

Signed-off-by Yafang Shao <[email protected]>

---
 mm/memcontrol.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

mm/memcontrol.c +12 -2

index 5beea03dd58a..0f7381bddcee 100644
@@ -5940,10 +5940,20 @@ static void mem_cgroup_bind(struct cgroup_subsys_state *root_css)
 	 * 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))
+	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
 		root_mem_cgroup->use_hierarchy = true;
-	else
+	} else {
 		root_mem_cgroup->use_hierarchy = false;
+		/*
+		 * Set CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy
+		 * by default to avoid inconsistent oom_kill behavior
+		 * between root memcg and non-root memcg.
+		 * Regarding default hierarchy, as this flag will be set
+		 * or cleared later, we don't need to process it in this
+		 * function.
+		 */
+		cgrp_dfl_root.flags |= CGRP_ROOT_MEMORY_LOCAL_EVENTS;
+	}
 }
 
 static int seq_puts_memcg_tunable(struct seq_file *m, unsigned long value)

Signature · 1 line

2.18.2

Michal Hocko

maintainer Acked-by on v2 1/1

2020-04-22 11:51 UTC #1 lore raw

Quoted 31 lines · Yafang Shao

A recent commit 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") changes the behavior of memcg events, which will consider subtrees in memory.events. But oom_kill event is a special one as it is used in both cgroup1 and cgroup2. In cgroup1, it is displayed in memory.oom_control. The file memory.oom_control is in both root memcg and non root memcg, that is different with memory.event as it only in non-root memcg. That commit is okay for cgroup2, but it is not okay for cgroup1 as it will cause inconsistent behavior between root memcg and non-root memcg.
Here's an example on why this behavior is inconsistent in cgroup1.
     root memcg
     /
memcg foo
/
memcg bar
Suppose there's an oom_kill in memcg bar, then the oon_kill will be
root memcg : memory.oom_control(oom_kill) 0
/
memcg foo : memory.oom_control(oom_kill) 1
/
memcg bar : memory.oom_control(oom_kill) 1
For the non-root memcg, its memory.oom_control(oom_kill) includes its descendants' oom_kill, but for root memcg, it doesn't include its descendants' oom_kill. That means, memory.oom_control(oom_kill) has different meanings in different memcgs. That is inconsistent. Then the user has to know whether the memcg is root or not.

Quoted 7 lines

If we can't fully support it in cgroup1, for example by adding memory.events.local into cgroup1 as well, then let's don't touch its original behavior.
Setting CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy by default rather than special casing it somewhere quite deep in the code would be better, per discussion with Michal.

OK, this makes sense to me. Cgroup v1 really had local semantic and 9852ae3fe529 changed it unintentionally. I think it is reasonable to use the CGRP_ROOT_MEMORY_LOCAL_EVENTS which denotes this mode but I will defer to cgroup maintainers. Maybe there are some other side effects which I am not aware of that would make this more awkward than a special case for cgroup v1

Quoted 6 lines

Fixes 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
Cc Chris Down <[email protected]>
Cc Shakeel Butt <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Signed-off-by Yafang Shao <[email protected]>

Acked-by Michal Hocko <[email protected]>

Quoted 16 lines

---
 mm/memcontrol.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

mm/memcontrol.c +1 -2

index 5beea03dd58a..0f7381bddcee 100644
@@ -5940,10 +5940,20 @@ static void mem_cgroup_bind(struct cgroup_subsys_state *root_css)
 	 * 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))
+	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
 		root_mem_cgroup->use_hierarchy = true;
-	else

mm/memcontrol.c static void mem_cgroup_bind(struct cgroup_subsys_state *root_css) · line 5954

Quoted 10 lines

mm/memcontrol.c +9 -0

+	} else {
 		root_mem_cgroup->use_hierarchy = false;
+		/*
+		 * Set CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy
+		 * by default to avoid inconsistent oom_kill behavior
+		 * between root memcg and non-root memcg.
+		 * Regarding default hierarchy, as this flag will be set
+		 * or cleared later, we don't need to process it in this
+		 * function.
+		 */

I do not think the comment has to be so specific about oom events behavior. I would just go with
/*
* Cgroup v1 has traditionally had local semantic for
* event counters. Cgroup v2 changed that to a
* hierarchical behavior. This is expressed by
* CGRP_ROOT_MEMORY_LOCAL_EVENTS in the cgroup core. */

Quoted 7 lines

mm/memcontrol.c +2 -0

+		cgrp_dfl_root.flags |= CGRP_ROOT_MEMORY_LOCAL_EVENTS;
+	}
 }
 
 static int seq_puts_memcg_tunable(struct seq_file *m, unsigned long value)

Signature · 1 line

2.18.2

Signature · 2 lines

Michal Hocko
SUSE Labs

Johannes Weiner

maintainer

2020-04-22 12:54 UTC #2 lore raw

In reply to Yafang Shao

Quoted 62 lines · Yafang Shao

A recent commit 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") changes the behavior of memcg events, which will consider subtrees in memory.events. But oom_kill event is a special one as it is used in both cgroup1 and cgroup2. In cgroup1, it is displayed in memory.oom_control. The file memory.oom_control is in both root memcg and non root memcg, that is different with memory.event as it only in non-root memcg. That commit is okay for cgroup2, but it is not okay for cgroup1 as it will cause inconsistent behavior between root memcg and non-root memcg.
Here's an example on why this behavior is inconsistent in cgroup1.
     root memcg
     /
memcg foo
/
memcg bar
Suppose there's an oom_kill in memcg bar, then the oon_kill will be
root memcg : memory.oom_control(oom_kill) 0
/
memcg foo : memory.oom_control(oom_kill) 1
/
memcg bar : memory.oom_control(oom_kill) 1
For the non-root memcg, its memory.oom_control(oom_kill) includes its descendants' oom_kill, but for root memcg, it doesn't include its descendants' oom_kill. That means, memory.oom_control(oom_kill) has different meanings in different memcgs. That is inconsistent. Then the user has to know whether the memcg is root or not.
If we can't fully support it in cgroup1, for example by adding memory.events.local into cgroup1 as well, then let's don't touch its original behavior.
Setting CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy by default rather than special casing it somewhere quite deep in the code would be better, per discussion with Michal.
Fixes 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
Cc Chris Down <[email protected]>
Cc Shakeel Butt <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Signed-off-by Yafang Shao <[email protected]>
---
 mm/memcontrol.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
mm/memcontrol.c +2 -2
index 5beea03dd58a..0f7381bddcee 100644
@@ -5940,10 +5940,20 @@ static void mem_cgroup_bind(struct cgroup_subsys_state *root_css)
 	 * 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))
+	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
 		root_mem_cgroup->use_hierarchy = true;
-	else
+	} else {

mm/memcontrol.c static void mem_cgroup_bind(struct cgroup_subsys_state *root_css) · line 5955

Quoted 10 lines

mm/memcontrol.c +9 -0

 		root_mem_cgroup->use_hierarchy = false;
+		/*
+		 * Set CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy
+		 * by default to avoid inconsistent oom_kill behavior
+		 * between root memcg and non-root memcg.
+		 * Regarding default hierarchy, as this flag will be set
+		 * or cleared later, we don't need to process it in this
+		 * function.
+		 */
+		cgrp_dfl_root.flags |= CGRP_ROOT_MEMORY_LOCAL_EVENTS;

That will cause problems for people trying to remount. From cgroup1_reconfigure():

	/* Don't allow flags or name to change at remount */
	if ((ctx->flags ^ root->flags) ||
	    (ctx->name && strcmp(ctx->name, root->name))) {
		errorfc(fc, "option or name mismatch, new: 0x%x \"%s\", old: 0x%x \"%s\"",
		       ctx->flags, ctx->name ?: "", root->flags, root->name);
		ret = -EINVAL;
		goto out_unlock;
	}

These flags belong to the user, they're read-only to the cgroup implementation. Let's not mess with them from a controller.

include/linux/memcontrol.h +2 -0

index 4b868e5a687f..e831a90b5506 100644
@@ -773,6 +773,8 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg,
 		atomic_long_inc(&memcg->memory_events[event]);
 		cgroup_file_notify(&memcg->events_file);
 
+		if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
+			break;
 		if (cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_LOCAL_EVENTS)
 			break;
 	} while ((memcg = parent_mem_cgroup(memcg)) &&

Yafang Shao

author

2020-04-22 12:59 UTC #3 lore raw

Quoted 95 lines · Johannes Weiner

Quoted 62 lines · Yafang Shao
A recent commit 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") changes the behavior of memcg events, which will consider subtrees in memory.events. But oom_kill event is a special one as it is used in both cgroup1 and cgroup2. In cgroup1, it is displayed in memory.oom_control. The file memory.oom_control is in both root memcg and non root memcg, that is different with memory.event as it only in non-root memcg. That commit is okay for cgroup2, but it is not okay for cgroup1 as it will cause inconsistent behavior between root memcg and non-root memcg.
Here's an example on why this behavior is inconsistent in cgroup1.
     root memcg
     /
memcg foo
/
memcg bar
Suppose there's an oom_kill in memcg bar, then the oon_kill will be
root memcg : memory.oom_control(oom_kill) 0
/
memcg foo : memory.oom_control(oom_kill) 1
/
memcg bar : memory.oom_control(oom_kill) 1
For the non-root memcg, its memory.oom_control(oom_kill) includes its descendants' oom_kill, but for root memcg, it doesn't include its descendants' oom_kill. That means, memory.oom_control(oom_kill) has different meanings in different memcgs. That is inconsistent. Then the user has to know whether the memcg is root or not.
If we can't fully support it in cgroup1, for example by adding memory.events.local into cgroup1 as well, then let's don't touch its original behavior.
Setting CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy by default rather than special casing it somewhere quite deep in the code would be better, per discussion with Michal.
Fixes 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
Cc Chris Down <[email protected]>
Cc Shakeel Butt <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Signed-off-by Yafang Shao <[email protected]>
---
 mm/memcontrol.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
mm/memcontrol.c +2 -2
index 5beea03dd58a..0f7381bddcee 100644
@@ -5940,10 +5940,20 @@ static void mem_cgroup_bind(struct cgroup_subsys_state *root_css)
       * 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))
+     if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
              root_mem_cgroup->use_hierarchy = true;
-     else
+     } else {
mm/memcontrol.c static void mem_cgroup_bind(struct cgroup_subsys_state *root_css) · line 5955
Quoted 10 lines
mm/memcontrol.c +9 -0
              root_mem_cgroup->use_hierarchy = false;
+             /*
+              * Set CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy
+              * by default to avoid inconsistent oom_kill behavior
+              * between root memcg and non-root memcg.
+              * Regarding default hierarchy, as this flag will be set
+              * or cleared later, we don't need to process it in this
+              * function.
+              */
+             cgrp_dfl_root.flags |= CGRP_ROOT_MEMORY_LOCAL_EVENTS;
That will cause problems for people trying to remount. From cgroup1_reconfigure():
        /* Don't allow flags or name to change at remount */
        if ((ctx->flags ^ root->flags) ||
            (ctx->name && strcmp(ctx->name, root->name))) {
                errorfc(fc, "option or name mismatch, new: 0x%x \"%s\", old: 0x%x \"%s\"",
                       ctx->flags, ctx->name ?: "", root->flags, root->name);
                ret = -EINVAL;
                goto out_unlock;
        }
These flags belong to the user, they're read-only to the cgroup implementation. Let's not mess with them from a controller.
include/linux/memcontrol.h +0 -0
index 4b868e5a687f..e831a90b5506 100644

include/linux/memcontrol.h static inline void memcg_memory_event(struct mem_cgroup *memcg, · line 780

Quoted 8 lines

include/linux/memcontrol.h +2 -0

@@ -773,6 +773,8 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg,
                atomic_long_inc(&memcg->memory_events[event]);
                cgroup_file_notify(&memcg->events_file);
 
+               if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
+                       break;
                if (cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_LOCAL_EVENTS)
                        break;
        } while ((memcg = parent_mem_cgroup(memcg)) &&

Hi Johnanes,

That is what I did in the previous version, see also
https://lore.kernel.org/linux-mm/[email protected]/
.

Signature · 2 lines

Thanks
Yafang

Chris Down

2020-04-22 13:02 UTC #4 lore raw

Quoted 2 lines · Yafang Shao

That is what I did in the previous version, see also
https://lore.kernel.org/linux-mm/[email protected]/

Your v1 patch was significantly less complicated and self-contained, and I would ack it (whereas I wouldn't ack this because it really complicates matters for localevents). My only questions were around quantifying the issue in the changelog :-)

Yafang Shao

author

2020-04-22 13:16 UTC #5 lore raw

Quoted 9 lines · Chris Down

Quoted 2 lines · Yafang Shao
That is what I did in the previous version, see also
https://lore.kernel.org/linux-mm/[email protected]/
Your v1 patch was significantly less complicated and self-contained, and I would ack it (whereas I wouldn't ack this because it really complicates matters for localevents). My only questions were around quantifying the issue in the changelog :-)

The user tools parsing the memory.oom_control will be affected. I thought I have explained it clearly in the changelog :)

Signature · 2 lines

Thanks
Yafang

Michal Hocko

maintainer

2020-04-22 13:15 UTC #6 lore raw

In reply to Johannes Weiner

Quoted 77 lines · Johannes Weiner

Quoted 62 lines · Yafang Shao
A recent commit 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") changes the behavior of memcg events, which will consider subtrees in memory.events. But oom_kill event is a special one as it is used in both cgroup1 and cgroup2. In cgroup1, it is displayed in memory.oom_control. The file memory.oom_control is in both root memcg and non root memcg, that is different with memory.event as it only in non-root memcg. That commit is okay for cgroup2, but it is not okay for cgroup1 as it will cause inconsistent behavior between root memcg and non-root memcg.
Here's an example on why this behavior is inconsistent in cgroup1.
     root memcg
     /
memcg foo
/
memcg bar
Suppose there's an oom_kill in memcg bar, then the oon_kill will be
root memcg : memory.oom_control(oom_kill) 0
/
memcg foo : memory.oom_control(oom_kill) 1
/
memcg bar : memory.oom_control(oom_kill) 1
For the non-root memcg, its memory.oom_control(oom_kill) includes its descendants' oom_kill, but for root memcg, it doesn't include its descendants' oom_kill. That means, memory.oom_control(oom_kill) has different meanings in different memcgs. That is inconsistent. Then the user has to know whether the memcg is root or not.
If we can't fully support it in cgroup1, for example by adding memory.events.local into cgroup1 as well, then let's don't touch its original behavior.
Setting CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy by default rather than special casing it somewhere quite deep in the code would be better, per discussion with Michal.
Fixes 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
Cc Chris Down <[email protected]>
Cc Shakeel Butt <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Signed-off-by Yafang Shao <[email protected]>
---
 mm/memcontrol.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
mm/memcontrol.c +2 -2
index 5beea03dd58a..0f7381bddcee 100644
@@ -5940,10 +5940,20 @@ static void mem_cgroup_bind(struct cgroup_subsys_state *root_css)
 	 * 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))
+	if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
 		root_mem_cgroup->use_hierarchy = true;
-	else
+	} else {
mm/memcontrol.c static void mem_cgroup_bind(struct cgroup_subsys_state *root_css) · line 5955
Quoted 10 lines
mm/memcontrol.c +9 -0
 		root_mem_cgroup->use_hierarchy = false;
+		/*
+		 * Set CGRP_ROOT_MEMORY_LOCAL_EVENTS for legacy hierarchy
+		 * by default to avoid inconsistent oom_kill behavior
+		 * between root memcg and non-root memcg.
+		 * Regarding default hierarchy, as this flag will be set
+		 * or cleared later, we don't need to process it in this
+		 * function.
+		 */
+		cgrp_dfl_root.flags |= CGRP_ROOT_MEMORY_LOCAL_EVENTS;
That will cause problems for people trying to remount. From cgroup1_reconfigure():

Quoted 8 lines

	/* Don't allow flags or name to change at remount */
	if ((ctx->flags ^ root->flags) ||
	    (ctx->name && strcmp(ctx->name, root->name))) {
		errorfc(fc, "option or name mismatch, new: 0x%x \"%s\", old: 0x%x \"%s\"",
		       ctx->flags, ctx->name ?: "", root->flags, root->name);
		ret = -EINVAL;
		goto out_unlock;
	}

OK, I was not aware of this restriction. Under these circumstances special casing in memcg_memory_event is the right approach.

Quoted 16 lines

These flags belong to the user, they're read-only to the cgroup implementation. Let's not mess with them from a controller.

include/linux/memcontrol.h +2 -0

index 4b868e5a687f..e831a90b5506 100644
@@ -773,6 +773,8 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg,
 		atomic_long_inc(&memcg->memory_events[event]);
 		cgroup_file_notify(&memcg->events_file);
 
+		if (!cgroup_subsys_on_dfl(memory_cgrp_subsys))
+			break;
 		if (cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_LOCAL_EVENTS)
 			break;
 	} while ((memcg = parent_mem_cgroup(memcg)) &&

Signature · 2 lines

Michal Hocko
SUSE Labs