On Sat 25-04-20 11:24:18, Yafang Shao wrote:
> Since proportional memory.{min, low} reclaim is introduced in
> commit 9783aa9917f8 ("mm, memcg: proportional memory.{low,min} reclaim"),
> it have been proved that the proportional reclaim is hard to understand and
> the issues caused by it is harder to understand.[1]. That dilemma faced by
> us is caused by that the proportional reclaim mixed up memcg and the
> reclaim context.
>
> In proportional reclaim, the whole reclaim context - includes the memcg
> to be reclaimed and the reclaimer, should be considered, rather than
> memcg only.
>
> To make it clear, a new member 'protection' is introduced in the reclaim
> context (struct shrink_control) to replace mem_cgroup_protection(). This
s@shrink_control@scan_control@
> one is set when we check whether the memcg is protected or not.
>
> After this change, the issue pointed by me[1] - a really old left-over
> value can slow donw target reclaim - can be fixed, and I think it could
> also avoid some potential race.
The patch would have been much esier to review if you only focused on
the effective protection value caching. I really fail to see why you had
to make mem_cgroup_protected even more convoluted with more side effects
(e.g. sc->memcg_low_skipped). This goes directly opposite to what
Johannes was proposing in other email AFAICS.
Your changelog doesn't explain why double caching the effective value is
an improvement. I believe your goal was to drop the special casing for
the reclaim targets which are the only to ignore protection as they are
clearly violating the consumption constraints. This makes some sense
to me because it makes the special case have a local effect.
But I really dislike your patch. Please follow up on Johannes'
suggestion to split up the mem_cgroup_protected into parts
http://lkml.kernel.org/r/20200424134438.GA496852@xxxxxxxxxxx
--
Michal Hocko
SUSE Labs
