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

From: Michal Hocko <mhocko@kernel.org>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Yafang Shao <laoar.shao@gmail.com>,
	akpm@linux-foundation.org, vdavydov.dev@gmail.com,
	linux-mm@kvack.org, Chris Down <chris@chrisdown.name>,
	Roman Gushchin <guro@fb.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] mm, memcg: fix wrong mem cgroup protection
Date: Fri, 24 Apr 2020 16:33:46 +0200	[thread overview]
Message-ID: <20200424143346.GG11591@dhcp22.suse.cz> (raw)
In-Reply-To: <20200424134438.GA496852@cmpxchg.org>

On Fri 24-04-20 09:44:38, Johannes Weiner wrote:
> On Fri, Apr 24, 2020 at 09:14:52AM -0400, Johannes Weiner wrote:
> > 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.
> 
> Btw, I think there is opportunity to make this a bit less error prone.
> 
> We have a mem_cgroup_protected() that returns yes or no, essentially,
> but protection isn't a binary state anymore.
> 
> It's also been a bit iffy that it looks like a simple predicate
> function, but it indeed needs to run procedurally for each cgroup in
> order for the calculations throughout the tree to be correct.
> 
> It might be better to have a
> 
> 	mem_cgroup_calculate_protection()
> 
> that runs for every cgroup we visit and sets up the internal state;
> then have more self-explanatory query functions on top of that:
> 
> 	mem_cgroup_below_min()
> 	mem_cgroup_below_low()
> 	mem_cgroup_protection()
> 
> What do you guys think?

Fully agreed. I have to say that I have considered the predicate with
side effects really confusing.
-- 
Michal Hocko
SUSE Labs