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

[sashiko-bot@kernel.org]

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