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=800MOnce 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)).
Btw. have you seen the latest patch from Johannes touching this area [1]? Is it possible that the issue you are referring to is related with the one he has fixed?
[1] http://lkml.kernel.org/r/20191219200718.15696-2-hannes@cmpxchg.org
I haven't taken a look at it yet.