Re: [PATCH v2 03/20] s390/ap: Introduce ap message buffer pool

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

On 2025-03-17 17:14, Holger Dengler wrote:
> On 04/03/2025 18:20, Harald Freudenberger wrote:
>> There is a need for a do-not-allocate-memory path through the
>> ap bus layer. When ap_init_apmsg() with use_mempool set to true
>> is called, instead of kmalloc() the ap message buffer is
>> allocated from the ap_msg_pool. This pool only holds a limited
>> amount of buffers: ap_msg_pool_min_items with the item size
>> AP_DEFAULT_MAX_MSG_SIZE and exactly one of these items (if available)
>> is returned if ap_init_apmsg() with the use_mempool arg set to true
>> is called. When this pool is exhausted and use_mempool is set true,
>> ap_init_apmsg() returns -ENOMEM without any attempt to allocate
>> memory.
>> 
>> Default values for this mempool of ap messages is:
>>  * Each buffer is 12KB (that is the default AP bus size
>>    and all the urgend messages should fit into this space).
> 
> typo: urgent
> 

done -> v3

>>  * Minimum items held in the pool is 8. This value is adjustable
>>    via module parameter ap.msgpool_min_items.
>> 
>> The zcrypt layer may use this flag to indicate to the ap bus
>> that the processing path for this message should not allocate
>> memory but should use pre-allocated memory buffer instead.
>> This is to prevent deadlocks with crypto and io for example
>> with encrypted swap volumes.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> 
> With the typo and the changed indent (see comment below)
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
> 
>> ---
>>  drivers/s390/crypto/ap_bus.c     | 57 
>> ++++++++++++++++++++++++++++----
>>  drivers/s390/crypto/ap_bus.h     |  9 ++---
>>  drivers/s390/crypto/zcrypt_api.c | 10 +++---
>>  3 files changed, 60 insertions(+), 16 deletions(-)
>> 
> [...]
>> diff --git a/drivers/s390/crypto/ap_bus.h 
>> b/drivers/s390/crypto/ap_bus.h
>> index 483231bcdea6..a7bd44e5cc76 100644
>> --- a/drivers/s390/crypto/ap_bus.h
>> +++ b/drivers/s390/crypto/ap_bus.h
>> @@ -233,11 +233,12 @@ struct ap_message {
>>  			struct ap_message *);
>>  };
>> 
>> -#define AP_MSG_FLAG_SPECIAL  0x0001	/* flag msg as 'special' with 
>> NQAP */
>> -#define AP_MSG_FLAG_USAGE    0x0002	/* CCA, EP11: usage (no admin) 
>> msg */
>> -#define AP_MSG_FLAG_ADMIN    0x0004	/* CCA, EP11: admin (=control) 
>> msg */
>> +#define AP_MSG_FLAG_SPECIAL  0x0001   /* flag msg as 'special' with 
>> NQAP */
>> +#define AP_MSG_FLAG_USAGE    0x0002   /* CCA, EP11: usage (no admin) 
>> msg */
>> +#define AP_MSG_FLAG_ADMIN    0x0004   /* CCA, EP11: admin (=control) 
>> msg */
>> +#define AP_MSG_FLAG_MEMPOOL  0x0008   /* ap msg buffer allocated from 
>> mempool */
> 
> Please do not change the indent here. The change is not required, but
> it makes the life of reviewers much harder.
> 

corrected -> v3

>> 
>> -int ap_init_apmsg(struct ap_message *ap_msg);
>> +int ap_init_apmsg(struct ap_message *ap_msg, bool use_mempool);
>>  void ap_release_apmsg(struct ap_message *ap_msg);
>> 
>>  enum ap_sm_wait ap_sm_event(struct ap_queue *aq, enum ap_sm_event 
>> event);
> [...]