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

Holger Dengler
2025-04-11T12:41:01Z

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