| Message ID | 1577174006-13025-3-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:23AM -0500, Yafang Shao wrote: > If the usage of a memcg is zero, we don't need to do useless work to scan > it. That is a minor optimization. The optimization isn't really related to the main idea of the patchset, so I'd prefer to treat it separately. > > Cc: Roman Gushchin <guro@fb.com> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > include/linux/memcontrol.h | 1 + > mm/memcontrol.c | 2 +- > mm/vmscan.c | 6 ++++++ > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 612a457..1a315c7 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -54,6 +54,7 @@ enum mem_cgroup_protection { > MEMCG_PROT_NONE, > MEMCG_PROT_LOW, > MEMCG_PROT_MIN, > + MEMCG_PROT_SKIP, /* For zero usage case */ > }; > > struct mem_cgroup_reclaim_cookie { > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index c5b5f74..f35fcca 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -6292,7 +6292,7 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, > > usage = page_counter_read(&memcg->memory); > if (!usage) > - return MEMCG_PROT_NONE; > + return MEMCG_PROT_SKIP; I'm concerned that it might lead to a regression with the scraping of last pages from a memcg. Charge is batched using percpu stocks, so the value of the page counter is approximate. Skipping the cgroup entirely we're losing all chances to reclaim these few pages. Idk how serious the problem could be in the real life, and maybe it's OK to skip if the cgroup is online, but I'd triple check here. Also, because this optimization isn't really related to protection, why not check the page counter first, e.g.: memcg = mem_cgroup_iter(root, NULL, NULL); do { unsigned long reclaimed; unsigned long scanned; if (!page_counter_read(&memcg->memory)) continue; switch (mem_cgroup_protected(root, memcg)) { case MEMCG_PROT_MIN: /* * Hard protection. * If there is no reclaimable memory, OOM. */ continue; case MEMCG_PROT_LOW: -- Thank you!
On Fri, Dec 27, 2019 at 5:36 AM Roman Gushchin <guro@fb.com> wrote: > > On Tue, Dec 24, 2019 at 02:53:23AM -0500, Yafang Shao wrote: > > If the usage of a memcg is zero, we don't need to do useless work to scan > > it. That is a minor optimization. > > The optimization isn't really related to the main idea of the patchset, > so I'd prefer to treat it separately. > Sure. > > > > Cc: Roman Gushchin <guro@fb.com> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > include/linux/memcontrol.h | 1 + > > mm/memcontrol.c | 2 +- > > mm/vmscan.c | 6 ++++++ > > 3 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > > index 612a457..1a315c7 100644 > > --- a/include/linux/memcontrol.h > > +++ b/include/linux/memcontrol.h > > @@ -54,6 +54,7 @@ enum mem_cgroup_protection { > > MEMCG_PROT_NONE, > > MEMCG_PROT_LOW, > > MEMCG_PROT_MIN, > > + MEMCG_PROT_SKIP, /* For zero usage case */ > > }; > > > > struct mem_cgroup_reclaim_cookie { > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index c5b5f74..f35fcca 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -6292,7 +6292,7 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, > > > > usage = page_counter_read(&memcg->memory); > > if (!usage) > > - return MEMCG_PROT_NONE; > > + return MEMCG_PROT_SKIP; > > I'm concerned that it might lead to a regression with the scraping of > last pages from a memcg. Charge is batched using percpu stocks, so the > value of the page counter is approximate. Skipping the cgroup entirely > we're losing all chances to reclaim these few pages. > Agree with you. It may lose the chances to reclaim these last few pages. I will think about it. > Idk how serious the problem could be in the real life, and maybe it's OK > to skip if the cgroup is online, but I'd triple check here. > > Also, because this optimization isn't really related to protection, > why not check the page counter first, e.g.: > > memcg = mem_cgroup_iter(root, NULL, NULL); > do { > unsigned long reclaimed; > unsigned long scanned; > > if (!page_counter_read(&memcg->memory)) > continue; > Seems better. Thanks for your suggestion. > switch (mem_cgroup_protected(root, memcg)) { > case MEMCG_PROT_MIN: > /* > * Hard protection. > * If there is no reclaimable memory, OOM. > */ > continue; > case MEMCG_PROT_LOW: > > -- > > Thank you!
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 612a457..1a315c7 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -54,6 +54,7 @@ enum mem_cgroup_protection { MEMCG_PROT_NONE, MEMCG_PROT_LOW, MEMCG_PROT_MIN, + MEMCG_PROT_SKIP, /* For zero usage case */ }; struct mem_cgroup_reclaim_cookie { diff --git a/mm/memcontrol.c b/mm/memcontrol.c index c5b5f74..f35fcca 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6292,7 +6292,7 @@ enum mem_cgroup_protection mem_cgroup_protected(struct mem_cgroup *root, usage = page_counter_read(&memcg->memory); if (!usage) - return MEMCG_PROT_NONE; + return MEMCG_PROT_SKIP; emin = memcg->memory.min; elow = memcg->memory.low; diff --git a/mm/vmscan.c b/mm/vmscan.c index 5a6445e..3c4c2da 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -2677,6 +2677,12 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc) * thresholds (see get_scan_count). */ break; + case MEMCG_PROT_SKIP: + /* + * Skip scanning this memcg if the usage of it is + * zero. + */ + continue; } reclaimed = sc->nr_reclaimed;
If the usage of a memcg is zero, we don't need to do useless work to scan it. That is a minor optimization. Cc: Roman Gushchin <guro@fb.com> Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- include/linux/memcontrol.h | 1 + mm/memcontrol.c | 2 +- mm/vmscan.c | 6 ++++++ 3 files changed, 8 insertions(+), 1 deletion(-)