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

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

On Sat, Apr 25, 2020 at 12:22 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Fri, Apr 24, 2020 at 04:29:58PM +0200, Michal Hocko wrote:
> > On Fri 24-04-20 09:14:50, Johannes Weiner 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.
> > >
> > > Now that I understand the problem, I much prefer the previous version.
> > >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 745697906ce3..2bf91ae1e640 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -6332,8 +6332,19 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root,
> > >
> > >     if (!root)
> > >             root = root_mem_cgroup;
> > > -   if (memcg == root)
> > > +   if (memcg == root) {
> > > +           /*
> > > +            * The cgroup is the reclaim root in this reclaim
> > > +            * cycle, and therefore not protected. But it may have
> > > +            * stale effective protection values from previous
> > > +            * cycles in which it was not the reclaim root - for
> > > +            * example, global reclaim followed by limit reclaim.
> > > +            * Reset these values for mem_cgroup_protection().
> > > +            */
> > > +           memcg->memory.emin = 0;
> > > +           memcg->memory.elow = 0;
> > >             return MEMCG_PROT_NONE;
> > > +   }
> >
> > Could you be more specific why you prefer this over the
> > mem_cgroup_protection which doesn't change the effective value?
> > Isn't it easier to simply ignore effective value for the reclaim roots?
>
> Hm, I think I like the new version better, because it feels "safer" in terms
> of preserving sane effective protection values for concurrent reclaimers.
>
> >
> > [...]
> >
> > > As others have noted, it's fairly hard to understand the problem from
> > > the above changelog. How about the following:
> > >
> > > A cgroup can have both memory protection and a memory limit to isolate
> > > it from its siblings in both directions - for example, to prevent it
> > > from being shrunk below 2G under high pressure from outside, but also
> > > from growing beyond 4G under low pressure.
> > >
> > > 9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim")
> > > implemented proportional scan pressure so that multiple siblings in
> > > excess of their protection settings don't get reclaimed equally but
> > > instead in accordance to their unprotected portion.
> > >
> > > During limit reclaim, this proportionality shouldn't apply of course:
> > > there is no competition, all pressure is from within the cgroup and
> > > should be applied as such. Reclaim should operate at full efficiency.
> > >
> > > However, mem_cgroup_protected() never expected anybody to look at the
> > > effective protection values when it indicated that the cgroup is above
> > > its protection. As a result, a query during limit reclaim may return
> > > stale protection values that were calculated by a previous reclaim
> > > cycle in which the cgroup did have siblings.
> >
> > This is better. Thanks!
>
> +1
>
> and I like the proposed renaming/cleanup. Thanks, Johannes!
>
> >
> > > When this happens, reclaim is unnecessarily hesitant and potentially
> > > slow to meet the desired limit. In theory this could lead to premature
> > > OOM kills, although it's not obvious this has occurred in practice.
> >
> > I do not see how this would lead all the way to OOM killer but it
> > certainly can lead to unnecessary increase of the reclaim priority. The
> > smaller the difference between the reclaim target and protection the
> > more visible the effect would be. But if there are reclaimable pages
> > then the reclaim should see them sooner or later
>
> I guess if all memory is protected by emin and the targeted reclaim
> will be unable to reclaim anything, OOM can be triggered.
>
> Btw, I wonder if this case can be covered by a new memcg kselftest?
> I'm not sure it can be easily reproducible, but if it can, it would be
> the best demonstration of a problem and the fix.
> Yafang, don't you want to try?

I have tried to produce the premature OOM before I send this fix, but
I find that it is really not easy to produce.
But if a new memcg kselftest is needed, I can try it again.

-- 
Thanks
Yafang