On Tue, Feb 18, 2020 at 4:59 PM Michal Hocko mhocko@kernel.org wrote:
On Tue 18-02-20 10:09:06, Yafang Shao wrote:
On Mon, Feb 17, 2020 at 11:14 PM Michal Hocko mhocko@kernel.org wrote:
On Mon 17-02-20 22:40:22, Yafang Shao wrote:
On Mon, Feb 17, 2020 at 10:35 PM Michal Hocko mhocko@kernel.org wrote:
On Mon 17-02-20 22:28:38, Yafang Shao wrote:
On Mon, Feb 17, 2020 at 10:04 PM Michal Hocko mhocko@kernel.org wrote: > > On Mon 17-02-20 21:51:23, Yafang Shao wrote: > > On Mon, Feb 17, 2020 at 9:24 PM Michal Hocko mhocko@kernel.org wrote: > > > > > > On Mon 17-02-20 21:08:12, Yafang Shao wrote: > > > > On Mon, Feb 17, 2020 at 5:25 PM Michal Hocko mhocko@kernel.org wrote: > > > > > > > > > > On Sun 16-02-20 09:52:49, Yafang Shao wrote: > > > > > > memory.{emin, elow} are set in mem_cgroup_protected(), and the values of > > > > > > them won't be changed until next recalculation in this function. After > > > > > > either or both of them are set, the next reclaimer to relcaim this memcg > > > > > > may be a different reclaimer, e.g. this memcg is also the root memcg of > > > > > > the new reclaimer, and then in mem_cgroup_protection() in get_scan_count() > > > > > > the old values of them will be used to calculate scan count, that is not > > > > > > proper. We should reset them to zero in this case. > > > > > > > > > > > > Here's an example of this issue. > > > > > > > > > > > > root_mem_cgroup > > > > > > / > > > > > > A memory.max=1024M memory.min=512M memory.current=800M > > > > > > > > > > > > Once kswapd is waked up, it will try to scan all MEMCGs, including > > > > > > this A, and it will assign memory.emin of A with 512M. > > > > > > After that, A may reach its hard limit(memory.max), and then it will > > > > > > do memcg reclaim. Because A is the root of this reclaimer, so it will > > > > > > not calculate its memory.emin. So the memory.emin is the old value > > > > > > 512M, and then this old value will be used in > > > > > > mem_cgroup_protection() in get_scan_count() to get the scan count. > > > > > > That is not proper. > > > > > > > > > > Please document user visible effects of this patch. What does it mean > > > > > that this is not proper behavior? > > > > > > > > In the memcg reclaim, if the target memcg is the root of the reclaimer, > > > > the reclaimer should scan this memcg's all page cache pages in the LRU, > > > > but now as the old memcg.{emin, elow} value are still there, it will get > > > > a wrong protection value, > > > > and the reclaimer can't reclaim the page cache pages protected by this > > > > wrong protection. > > > > > > Could you be more specific please. Your example above says that emin is > > > not going to be recalculated and stays at 512M even for a potential max > > > limit reclaim. The min limit is still 512M so why is this value wrong? > > > > > > > Because the relcaimers are changed or the root the relcaimer is changed. > > > > Kswapd begins to relcaim memcg-A. > > kswapd > > | > > calculate the {emin, elow} for memcg-A > > | > > stores {emin, elow} in memory.{emin, elow} of memcg-A > > | > > This memory.{emin, elow} will protect the page cache pages in memcg-A > > (See get_scan_count->mem_cgroup_protection) > > | > > exit > > (And it won't relcaim memcg-A for a long time) > > > > > > Then memcg relcaimer is woke up (reached the hard limit of memcg-A), > > and the root of this new reclaimer is memcg-A. > > > > This memcg relcaimer begins to reclaim memcg-A. > > memcg relcaimer > > | > > As the root of the relcaimer is memcg-A, it won't calculate emin, elow > > for memcg-A. > > (See if (memcg == root) in mem_cgroup_protected()) > > | > > The old memory.{emin, elow} will protect the page cache pages in memcg-A > > (SO WE SHOULD CLEAR THE OLD VALUE) > > I am sorry but I still do not follow. Could you focus on _why_ the old > value is no longer valid?
Because for the new reclaimer the memory.{emin, elow} should be 0. The old value may be not 0, but it was thought as 0 in the if statement (if (memcg == root)).
Why should it be 0 when the A.min is still 512MB?
Because A's hard limit is reached and A is the root of memcg relcaimer.
Confused. But your examples suggests that memory.max > memory.min so having an effective emin 0 or not doesn't make any difference.
Why is it having an effective emin 0 if memory.max > memory.min ? Note that effective emin is only set in function mem_cgroup_protected(), so if we don't set it explicitly to 0 then it can't be 0.
Besides mem_cgroup_protected(), the effective emin also take effect in the function mem_cgroup_protection(), but in this function it only use the existed memory.emin rather than verifying memory.max > memory.min.
So the real issue is in mem_cgroup_protection(), because the value it is using may be an old value.
I am sorry but I still do not follow. You keep focusing on talking about the code while I am really interested in the user visible semantic that you want to achieve. I am sorry to be dense here but believe me I am trying.
Sorry about my poor English that hasn't described it clearly.
Your example doesn't help much because the effective protection doesn't play any role in the limit reclaim there AFAICS. I would even argue that emin == min is the proper thing in your example.
So I can only recommend you to rethink your usecase and try to describe it in a higher level way.
Yes, I will try to improve the example and make it more clear.