drivers/s390/crypto/zcrypt_card.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
From: Yunseong Kim <yskelg@gmail.com>
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/
Cc: linux-s390@vger.kernel.org
Signed-off-by: Yunseong Kim <yskelg@gmail.com>
---
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)
+ maxzqs++;
if (maxzqs > 0)
- zq_uelist = kcalloc(maxzqs + 1, sizeof(*zq_uelist), GFP_ATOMIC);
+ zq_uelist = kcalloc(maxzqs, sizeof(*zq_uelist), GFP_ATOMIC);
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]);
}
+ kfree(zq_uelist);
return count;
}
--
2.45.2On 2024-06-25 00:29, yskelg@gmail.com wrote:
> From: Yunseong Kim <yskelg@gmail.com>
>
> 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@vger.kernel.org
> Signed-off-by: Yunseong Kim <yskelg@gmail.com>
> ---
> 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
Hi Harald,
On 6/25/24 5:27 오후, Harald Freudenberger wrote:
> On 2024-06-25 00:29, yskelg@gmail.com wrote:
>> From: Yunseong Kim <yskelg@gmail.com>
>>
>> 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@vger.kernel.org
>> Signed-off-by: Yunseong Kim <yskelg@gmail.com>
>> ---
>> 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.
Thank you Harald for the code review! Oh I see, thanks for the advice.
I was wondering if it was useful when I was coding it too.
>> + 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.
Oh this is where I wrote the code without understanding it properly,
thanks for the guidance!
>> }
>> + 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.
I'm so sorry Harald, This was missing judgment, I should have checked it
one last time before sending v2 patch mail.
> regards Harald Freudenberger
I truly appreciate Harald for the detailed code review of my patch.,
even though it may be less understanding in many part.
Thank you very much again!
Warm regards,
Yunseong Kim
© 2016 - 2025 Red Hat, Inc.