On Mon, Apr 27, 2020 at 5:40 PM Michal Hocko <mhocko@xxxxxxxxxx> wrote:
>
> 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@
>
Thanks for pointing this out.
> > 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.
>
Sorry, I failed to see what the advantage of Johannes's proposal
except the better naming.
> Your changelog doesn't explain why double caching the effective value is
> an improvement.
The improvement is, to avoid getting an wrong value calculated by
other reclaimers and avoid issues in mem_cgroup_protection() that we
haven't noticed.
> 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
>
As I said above, I failed to see the advantage of this proposal.
Maybe we can wait until Johannes can show his code.
Hi Johannes,
Would you pls. show a simple implementation on your proposal ASAP ?
--
Thanks
Yafang
