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

Harald Freudenberger
2025-03-24T14:41:23Z

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