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

[Dave Chinner]

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 <shy828...@gmail.com>
> ---
>  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....

>  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
da...@fromorbit.com