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

Dave Chinner
2020-12-15T03:05:36Z

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