Re: [PATCH] mm, memcg: fix wrong mem cgroup protection

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

On Fri, Apr 24, 2020 at 6:40 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Thu 23-04-20 14:13:19, Roman Gushchin wrote:
> > On Thu, Apr 23, 2020 at 04:33:23PM +0100, Chris Down wrote:
> > > Hi Yafang,
> > >
> > > I'm afraid I'm just as confused as Michal was about the intent of this patch.
> > >
> > > Can you please be more concise and clear about the practical ramifications
> > > and demonstrate some pathological behaviour? I really can't visualise what's
> > > wrong here from your explanation, and if I can't understand it as the person
> > > who wrote this code, I am not surprised others are also confused :-)
> > >
> > > Or maybe Roman can try to explain, since he acked the previous patch? At
> > > least to me, the emin/elow behaviour seems fairly non-trivial to reason
> > > about right now.
> >
> > Hi Chris!
> >
> > So the thing is that emin/elow cached values are shared between global and
> > targeted (caused by memory.max) reclaim. It's racy by design, but in general
> > it should work ok, because in the end we'll reclaim or not approximately
> > the same amount of memory.
> >
> > In the case which Yafang described, the emin value calculated in the process
> > of the global reclaim leads to a slowdown of the targeted reclaim. It's not
> > a tragedy, but not perfect too. It seems that the proposed patch makes it better,
> > and as now I don't see any bad consequences.
>
> Do we have any means to quantify the effect?
>
> I do understand the racy nature of the effective protection values. We
> do update them in mem_cgroup_protected and that handles the
> reclaim_target == memcg case already. So why do we care later on in
> mem_cgroup_protection? And why don't we care about any other concurrent
> reclaimers which have a different reclaim memcg target? This just
> doesn't make any sense.
>

No, you missed the point.
The issue pointed by me isn't related with racy.  Roman also explained
that the racy is not the point.

> Either we do care about races because they are harmful and then we care
> for all possible case or we don't and this patch doesn't really any big
> value. Or I still miss the point.
>

The real point is memcg relcaim will get a wrong value from
mem_cgroup_protection() after memory.emin is set.
Suppose target memcg has memory.current is 2G and memory.min is 2G, in
memcg reclaim, the scan count will be
(SWAP_CLUSTER_MAX>>sc->priority), rather than (lruvec_size >>
sc->priority). That's a slowdown, and that's explained by Roman as
well.



-- 
Thanks
Yafang