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

Harald Freudenberger
2025-04-14T14:17:25Z

On 2025-04-11 14:40, Holger Dengler wrote:
> 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)."

Done

>
>> +
>> /*
>> * 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);
>
> [...]

Hm, not clear what exactly you mean. I changed the code to:

{
u8 *cprbmem = NULL;
size_t cprbplusparamblen = sizeof(struct CPRBX) + paramblen;
size_t len = 2 * cprbplusparamblen;
struct CPRBX *preqcblk, *prepcblk;

/*
* allocate consecutive memory for request CPRB, request param
* block, reply CPRB and reply param block
*/
if (xflags & ZCRYPT_XFLAG_NOMEMALLOC) {
if (len <= CPRB_MEMPOOL_ITEM_SIZE)
cprbmem = mempool_alloc_preallocated(cprb_mempool);
} else {
cprbmem = kmalloc(len, GFP_KERNEL);
}
if (!cprbmem)
return -ENOMEM;
memset(cprbmem, 0, len);
...