| Message ID | 1576582159-5198-4-git-send-email-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memcg, inode: protect page cache from freeing inode | expand |
Hi Yafang, Yafang Shao writes: >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. If the memcg in question is passed as "root" to mem_cgroup_protected with a child as the new "memcg" argument, then I still don't see what is wrong. mem_cgroup_protected must be called top-down from the root of the hierarchy in order to work already, which we already do in shrink_node_memcgs. This will already update the tree's cached effective protections properly, as far as I can see. As such I'm not sure I understand what you mean in the changelog or in the patch. emin/elow as a mechanism is already intended to be racy/best-effort, since by the time we get to doing work it's always possible that reclaim eligibility state changed, and callers have to consider that. Could you please explain further the situation you're trying to guard against? 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..234370c 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.emin = 0; >+ } > return MEMCG_PROT_NONE; >+ } > > usage = page_counter_read(&memcg->memory); > if (!usage) >-- >1.8.3.1 >
On Tue, Dec 17, 2019 at 10:20 PM Chris Down <chris@chrisdown.name> wrote: > > Hi Yafang, > > Yafang Shao writes: > >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. > > If the memcg in question is passed as "root" to mem_cgroup_protected with a > child as the new "memcg" argument, then I still don't see what is wrong. > mem_cgroup_protected must be called top-down from the root of the hierarchy in > order to work already, which we already do in shrink_node_memcgs. This will > already update the tree's cached effective protections properly, as far as I > can see. > Right. > As such I'm not sure I understand what you mean in the changelog or in the > patch. emin/elow as a mechanism is already intended to be racy/best-effort, > since by the time we get to doing work it's always possible that reclaim > eligibility state changed, and callers have to consider that. > > Could you please explain further the situation you're trying to guard against? > Thanks. > Considering bellow case, 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 to 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 vaule 512M, and then this old value will be used to in mem_cgroup_protection() in get_scan_count() to get the scan count. That is not proper. Right ? Thanks Yafang > >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..234370c 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.emin = 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..234370c 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.emin = 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. 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(-)