On Mon, Feb 17, 2020 at 9:24 PM Michal Hocko <mhocko@xxxxxxxxxx> wrote:
>
> On Mon 17-02-20 21:08:12, Yafang Shao wrote:
> > On Mon, Feb 17, 2020 at 5:25 PM Michal Hocko <mhocko@xxxxxxxxxx> 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)
|
exit
I try my best to illustrate it. Hope it could clarify.
> > > What happens if we have concurrent
> > > reclaimers at different levels of the hierarchy how that would affect
> > > the resulting protection?
> > >
> >
> > Well, I thought the synchronization mechanisms have already existed ?
> > Otherwise there must be concurrent issue in the original code of
> > setting the memcg.{emin, elow} as well.
> > (Because memcg->memory.{emin, elow} are also set at the end of the
> > function mem_cgroup_protected())
>
> This function is documented to be racy and I believe this is OK because
> it doesn't really have to be precise and concurrent updates are not
> going to change values much. But does the same apply to reseting the
> effective values? Maybe yes. Make sure to document this in the changelog
> please.
Sure. I will document it.
--
Yafang Shao
DiDi
