Re: [PATCH v2 2/5] mm, memcg: introduce MEMCG_PROT_SKIP for memcg zero usage case

[Yafang Shao <laoar.shao@gmail.com>]

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!