On Fri, Apr 24, 2020 at 6:40 PM Michal Hocko <mhocko@xxxxxxxxxx> 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
