On Fri, Dec 27, 2019 at 5:36 AM Roman Gushchin <guro@xxxxxx> 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@xxxxxx>
> > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > ---
> > 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!
