Re: [PATCH 3/3] mm: improvements on memcg protection functions

[Michal Hocko <mhocko@kernel.org>]

On Mon 27-04-20 19:06:52, Yafang Shao wrote:
> On Mon, Apr 27, 2020 at 6:50 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Mon 27-04-20 18:09:27, Yafang Shao wrote:
> > > On Mon, Apr 27, 2020 at 5:40 PM Michal Hocko <mhocko@kernel.org> 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.
> 
> No, it still has side effect. That is not an immediate advantage neither.
> See bellow,

I believe you have misunderstood the proposal.
mem_cgroup_below_{min,low,protection} won't have any side effect on the
memcg. It would be only mem_cgroup_calculate_protection which updates
the cached state. So there won't be any predicate to have 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.
> >
> 
> When you calculate the effective values,  the source values to
> calculate it  may be changed with these values when you do the
> predicate.
> IOW, this proposal greatly increase the race window.

Why do you think so?

> > > > 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.
> 
> The race between  parallel reclaimers is fundamentally exist, and we
> can do now is reducing the race window as far as possible.

Reducing the race window is futile. The situation changes after each
reclaim attempt. All we need is to keep a protection consistent
regardless of where the reclaim root. That means that hierarchies deeper
in the tree cannot override the protection from those which are higher.

> > All intermediate nodes really do not care about the cached values
> > because they do not have any pages on the LRU lists.
> >
> 
> But read these cached values can save lot of time against the existing
> code, as the existing code still calculates them even if they are
> useless.

They are not useless. Intermediate values are necessary for the
protection distribution for lower level memcgs.

-- 
Michal Hocko
SUSE Labs