1. Patchew
  2. QEMU
  3. View series

[PATCH] target/s390x/arch_dump: Simplify memory allocation in s390x_write_elf64_notes()

Thomas Huth posted 1 patch 1 year, 2 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
target/s390x/arch_dump.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
[PATCH] target/s390x/arch_dump: Simplify memory allocation in s390x_write_elf64_notes()
Posted by Thomas Huth 1 year, 2 months ago 
We are not on a hot path here, so there is no real need for the logic
here with the split heap and stack space allocation. Simplify it by
always allocating memory from the heap.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 Based-on: <20230214141056.680969-1-thuth@redhat.com>

 target/s390x/arch_dump.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index a7c44ba49d..84e17effda 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -227,25 +227,23 @@ static int s390x_write_elf64_notes(const char *note_name,
                                        DumpState *s,
                                        const NoteFuncDesc *funcs)
 {
-    Note note, *notep;
+    g_autofree Note *notep = NULL;
     const NoteFuncDesc *nf;
-    int note_size, content_size;
+    int note_size, prev_size = 0, content_size;
     int ret = -1;
 
-    assert(strlen(note_name) < sizeof(note.name));
+    assert(strlen(note_name) < sizeof(notep->name));
 
     for (nf = funcs; nf->note_contents_func; nf++) {
-        notep = &note;
         if (nf->pvonly && !s390_is_pv()) {
             continue;
         }
 
         content_size = nf->note_size_func ? nf->note_size_func() : nf->contents_size;
-        note_size = sizeof(note) - sizeof(notep->contents) + content_size;
+        note_size = sizeof(Note) - sizeof(notep->contents) + content_size;
 
-        /* Notes with dynamic sizes need to allocate a note */
-        if (nf->note_size_func) {
-            notep = g_malloc(note_size);
+        if (prev_size < note_size) {
+            notep = g_realloc(notep, note_size);
         }
 
         memset(notep, 0, note_size);
@@ -258,15 +256,9 @@ static int s390x_write_elf64_notes(const char *note_name,
         /* Get contents and write them out */
         (*nf->note_contents_func)(notep, cpu, id);
         ret = f(notep, note_size, s);
-
-        if (nf->note_size_func) {
-            g_free(notep);
-        }
-
         if (ret < 0) {
             return -1;
         }
-
     }
 
     return 0;
-- 
2.31.1
Re: [PATCH] target/s390x/arch_dump: Simplify memory allocation in s390x_write_elf64_notes()
Posted by Thomas Huth 1 year, 2 months ago 
On 15/02/2023 06.48, Thomas Huth wrote:
> We are not on a hot path here, so there is no real need for the logic
> here with the split heap and stack space allocation. Simplify it by
> always allocating memory from the heap.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   Based-on: <20230214141056.680969-1-thuth@redhat.com>
> 
>   target/s390x/arch_dump.c | 20 ++++++--------------
>   1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index a7c44ba49d..84e17effda 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -227,25 +227,23 @@ static int s390x_write_elf64_notes(const char *note_name,
>                                          DumpState *s,
>                                          const NoteFuncDesc *funcs)
>   {
> -    Note note, *notep;
> +    g_autofree Note *notep = NULL;
>       const NoteFuncDesc *nf;
> -    int note_size, content_size;
> +    int note_size, prev_size = 0, content_size;
>       int ret = -1;
>   
> -    assert(strlen(note_name) < sizeof(note.name));
> +    assert(strlen(note_name) < sizeof(notep->name));
>   
>       for (nf = funcs; nf->note_contents_func; nf++) {
> -        notep = &note;
>           if (nf->pvonly && !s390_is_pv()) {
>               continue;
>           }
>   
>           content_size = nf->note_size_func ? nf->note_size_func() : nf->contents_size;
> -        note_size = sizeof(note) - sizeof(notep->contents) + content_size;
> +        note_size = sizeof(Note) - sizeof(notep->contents) + content_size;
>   
> -        /* Notes with dynamic sizes need to allocate a note */
> -        if (nf->note_size_func) {
> -            notep = g_malloc(note_size);
> +        if (prev_size < note_size) {
> +            notep = g_realloc(notep, note_size);
>           }

-ENOTENOUGHCOFFEEYET

Janosch just told me that there is a prev_size = note_size missing here ... 
sorry for that, I'll send a v2.

  Thomas
Re: [PATCH] target/s390x/arch_dump: Simplify memory allocation in s390x_write_elf64_notes()
Posted by Philippe Mathieu-Daudé 1 year, 2 months ago 
On 15/2/23 06:48, Thomas Huth wrote:
> We are not on a hot path here, so there is no real need for the logic
> here with the split heap and stack space allocation. Simplify it by
> always allocating memory from the heap.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   Based-on: <20230214141056.680969-1-thuth@redhat.com>
> 
>   target/s390x/arch_dump.c | 20 ++++++--------------
>   1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index a7c44ba49d..84e17effda 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -227,25 +227,23 @@ static int s390x_write_elf64_notes(const char *note_name,
>                                          DumpState *s,
>                                          const NoteFuncDesc *funcs)
>   {
> -    Note note, *notep;
> +    g_autofree Note *notep = NULL;
>       const NoteFuncDesc *nf;
> -    int note_size, content_size;
> +    int note_size, prev_size = 0, content_size;

We can start with:

   prev_size = sizeof(Note);

If this goes thru your tree, feel free to modify without respining.


Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

>       int ret = -1;
>   
> -    assert(strlen(note_name) < sizeof(note.name));
> +    assert(strlen(note_name) < sizeof(notep->name));
>   
>       for (nf = funcs; nf->note_contents_func; nf++) {
> -        notep = &note;
>           if (nf->pvonly && !s390_is_pv()) {
>               continue;
>           }
>   
>           content_size = nf->note_size_func ? nf->note_size_func() : nf->contents_size;
> -        note_size = sizeof(note) - sizeof(notep->contents) + content_size;
> +        note_size = sizeof(Note) - sizeof(notep->contents) + content_size;
>   
> -        /* Notes with dynamic sizes need to allocate a note */
> -        if (nf->note_size_func) {
> -            notep = g_malloc(note_size);
> +        if (prev_size < note_size) {
> +            notep = g_realloc(notep, note_size);
>           }
Re: [PATCH] target/s390x/arch_dump: Simplify memory allocation in s390x_write_elf64_notes()
Posted by Thomas Huth 1 year, 2 months ago 
On 15/02/2023 08.10, Philippe Mathieu-Daudé wrote:
> On 15/2/23 06:48, Thomas Huth wrote:
>> We are not on a hot path here, so there is no real need for the logic
>> here with the split heap and stack space allocation. Simplify it by
>> always allocating memory from the heap.
>>
>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   Based-on: <20230214141056.680969-1-thuth@redhat.com>
>>
>>   target/s390x/arch_dump.c | 20 ++++++--------------
>>   1 file changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
>> index a7c44ba49d..84e17effda 100644
>> --- a/target/s390x/arch_dump.c
>> +++ b/target/s390x/arch_dump.c
>> @@ -227,25 +227,23 @@ static int s390x_write_elf64_notes(const char 
>> *note_name,
>>                                          DumpState *s,
>>                                          const NoteFuncDesc *funcs)
>>   {
>> -    Note note, *notep;
>> +    g_autofree Note *notep = NULL;
>>       const NoteFuncDesc *nf;
>> -    int note_size, content_size;
>> +    int note_size, prev_size = 0, content_size;
> 
> We can start with:
> 
>    prev_size = sizeof(Note);
> 
> If this goes thru your tree, feel free to modify without respining.

But then I'd also need to initialize notep above differently, don't I?
And if I've got it right, this function sometimes also deals with chunks 
that are smaller, so I think it's cleaner if we start with zero instead of 
sizeof(Note).

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Thanks!

  Thomas
Re: [PATCH] target/s390x/arch_dump: Simplify memory allocation in s390x_write_elf64_notes()
Posted by Philippe Mathieu-Daudé 1 year, 2 months ago 
On 15/2/23 08:26, Thomas Huth wrote:
> On 15/02/2023 08.10, Philippe Mathieu-Daudé wrote:
>> On 15/2/23 06:48, Thomas Huth wrote:
>>> We are not on a hot path here, so there is no real need for the logic
>>> here with the split heap and stack space allocation. Simplify it by
>>> always allocating memory from the heap.
>>>
>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   Based-on: <20230214141056.680969-1-thuth@redhat.com>
>>>
>>>   target/s390x/arch_dump.c | 20 ++++++--------------
>>>   1 file changed, 6 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
>>> index a7c44ba49d..84e17effda 100644
>>> --- a/target/s390x/arch_dump.c
>>> +++ b/target/s390x/arch_dump.c
>>> @@ -227,25 +227,23 @@ static int s390x_write_elf64_notes(const char 
>>> *note_name,
>>>                                          DumpState *s,
>>>                                          const NoteFuncDesc *funcs)
>>>   {
>>> -    Note note, *notep;
>>> +    g_autofree Note *notep = NULL;
>>>       const NoteFuncDesc *nf;
>>> -    int note_size, content_size;
>>> +    int note_size, prev_size = 0, content_size;
>>
>> We can start with:
>>
>>    prev_size = sizeof(Note);
>>
>> If this goes thru your tree, feel free to modify without respining.
> 
> But then I'd also need to initialize notep above differently, don't I?
> And if I've got it right, this function sometimes also deals with chunks 
> that are smaller, so I think it's cleaner if we start with zero instead 
> of sizeof(Note).

Oh you are right. We are good then!

>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> Thanks!
> 
>   Thomas
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.