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

[Harald Freudenberger <freude@linux.ibm.com>]

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);
         ...