On Fri, Jun 21, 2024 at 12:15 AM Chengming Zhou
<chengming.zhou@xxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Chengming Zhou <chengming.zhou@xxxxxxxxx>
> ---
> 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.
