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

[Johannes Weiner <hannes@cmpxchg.org>]

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?

Because now both mem_cgroup_protection() and mem_cgroup_protected()
have to know about the reclaim root semantics, instead of just the one
central place.

And the query function has to know additional rules about when the
emin/elow values are uptodate or it could silently be looking at stale
data, which isn't very robust.

"The effective protection values are uptodate after calling
mem_cgroup_protected() inside the reclaim cycle - UNLESS the group
you're looking at happens to be..."

It's much easier to make the rule: The values are uptodate after you
called mem_cgroup_protected().

Or mem_cgroup_calculate_protection(), if we go with that later.

> > 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!
> 
> > 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

It would be a pretty extreme case, but not impossible AFAICS, because
OOM is just a sampled state, not deterministic.

If memory.max is 64G and memory.low is 64G minus one page, this bug
could cause limit reclaim to look at no more than SWAP_CLUSTER_MAX
pages at priority 0. It's possible it wouldn't get through the full
64G worth of memory before giving up and declaring OOM.

Not that that would be a sensical configuration... My point is that
OOM is defined as "I've looked at X pages and found nothing" and this
bug can significantly lower X.