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

[Chengming Zhou <chengming.zhou@linux.dev>]

On 2024/6/24 20:14, Yosry Ahmed wrote:
> 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.

Oops, my bad, will fix in the next version.

Thanks!