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

[Michal Hocko <mhocko@kernel.org>]

On Fri 24-04-20 11:10:13, Johannes Weiner 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?
> 
> 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.

Yes this is true but it is also potentially overwriting the state with
a parallel reclaim which can lead to surprising results beacause
parent's effective protection is used to define protection distribution
for children. Let's have global and A's reclaim in parallel:
 |
 A (low=2G, usage = 3G, max = 3G, children_low_usage = 1.5G)
 |\
 | C (low = 1G, usage = 2.5G)
 B (low = 1G, usage = 0.5G)

for A reclaim we have
B.elow = B.low
C.elow = C.low

For the global reclaim
A.elow = A.low
B.elow = min(B.usage, B.low) because children_low_usage <= A.elow
C.elow = min(C.usage, C.low)

With the effective values reseting we have A reclaim
A.elow = 0
B.elow = B.low
C.elow = C.low
[...]

and global reclaim could see the above and then
B.elow = C.elow = 0 because children_low_usage > A.elow

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

Yes, my bad I didn't really realize that there won't be a full scan even
under priority 0.
-- 
Michal Hocko
SUSE Labs