| Message ID | 1577174006-13025-4-git-send-email-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | protect page cache from freeing inode | expand |
On Tue, Dec 24, 2019 at 02:53:24AM -0500, 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. Good catch! But it seems to be a bug introduced with the implementation of the proportional reclaim. So I'd remove it from the patchset, add the "Fixes" tag and cc stable@. Then it will have chances to be backported to stable trees. Thank you! > > Cc: Chris Down <chris@chrisdown.name> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > mm/memcontrol.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index f35fcca..2e78931 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -6287,8 +6287,17 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, > > if (!root) > root = root_mem_cgroup; > - if (memcg == root) > + if (memcg == root) { > + /* > + * Reset memory.(emin, elow) for reclaiming the memcg > + * itself. > + */ > + if (memcg != root_mem_cgroup) { > + memcg->memory.emin = 0; > + memcg->memory.elow = 0; > + } > return MEMCG_PROT_NONE; > + } > > usage = page_counter_read(&memcg->memory); > if (!usage) > -- > 1.8.3.1 > >
On Fri, Dec 27, 2019 at 5:46 AM Roman Gushchin <guro@fb.com> wrote: > > On Tue, Dec 24, 2019 at 02:53:24AM -0500, 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. > > Good catch! > > But it seems to be a bug introduced with the implementation of the proportional > reclaim. So I'd remove it from the patchset, add the "Fixes" tag and cc stable@. > Then it will have chances to be backported to stable trees. > Sure, will do it. Thanks! > > > > > Cc: Chris Down <chris@chrisdown.name> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > mm/memcontrol.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index f35fcca..2e78931 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -6287,8 +6287,17 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, > > > > if (!root) > > root = root_mem_cgroup; > > - if (memcg == root) > > + if (memcg == root) { > > + /* > > + * Reset memory.(emin, elow) for reclaiming the memcg > > + * itself. > > + */ > > + if (memcg != root_mem_cgroup) { > > + memcg->memory.emin = 0; > > + memcg->memory.elow = 0; > > + } > > return MEMCG_PROT_NONE; > > + } > > > > usage = page_counter_read(&memcg->memory); > > if (!usage) > > -- > > 1.8.3.1 > > > >
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index f35fcca..2e78931 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6287,8 +6287,17 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, if (!root) root = root_mem_cgroup; - if (memcg == root) + if (memcg == root) { + /* + * Reset memory.(emin, elow) for reclaiming the memcg + * itself. + */ + if (memcg != root_mem_cgroup) { + memcg->memory.emin = 0; + memcg->memory.elow = 0; + } return MEMCG_PROT_NONE; + } usage = page_counter_read(&memcg->memory); if (!usage)
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. Cc: Chris Down <chris@chrisdown.name> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- mm/memcontrol.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)