| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f52f007c-5418-4e01-99ed-f34892059a7b@linux.dev>
Date: Tue, 25 Jun 2024 12:31:11 +0800
From: Chengming Zhou <chengming.zhou@...ux.dev>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Minchan Kim <minchan@...nel.org>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Johannes Weiner <hannes@...xchg.org>, Nhat Pham <nphamcs@...il.com>,
Yu Zhao <yuzhao@...gle.com>, Takero Funaki <flintglass@...il.com>,
Chengming Zhou <zhouchengming@...edance.com>,
Dan Carpenter <dan.carpenter@...aro.org>, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] mm/zswap: use only one pool in zswap
On 2024/6/24 20:14, Yosry Ahmed wrote:
> On Fri, Jun 21, 2024 at 12:15 AM Chengming Zhou
> <chengming.zhou@...ux.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@...il.com>
>> Signed-off-by: Chengming Zhou <chengming.zhou@...ux.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!
Powered by blists - more mailing lists