On Mon 27-04-20 18:09:27, Yafang Shao wrote:
> 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.
The immediate advantage is that predicate should better not have any
side effect. So splitting into the calculation part which has clearly
defined side effects and having a simple predicate that consults
pre-calculated values makes a lot of sense to me.
> > 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.
This is not true in general. There is still parallel calculation done
and so parallel reclaimers might affect each other. Your patch only
makes a real difference for leaf memcgs which are the reclaim target as
well. All intermediate nodes really do not care about the cached values
because they do not have any pages on the LRU lists.
--
Michal Hocko
SUSE Labs
