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

[Roman Gushchin <guro@fb.com>]

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?

Thanks!