Re: [PATCH v2] s390/zcrypt: optimizes memory allocation in online_show()

[Harald Freudenberger]

On 2024-06-25 00:29, yskelg@xxxxxxxxx wrote:
> From: Yunseong Kim <yskelg@xxxxxxxxx>
>
> Make memory allocation more precise (based on maxzqs) by allocating
> memory only for the queues that are truly affected by the online state
> changes.
>
> Fixes: df6f508c68db ("s390/ap/zcrypt: notify userspace with online,
> config and mode info")
> Link:
> https://lore.kernel.org/linux-s390/your-ad-here.call-01625406648-ext-2488@work.hours/

What is this Link here? It is pointing to a PR for a 5.14 kernel and has 
no relation to this patch.

> Cc: linux-s390@xxxxxxxxxxxxxxx
> Signed-off-by: Yunseong Kim <yskelg@xxxxxxxxx>
> ---
>  drivers/s390/crypto/zcrypt_card.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/s390/crypto/zcrypt_card.c
> b/drivers/s390/crypto/zcrypt_card.c
> index 050462d95222..2c80be3f2a00 100644
> --- a/drivers/s390/crypto/zcrypt_card.c
> +++ b/drivers/s390/crypto/zcrypt_card.c
> @@ -88,9 +88,10 @@ static ssize_t online_store(struct device *dev,
>  	 * the zqueue objects, we make sure they exist after lock release.
>  	 */
>  	list_for_each_entry(zq, &zc->zqueues, list)
> -		maxzqs++;
> +		if (!!zq->online != !!online)

I don't like this line. It is code duplication from the zcrypt_queue.c 
file
and uses knowledge about the internals of the zqueue which is not 
appropriate
here in zcrypt_card.c. Please note also that usually the total number of
queues attached to a card is in a one digit range. As kcalloc() anyway 
uses
the kmalloc pool which is ordered in powers of two it is unlikely to 
really
spare some memory by only allocating a pointer space for the online 
queues.

> +			maxzqs++;
>  	if (maxzqs > 0)
> -		zq_uelist = kcalloc(maxzqs + 1, sizeof(*zq_uelist), GFP_ATOMIC);
> +		zq_uelist = kcalloc(maxzqs, sizeof(*zq_uelist), GFP_ATOMIC);

Your improvement about removal of the +1 and use the i value later 
instead
of my implementation which uses a NULL as end of list is valid and makes 
sense
to me.

>  	list_for_each_entry(zq, &zc->zqueues, list)
>  		if (zcrypt_queue_force_online(zq, online))
>  			if (zq_uelist) {
> @@ -98,14 +99,11 @@ static ssize_t online_store(struct device *dev,
>  				zq_uelist[i++] = zq;
>  			}
>  	spin_unlock(&zcrypt_list_lock);
> -	if (zq_uelist) {
> -		for (i = 0; zq_uelist[i]; i++) {
> -			zq = zq_uelist[i];
> -			ap_send_online_uevent(&zq->queue->ap_dev, online);
> -			zcrypt_queue_put(zq);
> -		}
> -		kfree(zq_uelist);
> +	while (i--) {
> +		ap_send_online_uevent(&zq->queue->ap_dev, online);
> +		zcrypt_queue_put(zq_uelist[i]);

The content of this while loop is NOT covering the old code. zq is not
set any more and thus the ap_sen_online_uevent() uses a random zq which
is a left over from the list_for_each() loop.

>  	}
> +	kfree(zq_uelist);
>
>  	return count;
>  }

You sent another patch for the online_store() function with exactly the
same code changes. I would see these changes as one patch and don't want
to have more or less equal changes spread over two patches.

I am sorry, I will not pick this and the online_store() patch.

regards Harald Freudenberger