| Message ID | 20210429185457.23381-5-tim.gardner@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | nr_writeback memory leak in kernel 4.15.0-137+ | expand |
On 29/04/2021 20:54, Tim Gardner wrote: > From: Roman Gushchin <guro@fb.com> > > BugLink: https://bugs.launchpad.net/bugs/1926081 > > Commit e27be240df53 ("mm: memcg: make sure memory.events is uptodate > when waking pollers") converted most of memcg event counters to > per-memcg atomics, which made them less confusing for a user. The > "oom_kill" counter remained untouched, so now it behaves differently > than other counters (including "oom"). This adds nothing but confusion. > > Let's fix this by adding the MEMCG_OOM_KILL event, and follow the > MEMCG_OOM approach. > > This also removes a hack from count_memcg_event_mm(), introduced earlier > specially for the OOM_KILL counter. > > [akpm@linux-foundation.org: fix for droppage of memcg-replace-mm-owner-with-mm-memcg.patch] > Link: http://lkml.kernel.org/r/20180508124637.29984-1-guro@fb.com > Signed-off-by: Roman Gushchin <guro@fb.com> > Acked-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > Acked-by: Michal Hocko <mhocko@suse.com> > Cc: Vladimir Davydov <vdavydov.dev@gmail.com> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (backported from commit fe6bdfc8e1e131720abbe77a2eb990c94c9024cb) > [rtg - context adjustments] > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > include/linux/memcontrol.h | 26 ++++++++++++++++++++++---- > mm/memcontrol.c | 6 ++++-- > mm/oom_kill.c | 2 +- > 3 files changed, 27 insertions(+), 7 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index c7876eadd206d..b5cd86e320ff3 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -53,6 +53,7 @@ enum memcg_memory_event { > MEMCG_HIGH, > MEMCG_MAX, > MEMCG_OOM, > + MEMCG_OOM_KILL, > MEMCG_NR_MEMORY_EVENTS, > }; > > @@ -706,11 +707,8 @@ static inline void count_memcg_event_mm(struct mm_struct *mm, > > rcu_read_lock(); > memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > - if (likely(memcg)) { > + if (likely(memcg)) > count_memcg_events(memcg, idx, 1); > - if (idx == OOM_KILL) > - cgroup_file_notify(&memcg->events_file); > - } > rcu_read_unlock(); > } > > @@ -721,6 +719,21 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg, > cgroup_file_notify(&memcg->events_file); > } > > +static inline void memcg_memory_event_mm(struct mm_struct *mm, > + enum memcg_memory_event event) > +{ > + struct mem_cgroup *memcg; > + > + if (mem_cgroup_disabled()) > + return; > + > + rcu_read_lock(); > + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > + if (likely(memcg)) > + memcg_memory_event(memcg, event); > + rcu_read_unlock(); > +} I cannot find memcg_memory_event() in bionic/linux. Does this compile? Best regards, Krzysztof
On 4/30/21 1:36 AM, Krzysztof Kozlowski wrote: > On 29/04/2021 20:54, Tim Gardner wrote: >> From: Roman Gushchin <guro@fb.com> >> >> BugLink: https://bugs.launchpad.net/bugs/1926081 >> >> Commit e27be240df53 ("mm: memcg: make sure memory.events is uptodate >> when waking pollers") converted most of memcg event counters to >> per-memcg atomics, which made them less confusing for a user. The >> "oom_kill" counter remained untouched, so now it behaves differently >> than other counters (including "oom"). This adds nothing but confusion. >> >> Let's fix this by adding the MEMCG_OOM_KILL event, and follow the >> MEMCG_OOM approach. >> >> This also removes a hack from count_memcg_event_mm(), introduced earlier >> specially for the OOM_KILL counter. >> >> [akpm@linux-foundation.org: fix for droppage of memcg-replace-mm-owner-with-mm-memcg.patch] >> Link: http://lkml.kernel.org/r/20180508124637.29984-1-guro@fb.com >> Signed-off-by: Roman Gushchin <guro@fb.com> >> Acked-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> >> Acked-by: Johannes Weiner <hannes@cmpxchg.org> >> Acked-by: Michal Hocko <mhocko@suse.com> >> Cc: Vladimir Davydov <vdavydov.dev@gmail.com> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> >> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> >> (backported from commit fe6bdfc8e1e131720abbe77a2eb990c94c9024cb) >> [rtg - context adjustments] >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> >> --- >> include/linux/memcontrol.h | 26 ++++++++++++++++++++++---- >> mm/memcontrol.c | 6 ++++-- >> mm/oom_kill.c | 2 +- >> 3 files changed, 27 insertions(+), 7 deletions(-) >> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h >> index c7876eadd206d..b5cd86e320ff3 100644 >> --- a/include/linux/memcontrol.h >> +++ b/include/linux/memcontrol.h >> @@ -53,6 +53,7 @@ enum memcg_memory_event { >> MEMCG_HIGH, >> MEMCG_MAX, >> MEMCG_OOM, >> + MEMCG_OOM_KILL, >> MEMCG_NR_MEMORY_EVENTS, >> }; >> >> @@ -706,11 +707,8 @@ static inline void count_memcg_event_mm(struct mm_struct *mm, >> >> rcu_read_lock(); >> memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); >> - if (likely(memcg)) { >> + if (likely(memcg)) >> count_memcg_events(memcg, idx, 1); >> - if (idx == OOM_KILL) >> - cgroup_file_notify(&memcg->events_file); >> - } >> rcu_read_unlock(); >> } >> >> @@ -721,6 +719,21 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg, >> cgroup_file_notify(&memcg->events_file); >> } >> >> +static inline void memcg_memory_event_mm(struct mm_struct *mm, >> + enum memcg_memory_event event) >> +{ >> + struct mem_cgroup *memcg; >> + >> + if (mem_cgroup_disabled()) >> + return; >> + >> + rcu_read_lock(); >> + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); >> + if (likely(memcg)) >> + memcg_memory_event(memcg, event); >> + rcu_read_unlock(); >> +} > > I cannot find memcg_memory_event() in bionic/linux. Does this compile? > > Best regards, > Krzysztof > It must have. I made a test kernel. https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1926081/comments/22 gloin:for-review/linux.git nr_writeback-memory-leak-LP#1926081 rtg ----------- Tim Gardner Canonical, Inc
On Fri, Apr 30, 2021 at 09:36:00AM +0200, Krzysztof Kozlowski wrote: > On 29/04/2021 20:54, Tim Gardner wrote: [...] > > @@ -721,6 +719,21 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg, > > cgroup_file_notify(&memcg->events_file); > > } > > > > +static inline void memcg_memory_event_mm(struct mm_struct *mm, > > + enum memcg_memory_event event) > > +{ > > + struct mem_cgroup *memcg; > > + > > + if (mem_cgroup_disabled()) > > + return; > > + > > + rcu_read_lock(); > > + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); > > + if (likely(memcg)) > > + memcg_memory_event(memcg, event); > > + rcu_read_unlock(); > > +} > > I cannot find memcg_memory_event() in bionic/linux. Does this compile? > > Best regards, > Krzysztof It is introduced by patch 2 of this series. Cascardo.
On 30/04/2021 13:37, Thadeu Lima de Souza Cascardo wrote: > On Fri, Apr 30, 2021 at 09:36:00AM +0200, Krzysztof Kozlowski wrote: >> On 29/04/2021 20:54, Tim Gardner wrote: > [...] >>> @@ -721,6 +719,21 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg, >>> cgroup_file_notify(&memcg->events_file); >>> } >>> >>> +static inline void memcg_memory_event_mm(struct mm_struct *mm, >>> + enum memcg_memory_event event) >>> +{ >>> + struct mem_cgroup *memcg; >>> + >>> + if (mem_cgroup_disabled()) >>> + return; >>> + >>> + rcu_read_lock(); >>> + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); >>> + if (likely(memcg)) >>> + memcg_memory_event(memcg, event); >>> + rcu_read_unlock(); >>> +} >> >> I cannot find memcg_memory_event() in bionic/linux. Does this compile? >> >> Best regards, >> Krzysztof > > It is introduced by patch 2 of this series. Ah yes, enum confused me. All good now: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index c7876eadd206d..b5cd86e320ff3 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -53,6 +53,7 @@ enum memcg_memory_event { MEMCG_HIGH, MEMCG_MAX, MEMCG_OOM, + MEMCG_OOM_KILL, MEMCG_NR_MEMORY_EVENTS, }; @@ -706,11 +707,8 @@ static inline void count_memcg_event_mm(struct mm_struct *mm, rcu_read_lock(); memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); - if (likely(memcg)) { + if (likely(memcg)) count_memcg_events(memcg, idx, 1); - if (idx == OOM_KILL) - cgroup_file_notify(&memcg->events_file); - } rcu_read_unlock(); } @@ -721,6 +719,21 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg, cgroup_file_notify(&memcg->events_file); } +static inline void memcg_memory_event_mm(struct mm_struct *mm, + enum memcg_memory_event event) +{ + struct mem_cgroup *memcg; + + if (mem_cgroup_disabled()) + return; + + rcu_read_lock(); + memcg = mem_cgroup_from_task(rcu_dereference(mm->owner)); + if (likely(memcg)) + memcg_memory_event(memcg, event); + rcu_read_unlock(); +} + #ifdef CONFIG_TRANSPARENT_HUGEPAGE void mem_cgroup_split_huge_fixup(struct page *head); #endif @@ -742,6 +755,11 @@ static inline void memcg_memory_event(struct mem_cgroup *memcg, { } +static inline void memcg_memory_event_mm(struct mm_struct *mm, + enum memcg_memory_event event) +{ +} + static inline bool mem_cgroup_low(struct mem_cgroup *root, struct mem_cgroup *memcg) { diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 1161dd0ba3ff4..d9ce3263d97fb 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3666,7 +3666,8 @@ static int mem_cgroup_oom_control_read(struct seq_file *sf, void *v) seq_printf(sf, "oom_kill_disable %d\n", memcg->oom_kill_disable); seq_printf(sf, "under_oom %d\n", (bool)memcg->under_oom); - seq_printf(sf, "oom_kill %lu\n", memcg_sum_events(memcg, OOM_KILL)); + seq_printf(sf, "oom_kill %lu\n", + atomic_long_read(&memcg->memory_events[MEMCG_OOM_KILL])); return 0; } @@ -5338,7 +5339,8 @@ static int memory_events_show(struct seq_file *m, void *v) atomic_long_read(&memcg->memory_events[MEMCG_MAX])); seq_printf(m, "oom %lu\n", atomic_long_read(&memcg->memory_events[MEMCG_OOM])); - seq_printf(m, "oom_kill %lu\n", memcg_sum_events(memcg, OOM_KILL)); + seq_printf(m, "oom_kill %lu\n", + atomic_long_read(&memcg->memory_events[MEMCG_OOM_KILL])); return 0; } diff --git a/mm/oom_kill.c b/mm/oom_kill.c index 6999cf9c99bcb..187e385683cb9 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -927,7 +927,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message) /* Raise event before sending signal: task reaper must see this */ count_vm_event(OOM_KILL); - count_memcg_event_mm(mm, OOM_KILL); + memcg_memory_event_mm(mm, MEMCG_OOM_KILL); /* * We should send SIGKILL before granting access to memory reserves