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

[Yafang Shao <laoar.shao@gmail.com>]

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,

> 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.

> > > 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.

> 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.
If you think that is a issue, I think why not skip scanning these
intermediate nodes because they don't have any pages on the LRU lists
?

-- 
Thanks
Yafang