Re: [PATCH] mm, memcg: fix wrong mem cgroup protection

[Yafang Shao <laoar.shao@gmail.com>]

On Fri, Apr 24, 2020 at 5:06 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Thu, Apr 23, 2020 at 02:16:29AM -0400, Yafang Shao wrote:
> > This patch is an improvement of a previous version[1], as the previous
> > version is not easy to understand.
> > This issue persists in the newest kernel, I have to resend the fix. As
> > the implementation is changed, I drop Roman's ack from the previous
> > version.
> >
> > Here's the explanation of this issue.
> > memory.{low,min} won't take effect if the to-be-reclaimed memcg is the
> > sc->target_mem_cgroup, that can also be proved by the implementation in
> > mem_cgroup_protected(), see bellow,
> >       mem_cgroup_protected
> >               if (memcg == root) [2]
> >                       return MEMCG_PROT_NONE;
> >
> > But this rule is ignored in mem_cgroup_protection(), which will read
> > memory.{emin, elow} as the protection whatever the memcg is.
> >
> > How would this issue happen?
> > Because in mem_cgroup_protected() we forget to clear the
> > memory.{emin, elow} if the memcg is target_mem_cgroup [2].
> >
> > An example to illustrate this issue.
> >    root_mem_cgroup
> >          /
> >         A   memory.max: 1024M
> >             memory.min: 512M
> >             memory.current: 800M ('current' must be greater than 'min')
> > Once kswapd starts to reclaim memcg A, it assigns 512M to memory.emin of A.
> > Then kswapd stops.
> > As a result of it, the memory values of A will be,
> >    root_mem_cgroup
> >          /
> >         A   memory.max: 1024M
> >             memory.min: 512M
> >             memory.current: 512M (approximately)
> >             memory.emin: 512M
> >
> > Then a new workload starts to run in memcg A, and it will trigger memcg
> > relcaim in A soon. As memcg A is the target_mem_cgroup of this
> > reclaimer, so it return directly without touching memory.{emin, elow}.[2]
> > The memory values of A will be,
> >    root_mem_cgroup
> >          /
> >         A   memory.max: 1024M
> >             memory.min: 512M
> >             memory.current: 1024M (approximately)
> >             memory.emin: 512M
> > Then this memory.emin will be used in mem_cgroup_protection() to get the
> > scan count, which is obvoiusly a wrong scan count.
> >
> > [1]. https://lore.kernel.org/linux-mm/20200216145249.6900-1-laoar.shao@gmail.com/
> >
> > Fixes: 9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim")
> > Cc: Chris Down <chris@chrisdown.name>
> > Cc: Roman Gushchin <guro@fb.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  include/linux/memcontrol.h | 13 +++++++++++--
> >  mm/vmscan.c                |  4 ++--
> >  2 files changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index d275c72c4f8e..114cfe06bf60 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -344,12 +344,20 @@ static inline bool mem_cgroup_disabled(void)
> >       return !cgroup_subsys_enabled(memory_cgrp_subsys);
> >  }
> >
> > -static inline unsigned long mem_cgroup_protection(struct mem_cgroup *memcg,
> > +static inline unsigned long mem_cgroup_protection(struct mem_cgroup *root,
> > +                                               struct mem_cgroup *memcg,
> >                                                 bool in_low_reclaim)
>
> I'd rename "root" to "target", maybe it will make the whole thing more clear.
>

That would make it better.  I will change it.

> I'll think a bit more about it, but at the first glance the patch looks sane to me.
>
> Thanks!



-- 
Thanks
Yafang