Re: [v2 PATCH 7/9] mm: vmscan: don't need allocate shrinker->nr_deferred for memcg aware shrinkers

[Yang Shi]

On Mon, Dec 14, 2020 at 7:05 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote:
>
> On Mon, Dec 14, 2020 at 02:37:20PM -0800, Yang Shi wrote:
> > Now nr_deferred is available on per memcg level for memcg aware shrinkers, so don't need
> > allocate shrinker->nr_deferred for such shrinkers anymore.
> >
> > Signed-off-by: Yang Shi <shy828301@xxxxxxxxx>
> > ---
> >  mm/vmscan.c | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index bce8cf44eca2..8d5bfd818acd 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -420,7 +420,15 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone
> >   */
> >  int prealloc_shrinker(struct shrinker *shrinker)
> >  {
> > -     unsigned int size = sizeof(*shrinker->nr_deferred);
> > +     unsigned int size;
> > +
> > +     if (is_deferred_memcg_aware(shrinker)) {
> > +             if (prealloc_memcg_shrinker(shrinker))
> > +                     return -ENOMEM;
> > +             return 0;
> > +     }
> > +
> > +     size = sizeof(*shrinker->nr_deferred);
> >
> >       if (shrinker->flags & SHRINKER_NUMA_AWARE)
> >               size *= nr_node_ids;
> > @@ -429,26 +437,18 @@ int prealloc_shrinker(struct shrinker *shrinker)
> >       if (!shrinker->nr_deferred)
> >               return -ENOMEM;
> >
> > -     if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
> > -             if (prealloc_memcg_shrinker(shrinker))
> > -                     goto free_deferred;
> > -     }
> > -
> >       return 0;
> > -
> > -free_deferred:
> > -     kfree(shrinker->nr_deferred);
> > -     shrinker->nr_deferred = NULL;
> > -     return -ENOMEM;
> >  }
>
> I'm trying to put my finger on it, but this seems wrong to me. If
> memcgs are disabled, then prealloc_memcg_shrinker() needs to fail.
> The preallocation code should not care about internal memcg details
> like this.
>
>         /*
>          * If the shrinker is memcg aware and memcgs are not
>          * enabled, clear the MEMCG flag and fall back to non-memcg
>          * behaviour for the shrinker.
>          */
>         if (shrinker->flags & SHRINKER_MEMCG_AWARE) {
>                 error = prealloc_memcg_shrinker(shrinker);
>                 if (!error)
>                         return 0;
>                 if (error != -ENOSYS)
>                         return error;
>
>                 /* memcgs not enabled! */
>                 shrinker->flags &= ~SHRINKER_MEMCG_AWARE;
>         }
>
>         size = sizeof(*shrinker->nr_deferred);
>         ....
>         return 0;
> }
>
> This guarantees that only the shrinker instances taht have a
> correctly set up memcg attached to them will have the
> SHRINKER_MEMCG_AWARE flag set. Hence in all the rest of the shrinker
> code, we only ever need to check for SHRINKER_MEMCG_AWARE to
> determine what we should do....

Thanks. I see your point. We could move the memcg specific details
into prealloc_memcg_shrinker().

It seems we have to acquire shrinker_rwsem before we check and modify
SHIRNKER_MEMCG_AWARE bit if we may clear it.

>
> >  void free_prealloced_shrinker(struct shrinker *shrinker)
> >  {
> > -     if (!shrinker->nr_deferred)
> > +     if (is_deferred_memcg_aware(shrinker)) {
> > +             unregister_memcg_shrinker(shrinker);
> >               return;
> > +     }
> >
> > -     if (shrinker->flags & SHRINKER_MEMCG_AWARE)
> > -             unregister_memcg_shrinker(shrinker);
> > +     if (!shrinker->nr_deferred)
> > +             return;
> >
> >       kfree(shrinker->nr_deferred);
> >       shrinker->nr_deferred = NULL;
>
> e.g. then this function can simply do:
>
> {
>         if (shrinker->flags & SHRINKER_MEMCG_AWARE)
>                 return unregister_memcg_shrinker(shrinker);
>         kfree(shrinker->nr_deferred);
>         shrinker->nr_deferred = NULL;
> }
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx