Re: [PATCH v2 2/2] mm/zswap: use only one pool in zswap

[Yosry Ahmed <yosryahmed@google.com>]

On Fri, Jun 21, 2024 at 12:15 AM Chengming Zhou
<chengming.zhou@linux.dev> wrote:
>
> Zswap uses 32 pools to workaround the locking scalability problem in
> zswap backends (mainly zsmalloc nowadays), which brings its own problems
> like memory waste and more memory fragmentation.
>
> Testing results show that we can have near performance with only one
> pool in zswap after changing zsmalloc to use per-size_class lock instead
> of pool spinlock.
>
> Testing kernel build (make bzImage -j32) on tmpfs with memory.max=1GB,
> and zswap shrinker enabled with 10GB swapfile on ext4.
>
>                                 real    user    sys
> 6.10.0-rc3                      138.18  1241.38 1452.73
> 6.10.0-rc3-onepool              149.45  1240.45 1844.69
> 6.10.0-rc3-onepool-perclass     138.23  1242.37 1469.71
>
> And do the same testing using zbud, which shows a little worse performance
> as expected since we don't do any locking optimization for zbud. I think
> it's acceptable since zsmalloc became a lot more popular than other
> backends, and we may want to support only zsmalloc in the future.
>
>                                 real    user    sys
> 6.10.0-rc3-zbud                 138.23  1239.58 1430.09
> 6.10.0-rc3-onepool-zbud         139.64  1241.37 1516.59
>
> Reviewed-by: Nhat Pham <nphamcs@gmail.com>
> Signed-off-by: Chengming Zhou <chengming.zhou@linux.dev>
> ---
>  mm/zswap.c | 60 +++++++++++++++++++-----------------------------------------
>  1 file changed, 19 insertions(+), 41 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index e25a6808c2ed..7925a3d0903e 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -122,9 +122,6 @@ static unsigned int zswap_accept_thr_percent = 90; /* of max pool size */
>  module_param_named(accept_threshold_percent, zswap_accept_thr_percent,
>                    uint, 0644);
>
> -/* Number of zpools in zswap_pool (empirically determined for scalability) */
> -#define ZSWAP_NR_ZPOOLS 32
> -
>  /* Enable/disable memory pressure-based shrinker. */
>  static bool zswap_shrinker_enabled = IS_ENABLED(
>                 CONFIG_ZSWAP_SHRINKER_DEFAULT_ON);
> @@ -160,7 +157,7 @@ struct crypto_acomp_ctx {
>   * needs to be verified that it's still valid in the tree.
>   */
>  struct zswap_pool {
> -       struct zpool *zpools[ZSWAP_NR_ZPOOLS];
> +       struct zpool *zpool;
>         struct crypto_acomp_ctx __percpu *acomp_ctx;
>         struct percpu_ref ref;
>         struct list_head list;
> @@ -237,7 +234,7 @@ static inline struct xarray *swap_zswap_tree(swp_entry_t swp)
>
>  #define zswap_pool_debug(msg, p)                               \
>         pr_debug("%s pool %s/%s\n", msg, (p)->tfm_name,         \
> -                zpool_get_type((p)->zpools[0]))
> +                zpool_get_type((p)->zpool))
>
>  /*********************************
>  * pool functions
> @@ -246,7 +243,6 @@ static void __zswap_pool_empty(struct percpu_ref *ref);
>
>  static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>  {
> -       int i;
>         struct zswap_pool *pool;
>         char name[38]; /* 'zswap' + 32 char (max) num + \0 */
>         gfp_t gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
> @@ -267,18 +263,14 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>         if (!pool)
>                 return NULL;
>
> -       for (i = 0; i < ZSWAP_NR_ZPOOLS; i++) {
> -               /* unique name for each pool specifically required by zsmalloc */
> -               snprintf(name, 38, "zswap%x",
> -                        atomic_inc_return(&zswap_pools_count));
> -
> -               pool->zpools[i] = zpool_create_pool(type, name, gfp);
> -               if (!pool->zpools[i]) {
> -                       pr_err("%s zpool not available\n", type);
> -                       goto error;
> -               }
> +       /* unique name for each pool specifically required by zsmalloc */
> +       snprintf(name, 38, "zswap%x", atomic_inc_return(&zswap_pools_count));
> +       pool->zpool = zpool_create_pool(type, name, gfp);
> +       if (!pool->zpool) {
> +               pr_err("%s zpool not available\n", type);
> +               return NULL;

We need to goto error here to free the pool.

>         }
> -       pr_debug("using %s zpool\n", zpool_get_type(pool->zpools[0]));
> +       pr_debug("using %s zpool\n", zpool_get_type(pool->zpool));
>
>         strscpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
>
> @@ -311,8 +303,7 @@ static struct zswap_pool *zswap_pool_create(char *type, char *compressor)
>  error:
>         if (pool->acomp_ctx)
>                 free_percpu(pool->acomp_ctx);
> -       while (i--)
> -               zpool_destroy_pool(pool->zpools[i]);
> +       zpool_destroy_pool(pool->zpool);

.. and then we will need a NULL check needed here.