* [PATCH resend] mm, memcg: fix inconsistent oom event behavior
@ 2020-05-02 14:10 Yafang Shao
2020-05-02 14:45 ` Johannes Weiner
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Yafang Shao @ 2020-05-02 14:10 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, Yafang Shao, Chris Down, Michal Hocko, Johannes Weiner
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.
Fixes: 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
Reviewed-by: Shakeel Butt <shakeelb@google.com>
Cc: Chris Down <chris@chrisdown.name>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
include/linux/memcontrol.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index d275c72c4f8e..977edd3b7bd8 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -783,6 +783,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)) &&
--
2.18.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH resend] mm, memcg: fix inconsistent oom event behavior
2020-05-02 14:10 [PATCH resend] mm, memcg: fix inconsistent oom event behavior Yafang Shao
@ 2020-05-02 14:45 ` Johannes Weiner
2020-05-02 15:23 ` Chris Down
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Johannes Weiner @ 2020-05-02 14:45 UTC (permalink / raw)
To: Yafang Shao; +Cc: akpm, linux-mm, Chris Down, Michal Hocko
On Sat, May 02, 2020 at 10:10:55AM -0400, Yafang Shao 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.
>
> 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.
>
> Fixes: 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
> Reviewed-by: Shakeel Butt <shakeelb@google.com>
> Cc: Chris Down <chris@chrisdown.name>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH resend] mm, memcg: fix inconsistent oom event behavior
2020-05-02 14:10 [PATCH resend] mm, memcg: fix inconsistent oom event behavior Yafang Shao
2020-05-02 14:45 ` Johannes Weiner
@ 2020-05-02 15:23 ` Chris Down
2020-05-04 7:54 ` Michal Hocko
2020-05-04 23:03 ` Andrew Morton
3 siblings, 0 replies; 6+ messages in thread
From: Chris Down @ 2020-05-02 15:23 UTC (permalink / raw)
To: Yafang Shao; +Cc: akpm, linux-mm, Michal Hocko, Johannes Weiner
Yafang Shao writes:
>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.
Thanks!
Acked-by: Chris Down <chris@chrisdown.name>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH resend] mm, memcg: fix inconsistent oom event behavior
2020-05-02 14:10 [PATCH resend] mm, memcg: fix inconsistent oom event behavior Yafang Shao
2020-05-02 14:45 ` Johannes Weiner
2020-05-02 15:23 ` Chris Down
@ 2020-05-04 7:54 ` Michal Hocko
2020-05-04 23:03 ` Andrew Morton
3 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2020-05-04 7:54 UTC (permalink / raw)
To: Yafang Shao; +Cc: akpm, linux-mm, Chris Down, Johannes Weiner
On Sat 02-05-20 10:10:55, Yafang Shao 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.
>
> 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.
>
> Fixes: 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
> Reviewed-by: Shakeel Butt <shakeelb@google.com>
> Cc: Chris Down <chris@chrisdown.name>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
and sorry to distract you to a cgroup generic solution without doing my
homework and double checking it is possible.
> ---
> include/linux/memcontrol.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index d275c72c4f8e..977edd3b7bd8 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -783,6 +783,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)) &&
> --
> 2.18.2
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH resend] mm, memcg: fix inconsistent oom event behavior
2020-05-02 14:10 [PATCH resend] mm, memcg: fix inconsistent oom event behavior Yafang Shao
` (2 preceding siblings ...)
2020-05-04 7:54 ` Michal Hocko
@ 2020-05-04 23:03 ` Andrew Morton
2020-05-05 7:29 ` Michal Hocko
3 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2020-05-04 23:03 UTC (permalink / raw)
To: Yafang Shao; +Cc: linux-mm, Chris Down, Michal Hocko, Johannes Weiner
On Sat, 2 May 2020 10:10:55 -0400 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.
>
> 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.
>
> Fixes: 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
Nearly a year ago. Should we backport this into earlier kernels?
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -783,6 +783,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)) &&
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH resend] mm, memcg: fix inconsistent oom event behavior
2020-05-04 23:03 ` Andrew Morton
@ 2020-05-05 7:29 ` Michal Hocko
0 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2020-05-05 7:29 UTC (permalink / raw)
To: Andrew Morton; +Cc: Yafang Shao, linux-mm, Chris Down, Johannes Weiner
On Mon 04-05-20 16:03:45, Andrew Morton wrote:
> On Sat, 2 May 2020 10:10:55 -0400 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.
> >
> > 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.
> >
> > Fixes: 9852ae3fe529 ("mm, memcg: consider subtrees in memory.events")
>
> Nearly a year ago. Should we backport this into earlier kernels?
It is a trivial change so I do not see problem marking it for stable.
>
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -783,6 +783,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)) &&
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-05 7:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-02 14:10 [PATCH resend] mm, memcg: fix inconsistent oom event behavior Yafang Shao
2020-05-02 14:45 ` Johannes Weiner
2020-05-02 15:23 ` Chris Down
2020-05-04 7:54 ` Michal Hocko
2020-05-04 23:03 ` Andrew Morton
2020-05-05 7:29 ` Michal Hocko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).