On Mon, Apr 13, 2020 at 5:58 PM Yafang Shao laoar.shao@gmail.com wrote:
On Tue, Apr 14, 2020 at 8:53 AM Shakeel Butt shakeelb@google.com wrote:
On Mon, Apr 13, 2020 at 5:36 PM Yafang Shao laoar.shao@gmail.com wrote:
On Tue, Apr 14, 2020 at 1:06 AM Shakeel Butt shakeelb@google.com wrote:
On Sun, Apr 12, 2020 at 7:04 AM Yafang Shao laoar.shao@gmail.com wrote:
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.
I still couldn't understand the cgroup v1's root vs non_root behavior change. The behavior change I see is the hierarchical one i.e. MEMCG_OOM_KILL event in the descendant will cause the notification and count increment in the ancestors even in the cgroup v1.
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.
[snip]
I suppose we don't want that behavior change in v1.
That is another topic. I think this feature is welcomed to cgroup1, if we can fully support it, for example by adding memory.events.local into cgroup1 as well, but as far as I know the cgroup1 is frozen.
Please note that after your patch the non-root memcg's memory.oom_control(oom_kill) will not include the descendant's oom_kill anymore. The non-root and root memcg's memory.oom_control(oom_kill) will not be hierarchical anymore but consistent. I think that was the intention of the patch, right?
Right. If we can't fully support it in cgroup1, then let's don't touch its original behavior.
Agreed.
Let's recover the original behavior for cgroup1.
Fixes: 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events") Cc: Chris Down chris@chrisdown.name Cc: Johannes Weiner hannes@cmpxchg.org Cc: Shakeel Butt shakeelb@google.com Cc: stable@vger.kernel.org Signed-off-by: Yafang Shao laoar.shao@gmail.com
Reviewed-by: Shakeel Butt shakeelb@google.com
include/linux/memcontrol.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 8c340e6b347f..a0ae080a67d1 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -798,7 +798,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 (cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_LOCAL_EVENTS)
if (cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_LOCAL_EVENTS ||!cgroup_subsys_on_dfl(memory_cgrp_subsys)) break; } while ((memcg = parent_mem_cgroup(memcg)) && !mem_cgroup_is_root(memcg));-- 2.18.2
Thanks Yafang