Re: [PATCH v4 06/24] s390/zcrypt: Introduce cprb mempool for cca misc functions

[Holger Dengler <dengler@linux.ibm.com>]

On 09/04/2025 16:02, Harald Freudenberger wrote:
> Introduce a new module parameter "zcrypt_mempool_threshold"
> for the zcrypt module. This parameter controls the minimal
> amount of mempool items which are pre-allocated for urgent
> requests/replies and will be used with the support for the
> new xflag ZCRYPT_XFLAG_NOMEMALLOC. The default value of 5
> shall provide enough memory items to support up to 5 requests
> (and their associated reply) in parallel. The minimum value
> is 1 and is checked and maybe adjusted in the module init().
> 
> If the mempool is depleted upon one cca misc functions is called
> with the named xflag set, the function will fail with -ENOMEM
> and the caller is responsible for taking further actions.
> 
> For CCA each mempool item is 16KB, as a CCA CPRB needs to
> hold the request and the reply. The pool items only support
> requests/replies with a limit of about 8KB.
> So by default the CCA mempool consumes
>   5 * 16KB = 80KB
> 
> This is only part of an rework to support a new xflag
> ZCRYPT_XFLAG_NOMEMALLOC but not yet complete.
> 
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>

See my comment below. With these fixed you can keep my R-b.

> ---
>  drivers/s390/crypto/zcrypt_api.c     |  16 +++-
>  drivers/s390/crypto/zcrypt_api.h     |   2 +
>  drivers/s390/crypto/zcrypt_ccamisc.c | 116 ++++++++++++++++++++-------
>  drivers/s390/crypto/zcrypt_ccamisc.h |   1 +
>  4 files changed, 104 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
> index f753c0403a18..888ab289bd10 100644
> --- a/drivers/s390/crypto/zcrypt_api.c
> +++ b/drivers/s390/crypto/zcrypt_api.c
> @@ -50,6 +50,10 @@ MODULE_DESCRIPTION("Cryptographic Coprocessor interface, " \
>  		   "Copyright IBM Corp. 2001, 2012");
>  MODULE_LICENSE("GPL");
>  
> +unsigned int zcrypt_mempool_threshold = 5;
> +module_param_named(mempool_threshold, zcrypt_mempool_threshold, uint, 0440);
> +MODULE_PARM_DESC(mempool_threshold, "CCA and EP11 request/reply mempool minimal items.");

Maybe you can mention the minimum value here as well?

"CCA and EP11 request/reply mempool minimal items (min: 1)."

> +
>  /*
>   * zcrypt tracepoint functions
>   */
> @@ -2147,13 +2151,20 @@ int __init zcrypt_api_init(void)
>  {
>  	int rc;
>  
> +	/* make sure the mempool threshold is >= 1 */
> +	zcrypt_mempool_threshold = max_t(unsigned int, zcrypt_mempool_threshold, 1);
> +

BTW: As far as I can see, mempool allows 0 as minimal preallocated elements. The result will be a mempool without any pre-allocated elements. This means, no NOMEMALLOC request could be processed. This is sad, but it is not really an error.

Anyhow, if you would limit the mempool to at least 1 element, a threshold value < 1 is an error and should be treated as such. So, do not silently fix the value, but printout an error message and return with -EINVAL here.

>  	rc = zcrypt_debug_init();
>  	if (rc)
>  		goto out;
>  
>  	rc = zcdn_init();
>  	if (rc)
> -		goto out;
> +		goto out_zcdn_init_failed;
> +
> +	rc = zcrypt_ccamisc_init();
> +	if (rc)
> +		goto out_ccamisc_init_failed;
>  
>  	/* Register the request sprayer. */
>  	rc = misc_register(&zcrypt_misc_device);
[...]
> diff --git a/drivers/s390/crypto/zcrypt_api.h b/drivers/s390/crypto/zcrypt_api.h
> index 94dffb01942f..84d636fd14a4 100644
> --- a/drivers/s390/crypto/zcrypt_api.h
> +++ b/drivers/s390/crypto/zcrypt_api.h
[...]
> diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c b/drivers/s390/crypto/zcrypt_ccamisc.c
> index 521baaea06ff..05085b40a55c 100644
> --- a/drivers/s390/crypto/zcrypt_ccamisc.c
> +++ b/drivers/s390/crypto/zcrypt_ccamisc.c
[...]

> @@ -229,7 +241,16 @@ static int alloc_and_prep_cprbmem(size_t paramblen,
>  	 * allocate consecutive memory for request CPRB, request param
>  	 * block, reply CPRB and reply param block
>  	 */
> -	cprbmem = kcalloc(2, cprbplusparamblen, GFP_KERNEL);
> +	if (xflags & ZCRYPT_XFLAG_NOMEMALLOC) {
> +		size_t len = 2 * cprbplusparamblen;
> +
> +		if (cprb_mempool && len <= CPRB_MEMPOOL_ITEM_SIZE) {

Remove the check for cprb_mempool != NULL. This case can never happen.

> +			cprbmem = mempool_alloc_preallocated(cprb_mempool);
> +			memset(cprbmem, 0, len);

Check for cprbmem != NULL before memset(), or move the memset() down.

> +		}
> +	} else {
> +		cprbmem = kcalloc(2, cprbplusparamblen, GFP_KERNEL);

Use kmalloc here and ...

> +	}
>  	if (!cprbmem)
>  		return -ENOMEM;

... wait until cprbmem is always valid and do the memset() here.

if (xflags & ZCRYPT_XFLAG_NOMEMALLOC) {
	size_t len = 2 * cprbplusparamblen;

	if (len <= CPRB_MEMPOOL_ITEM_SIZE)
		cprbmem = mempool_alloc_preallocated(cprb_mempool);
} else {
	cprbmem = kmalloc(2 * cprbplusparamblen, GFP_KERNEL);
}
if (!cprbmem)
	return -ENOMEM;
memset(cprbmem, 0, len);

[...]

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com