[PATCH v1 1/1] mm: zswap: Move the crypto_acomp to be per pool, instead of per-CPU.

[PATCH v1 1/1] mm: zswap: Move the crypto_acomp to be per pool, instead of per-CPU.

[Kanchana P. Sridhar]

The "struct crypto_acomp" member of the per-CPU "struct
crypto_acomp_ctx" has been moved to "struct zswap_pool" since this is a
global entity representing the crypto algorithm.

We save 56 bytes per-CPU by not allocating per-CPU memory for the
crypto_acomp. The size of the global "struct zswap_pool" increases from
240 to 248 bytes as a result of this change.

Suggested-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>
---
 mm/zswap.c | 63 +++++++++++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index ad01c691a263..68825a6c0b68 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -151,7 +151,6 @@ struct zswap_comp_ctx {
 
 /* Separate requests, so that decompression does not wait for compression. */
 struct crypto_acomp_ctx {
-	struct crypto_acomp *acomp;
 	struct zswap_comp_ctx comp;
 	struct zswap_acomp_req decomp;	/* unused with on-stack requests */
 };
@@ -167,6 +166,7 @@ struct zswap_pool {
 	struct crypto_acomp_ctx __percpu *acomp_ctx;
 	struct percpu_ref ref;
 	struct rcu_work release_rwork;
+	struct crypto_acomp *acomp;
 	struct hlist_node node;
 	u8 idx;
 	char tfm_name[CRYPTO_MAX_ALG_NAME];
@@ -286,16 +286,6 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
 	acomp_request_free(acomp_ctx->decomp.req);
 	acomp_ctx->decomp.req = NULL;
 
-	/*
-	 * We have to handle both cases here: an error pointer return from
-	 * crypto_alloc_acomp_node(); and a) NULL initialization by zswap, or
-	 * b) NULL assignment done in a previous call to acomp_ctx_free().
-	 */
-	if (!IS_ERR_OR_NULL(acomp_ctx->acomp))
-		crypto_free_acomp(acomp_ctx->acomp);
-
-	acomp_ctx->acomp = NULL;
-
 	kfree(acomp_ctx->comp.buffer);
 	acomp_ctx->comp.buffer = NULL;
 }
@@ -322,6 +312,17 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
 
 	strscpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
 
+	/*
+	 * In case of an error, crypto_alloc_acomp() returns an
+	 * error pointer, never NULL.
+	 */
+	pool->acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
+	if (IS_ERR(pool->acomp)) {
+		pr_err("could not alloc crypto acomp %s : %pe\n",
+			pool->tfm_name, pool->acomp);
+		goto error;
+	}
+
 	/* Many things rely on the zero-initialization. */
 	pool->acomp_ctx = alloc_percpu_gfp(*pool->acomp_ctx,
 					   GFP_KERNEL | __GFP_ZERO);
@@ -385,6 +386,14 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
 error:
 	if (pool->acomp_ctx)
 		free_percpu(pool->acomp_ctx);
+
+	/*
+	 * We have to handle both cases here: an error pointer return from
+	 * crypto_alloc_acomp(); and NULL initialization by kzalloc_obj().
+	 */
+	if (!IS_ERR_OR_NULL(pool->acomp))
+		crypto_free_acomp(pool->acomp);
+
 	if (pool->zs_pool)
 		zs_destroy_pool(pool->zs_pool);
 	kfree(pool);
@@ -423,6 +432,13 @@ static void zswap_pool_destroy(struct zswap_pool *pool)
 
 	free_percpu(pool->acomp_ctx);
 
+	/*
+	 * We have to handle both cases here: an error pointer return from
+	 * crypto_alloc_acomp(); and NULL initialization by kzalloc_obj().
+	 */
+	if (!IS_ERR_OR_NULL(pool->acomp))
+		crypto_free_acomp(pool->acomp);
+
 	zs_destroy_pool(pool->zs_pool);
 	kfree(pool);
 }
@@ -840,34 +856,20 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 	 * To handle cases where the CPU goes through online-offline-online
 	 * transitions, we return if the acomp_ctx has already been initialized.
 	 */
-	if (acomp_ctx->acomp) {
-		WARN_ON_ONCE(IS_ERR(acomp_ctx->acomp));
+	if (acomp_ctx->comp.areq.req)
 		return 0;
-	}
 
 	acomp_ctx->comp.buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
 	if (!acomp_ctx->comp.buffer)
 		return ret;
 
-	/*
-	 * In case of an error, crypto_alloc_acomp_node() returns an
-	 * error pointer, never NULL.
-	 */
-	acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
-	if (IS_ERR(acomp_ctx->acomp)) {
-		pr_err("could not alloc crypto acomp %s : %pe\n",
-				pool->tfm_name, acomp_ctx->acomp);
-		ret = PTR_ERR(acomp_ctx->acomp);
-		goto fail;
-	}
-
-	ret = zswap_acomp_req_init(&acomp_ctx->comp.areq, acomp_ctx->acomp);
+	ret = zswap_acomp_req_init(&acomp_ctx->comp.areq, pool->acomp);
 	if (ret)
 		goto req_fail;
 
 	/* Skip the per-CPU request when decompression can use the stack. */
-	if (acomp_is_async(acomp_ctx->acomp)) {
-		ret = zswap_acomp_req_init(&acomp_ctx->decomp, acomp_ctx->acomp);
+	if (acomp_is_async(pool->acomp)) {
+		ret = zswap_acomp_req_init(&acomp_ctx->decomp, pool->acomp);
 		if (ret)
 			goto req_fail;
 	}
@@ -876,7 +878,6 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 
 req_fail:
 	pr_err("could not alloc crypto acomp_request %s\n", pool->tfm_name);
-fail:
 	acomp_ctx_free(acomp_ctx);
 	return ret;
 }
@@ -1031,7 +1032,7 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
 	 * Otherwise serialize decompression on the shared per-CPU request.
 	 */
 	if (!acomp_ctx->decomp.req) {
-		ACOMP_REQUEST_ON_STACK(req, acomp_ctx->acomp);
+		ACOMP_REQUEST_ON_STACK(req, pool->acomp);
 		DECLARE_CRYPTO_WAIT(wait);
 
 		zswap_set_acomp_req_callback(req, &wait);
-- 
2.39.5

Re: [PATCH v1 1/1] mm: zswap: Move the crypto_acomp to be per pool, instead of per-CPU.

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm: zswap: per-pool crypto_alloc_acomp() breaks NUMA and HW queue scaling
--

commit 0abfb2c1c527ffcd5ca8df2224cd1c4533f5face
Author: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>

mm: zswap: Move the crypto_acomp to be per pool, instead of per-CPU.

This commit moves the struct crypto_acomp instance from the per-CPU struct
crypto_acomp_ctx into the global struct zswap_pool, allocating a single
transform per pool in zswap_pool_create() instead of allocating one per CPU
in zswap_cpu_comp_prepare().

> diff --git a/mm/zswap.c b/mm/zswap.c
> index 37d217860a6b2..a958b74d4425d 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c

[ ... ]

> @@ -323,6 +313,17 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
>  
>  	strscpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
>  
> +	/*
> +	 * In case of an error, crypto_alloc_acomp() returns an
> +	 * error pointer, never NULL.
> +	 */
> +	pool->acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
> +	if (IS_ERR(pool->acomp)) {
> +		pr_err("could not alloc crypto acomp %s : %pe\n",
> +			pool->tfm_name, pool->acomp);
> +		goto error;
> +	}

[ ... ]

> @@ -841,34 +857,20 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>  	 * To handle cases where the CPU goes through online-offline-online
>  	 * transitions, we return if the acomp_ctx has already been initialized.
>  	 */
> -	if (acomp_ctx->acomp) {
> -		WARN_ON_ONCE(IS_ERR(acomp_ctx->acomp));
> +	if (acomp_ctx->comp.areq.req)
>  		return 0;
> -	}
>  
>  	acomp_ctx->comp.buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
>  	if (!acomp_ctx->comp.buffer)
>  		return ret;
>  
> -	/*
> -	 * In case of an error, crypto_alloc_acomp_node() returns an
> -	 * error pointer, never NULL.
> -	 */
> -	acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> -	if (IS_ERR(acomp_ctx->acomp)) {
> -		pr_err("could not alloc crypto acomp %s : %pe\n",
> -				pool->tfm_name, acomp_ctx->acomp);
> -		ret = PTR_ERR(acomp_ctx->acomp);
> -		goto fail;
> -	}
> -
> -	ret = zswap_acomp_req_init(&acomp_ctx->comp.areq, acomp_ctx->acomp);
> +	ret = zswap_acomp_req_init(&acomp_ctx->comp.areq, pool->acomp);

[Severity: Medium]
Does replacing the per-CPU crypto_alloc_acomp_node() call in
zswap_cpu_comp_prepare() with a single non-NUMA-aware crypto_alloc_acomp()
call in zswap_pool_create() cause a NUMA locality and hardware queue scaling
regression for hardware compression accelerators?

Previously, commits 7bc13b5b60e9 and 1ec3b5fe6eec allocated a dedicated
struct crypto_acomp per CPU on cpu_to_node(cpu) so that hardware compression
drivers would allocate per-CPU, NUMA-local hardware queues.

While software scomp backends and Intel IAA select per-CPU streams or
workqueues at request submission time, other hardware acomp drivers bind
hardware queues, rings, and NUMA-local accelerator devices to the struct
crypto_acomp transform during initialization based on the transform's node.

For example, in drivers/crypto/hisilicon/zip/zip_crypto.c,
hisi_zip_acomp_init() allocates one compression queue pair, one
decompression queue pair, and a per-transform request queue and spinlock on
tfm->base.node:

drivers/crypto/hisilicon/zip/zip_crypto.c:hisi_zip_acomp_init() {
	...
	ret = hisi_zip_ctx_init(ctx, COMP_NAME_TO_TYPE(alg_name), tfm->base.node);
	...
	ret = hisi_zip_create_req_q(ctx);
	...
}

Similarly, in drivers/crypto/intel/qat/qat_common/qat_comp_algs.c,
qat_comp_alg_init_tfm() binds the transform to a single hardware compression
ring pair on tfm->node:

drivers/crypto/intel/qat/qat_common/qat_comp_algs.c:qat_comp_alg_init_tfm() {
	...
	inst = qat_compression_get_instance_node(node, alg);
	if (!inst)
		return -EINVAL;
	ctx->inst = inst;
	...
}

With a single pool->acomp allocated via crypto_alloc_acomp() (passing
NUMA_NO_NODE), won't all CPUs across all NUMA nodes share and contend on a
single hardware queue or ring pair on one accelerator device while leaving
all other hardware queues and NUMA-local devices unused?

Also, if pool->acomp is shared across all CPUs, should the comment in
zswap_compress() be updated as well? It currently still states:

mm/zswap.c:zswap_compress() {
	...
	 * but in different threads running on different cpu, we have different
	 * acomp instance, and compression and decompression use separate
	 * requests, so multiple threads can do (de)compression in parallel.
	 */
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103000313.13677-1-kanchanapsridhar2026@gmail.com?part=1

Re: [PATCH v1 1/1] mm: zswap: Move the crypto_acomp to be per pool, instead of per-CPU.

[Yosry Ahmed]

On Mon, Nov 2, 2026 at 9:42 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] mm: zswap: per-pool crypto_alloc_acomp() breaks NUMA and HW queue scaling
> --
>
> commit 0abfb2c1c527ffcd5ca8df2224cd1c4533f5face
> Author: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>
>
> mm: zswap: Move the crypto_acomp to be per pool, instead of per-CPU.
>
> This commit moves the struct crypto_acomp instance from the per-CPU struct
> crypto_acomp_ctx into the global struct zswap_pool, allocating a single
> transform per pool in zswap_pool_create() instead of allocating one per CPU
> in zswap_cpu_comp_prepare().
>
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 37d217860a6b2..a958b74d4425d 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
>
> [ ... ]
>
> > @@ -323,6 +313,17 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
> >
> >       strscpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
> >
> > +     /*
> > +      * In case of an error, crypto_alloc_acomp() returns an
> > +      * error pointer, never NULL.
> > +      */
> > +     pool->acomp = crypto_alloc_acomp(pool->tfm_name, 0, 0);
> > +     if (IS_ERR(pool->acomp)) {
> > +             pr_err("could not alloc crypto acomp %s : %pe\n",
> > +                     pool->tfm_name, pool->acomp);
> > +             goto error;
> > +     }
>
> [ ... ]
>
> > @@ -841,34 +857,20 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> >        * To handle cases where the CPU goes through online-offline-online
> >        * transitions, we return if the acomp_ctx has already been initialized.
> >        */
> > -     if (acomp_ctx->acomp) {
> > -             WARN_ON_ONCE(IS_ERR(acomp_ctx->acomp));
> > +     if (acomp_ctx->comp.areq.req)
> >               return 0;
> > -     }
> >
> >       acomp_ctx->comp.buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
> >       if (!acomp_ctx->comp.buffer)
> >               return ret;
> >
> > -     /*
> > -      * In case of an error, crypto_alloc_acomp_node() returns an
> > -      * error pointer, never NULL.
> > -      */
> > -     acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > -     if (IS_ERR(acomp_ctx->acomp)) {
> > -             pr_err("could not alloc crypto acomp %s : %pe\n",
> > -                             pool->tfm_name, acomp_ctx->acomp);
> > -             ret = PTR_ERR(acomp_ctx->acomp);
> > -             goto fail;
> > -     }
> > -
> > -     ret = zswap_acomp_req_init(&acomp_ctx->comp.areq, acomp_ctx->acomp);
> > +     ret = zswap_acomp_req_init(&acomp_ctx->comp.areq, pool->acomp);
>
> [Severity: Medium]
> Does replacing the per-CPU crypto_alloc_acomp_node() call in
> zswap_cpu_comp_prepare() with a single non-NUMA-aware crypto_alloc_acomp()
> call in zswap_pool_create() cause a NUMA locality and hardware queue scaling
> regression for hardware compression accelerators?
>
> Previously, commits 7bc13b5b60e9 and 1ec3b5fe6eec allocated a dedicated
> struct crypto_acomp per CPU on cpu_to_node(cpu) so that hardware compression
> drivers would allocate per-CPU, NUMA-local hardware queues.
>
> While software scomp backends and Intel IAA select per-CPU streams or
> workqueues at request submission time, other hardware acomp drivers bind
> hardware queues, rings, and NUMA-local accelerator devices to the struct
> crypto_acomp transform during initialization based on the transform's node.

Adding Barry here, I think this undos the optimization he previously added.

>
> For example, in drivers/crypto/hisilicon/zip/zip_crypto.c,
> hisi_zip_acomp_init() allocates one compression queue pair, one
> decompression queue pair, and a per-transform request queue and spinlock on
> tfm->base.node:
>
> drivers/crypto/hisilicon/zip/zip_crypto.c:hisi_zip_acomp_init() {
>         ...
>         ret = hisi_zip_ctx_init(ctx, COMP_NAME_TO_TYPE(alg_name), tfm->base.node);
>         ...
>         ret = hisi_zip_create_req_q(ctx);
>         ...
> }
>
> Similarly, in drivers/crypto/intel/qat/qat_common/qat_comp_algs.c,
> qat_comp_alg_init_tfm() binds the transform to a single hardware compression
> ring pair on tfm->node:
>
> drivers/crypto/intel/qat/qat_common/qat_comp_algs.c:qat_comp_alg_init_tfm() {
>         ...
>         inst = qat_compression_get_instance_node(node, alg);
>         if (!inst)
>                 return -EINVAL;
>         ctx->inst = inst;
>         ...
> }
>
> With a single pool->acomp allocated via crypto_alloc_acomp() (passing
> NUMA_NO_NODE), won't all CPUs across all NUMA nodes share and contend on a
> single hardware queue or ring pair on one accelerator device while leaving
> all other hardware queues and NUMA-local devices unused?
>
> Also, if pool->acomp is shared across all CPUs, should the comment in
> zswap_compress() be updated as well? It currently still states:
>
> mm/zswap.c:zswap_compress() {
>         ...
>          * but in different threads running on different cpu, we have different
>          * acomp instance, and compression and decompression use separate
>          * requests, so multiple threads can do (de)compression in parallel.
>          */
> }
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261103000313.13677-1-kanchanapsridhar2026@gmail.com?part=1