[v3] delay timer_new from init to realize to fix memleaks.

[PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by Pan Nengyuan 5 years, 8 months ago

This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:

Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
    #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
    #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
    #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
    #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
    #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
    #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
    #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
    #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
    #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
    #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
    #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
    #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Richard Henderson <rth@twiddle.net>
Cc: David Hildenbrand <david@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: qemu-s390x@nongnu.org
---
v2->v1:
- Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
v3->v2:
- Also do the timer_free in unrealize, it seems more balance.
---
 target/s390x/cpu.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index cf84d307c6..cc63c9db22 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
     S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
 #if !defined(CONFIG_USER_ONLY)
     S390CPU *cpu = S390_CPU(dev);
+    cpu->env.tod_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
+    cpu->env.cpu_timer =
+        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
 #endif
+
     Error *err = NULL;
 
     /* the model has to be realized before qemu_init_vcpu() due to kvm */
@@ -227,6 +232,18 @@ out:
     error_propagate(errp, err);
 }
 
+static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp)
+{
+#if !defined(CONFIG_USER_ONLY)
+    S390CPU *cpu = S390_CPU(dev);
+
+    timer_del(cpu->env.tod_timer);
+    timer_del(cpu->env.cpu_timer);
+    timer_free(cpu->env.tod_timer);
+    timer_free(cpu->env.cpu_timer);
+#endif
+}
+
 static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs)
 {
     GuestPanicInformation *panic_info;
@@ -279,10 +296,6 @@ static void s390_cpu_initfn(Object *obj)
                         s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
     s390_cpu_model_register_props(obj);
 #if !defined(CONFIG_USER_ONLY)
-    cpu->env.tod_timer =
-        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
-    cpu->env.cpu_timer =
-        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 #endif
 }
@@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
 
     device_class_set_parent_realize(dc, s390_cpu_realizefn,
                                     &scc->parent_realize);
+    dc->unrealize = s390_cpu_unrealizefn;
     device_class_set_props(dc, s390x_cpu_properties);
     dc->user_creatable = true;
 
-- 
2.18.2

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by David Hildenbrand 5 years, 8 months ago

On 27.02.20 03:50, Pan Nengyuan wrote:
> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
> 
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> ---
> v2->v1:
> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
> v3->v2:
> - Also do the timer_free in unrealize, it seems more balance.
> ---


As I already said, I think this is init and not realize stuff. Do we
have a convention now and documented that?

Anyhow, I don't really care
[...]


> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>  
>      device_class_set_parent_realize(dc, s390_cpu_realizefn,
>                                      &scc->parent_realize);
> +    dc->unrealize = s390_cpu_unrealizefn;

Shouldn't we use device_class_set_parent_unrealize?


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by Philippe Mathieu-Daudé 5 years, 8 months ago

On 2/27/20 9:41 AM, David Hildenbrand wrote:
> On 27.02.20 03:50, Pan Nengyuan wrote:
>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>
>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>      #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>      #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>      #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>      #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>      #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>      #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>      #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>      #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>      #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>      #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>      #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>      #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>      #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> ---
>> v2->v1:
>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>> v3->v2:
>> - Also do the timer_free in unrealize, it seems more balance.
>> ---
> 
> 
> As I already said, I think this is init and not realize stuff. Do we
> have a convention now and documented that?

The clearer doc I read so far is this post:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg680187.html
(but see the thread for more helpful comments)

Another thread that you might find interesting is "how to handle QOM 
'container' objects whose contents depend on QOM properties?"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg511703.html

> 
> Anyhow, I don't really care
> [...]

Well, looking at the time spent on these series and their review, having 
it better documented might save time the whole community.

> 
>> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>   
>>       device_class_set_parent_realize(dc, s390_cpu_realizefn,
>>                                       &scc->parent_realize);
>> +    dc->unrealize = s390_cpu_unrealizefn;
> 
> Shouldn't we use device_class_set_parent_unrealize?
> 
>

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by David Hildenbrand 5 years, 8 months ago

On 27.02.20 09:55, Philippe Mathieu-Daudé wrote:
> On 2/27/20 9:41 AM, David Hildenbrand wrote:
>> On 27.02.20 03:50, Pan Nengyuan wrote:
>>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>>
>>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>>      #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>>      #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>>      #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>>      #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>>      #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>>      #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>>      #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>>      #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>>      #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>>      #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>>      #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>>      #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>>      #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>> ---
>>> Cc: Richard Henderson <rth@twiddle.net>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Cornelia Huck <cohuck@redhat.com>
>>> Cc: qemu-s390x@nongnu.org
>>> ---
>>> v2->v1:
>>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>>> v3->v2:
>>> - Also do the timer_free in unrealize, it seems more balance.
>>> ---
>>
>>
>> As I already said, I think this is init and not realize stuff. Do we
>> have a convention now and documented that?
> 
> The clearer doc I read so far is this post:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg680187.html
> (but see the thread for more helpful comments)
> 
> Another thread that you might find interesting is "how to handle QOM 
> 'container' objects whose contents depend on QOM properties?"
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg511703.html
> 
>>
>> Anyhow, I don't really care
>> [...]
> 
> Well, looking at the time spent on these series and their review, having 
> it better documented might save time the whole community.

Thanks for the pointers. Yes, we should document that. Especially if it
might save me some time ;)

Moving stuff around without a clear convention is not-so-nice IMHO.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by Pan Nengyuan 5 years, 8 months ago


On 2/27/2020 4:41 PM, David Hildenbrand wrote:
> On 27.02.20 03:50, Pan Nengyuan wrote:
>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>
>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> ---
>> v2->v1:
>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>> v3->v2:
>> - Also do the timer_free in unrealize, it seems more balance.
>> ---
> 
> 
> As I already said, I think this is init and not realize stuff. Do we
> have a convention now and documented that?
> 
> Anyhow, I don't really care
> [...]
> 
> 
>> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>  
>>      device_class_set_parent_realize(dc, s390_cpu_realizefn,
>>                                      &scc->parent_realize);
>> +    dc->unrealize = s390_cpu_unrealizefn;
> 
> Shouldn't we use device_class_set_parent_unrealize?

We just only declare parent_realize field in S390CPUClass(), it seems nothing to do in parent_unrealize.

typedef struct S390CPUClass {
...
DeviceRealize parent_realize;    // no parent_unrealize;
...
}

So I think we can't use it.

Thanks.

> 
>

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by David Hildenbrand 5 years, 8 months ago

On 27.02.20 09:58, Pan Nengyuan wrote:
> 
> 
> On 2/27/2020 4:41 PM, David Hildenbrand wrote:
>> On 27.02.20 03:50, Pan Nengyuan wrote:
>>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>>
>>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>> ---
>>> Cc: Richard Henderson <rth@twiddle.net>
>>> Cc: David Hildenbrand <david@redhat.com>
>>> Cc: Cornelia Huck <cohuck@redhat.com>
>>> Cc: qemu-s390x@nongnu.org
>>> ---
>>> v2->v1:
>>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>>> v3->v2:
>>> - Also do the timer_free in unrealize, it seems more balance.
>>> ---
>>
>>
>> As I already said, I think this is init and not realize stuff. Do we
>> have a convention now and documented that?
>>
>> Anyhow, I don't really care
>> [...]
>>
>>
>>> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>>  
>>>      device_class_set_parent_realize(dc, s390_cpu_realizefn,
>>>                                      &scc->parent_realize);
>>> +    dc->unrealize = s390_cpu_unrealizefn;
>>
>> Shouldn't we use device_class_set_parent_unrealize?
> 
> We just only declare parent_realize field in S390CPUClass(), it seems nothing to do in parent_unrealize.
> 
> typedef struct S390CPUClass {
> ...
> DeviceRealize parent_realize;    // no parent_unrealize;
> ...
> }
> 
> So I think we can't use it.

So you should add it and properly call the parent_unrealize from your
new unrealize function?

AFAIKS you are overwriting cpu_common_unrealizefn set in hw/core/cpu.c
for TYPE_CPU with this change.

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by Pan Nengyuan 5 years, 8 months ago


On 2/27/2020 5:04 PM, David Hildenbrand wrote:
> On 27.02.20 09:58, Pan Nengyuan wrote:
>>
>>
>> On 2/27/2020 4:41 PM, David Hildenbrand wrote:
>>> On 27.02.20 03:50, Pan Nengyuan wrote:
>>>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>>>
>>>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>>>
>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>>> ---
>>>> Cc: Richard Henderson <rth@twiddle.net>
>>>> Cc: David Hildenbrand <david@redhat.com>
>>>> Cc: Cornelia Huck <cohuck@redhat.com>
>>>> Cc: qemu-s390x@nongnu.org
>>>> ---
>>>> v2->v1:
>>>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
>>>> v3->v2:
>>>> - Also do the timer_free in unrealize, it seems more balance.
>>>> ---
>>>
>>>
>>> As I already said, I think this is init and not realize stuff. Do we
>>> have a convention now and documented that?
>>>
>>> Anyhow, I don't really care
>>> [...]
>>>
>>>
>>>> @@ -453,6 +466,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>>>  
>>>>      device_class_set_parent_realize(dc, s390_cpu_realizefn,
>>>>                                      &scc->parent_realize);
>>>> +    dc->unrealize = s390_cpu_unrealizefn;
>>>
>>> Shouldn't we use device_class_set_parent_unrealize?
>>
>> We just only declare parent_realize field in S390CPUClass(), it seems nothing to do in parent_unrealize.
>>
>> typedef struct S390CPUClass {
>> ...
>> DeviceRealize parent_realize;    // no parent_unrealize;
>> ...
>> }
>>
>> So I think we can't use it.
> 
> So you should add it and properly call the parent_unrealize from your
> new unrealize function?
> 
> AFAIKS you are overwriting cpu_common_unrealizefn set in hw/core/cpu.c
> for TYPE_CPU with this change.

Oh, I think you are right, I will change it.

Thanks.
>

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by Cornelia Huck 5 years, 8 months ago

On Thu, 27 Feb 2020 10:50:50 +0800
Pan Nengyuan <pannengyuan@huawei.com> wrote:

> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
> 
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> ---
> v2->v1:
> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-DaudÃ©)
> v3->v2:
> - Also do the timer_free in unrealize, it seems more balance.
> ---
>  target/s390x/cpu.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index cf84d307c6..cc63c9db22 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>  #if !defined(CONFIG_USER_ONLY)
>      S390CPU *cpu = S390_CPU(dev);
> +    cpu->env.tod_timer =
> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
> +    cpu->env.cpu_timer =
> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>  #endif

It does not seem you addressed the comments I had last time, namely
- memory leak on error (we do not go through unrealize if the device
  was never completely realized)
- coding style (initialization in middle of declaration section)

> +
>      Error *err = NULL;
>  
>      /* the model has to be realized before qemu_init_vcpu() due to kvm */

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by Pan Nengyuan 5 years, 8 months ago


On 2/27/2020 7:06 PM, Cornelia Huck wrote:
> On Thu, 27 Feb 2020 10:50:50 +0800
> Pan Nengyuan <pannengyuan@huawei.com> wrote:
> 
>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>
>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> ---
>> v2->v1:
>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-DaudÃ©)
>> v3->v2:
>> - Also do the timer_free in unrealize, it seems more balance.
>> ---
>>  target/s390x/cpu.c | 22 ++++++++++++++++++----
>>  1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index cf84d307c6..cc63c9db22 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>>      S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>>  #if !defined(CONFIG_USER_ONLY)
>>      S390CPU *cpu = S390_CPU(dev);
>> +    cpu->env.tod_timer =
>> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
>> +    cpu->env.cpu_timer =
>> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>>  #endif
> 
> It does not seem you addressed the comments I had last time, namely
> - memory leak on error (we do not go through unrealize if the device
>   was never completely realized)
> - coding style (initialization in middle of declaration section)

I am sorry, I misread the peter's reply and miss the codeing style too.

Apologies for you. I will change it.

Thanks.

> 
>> +
>>      Error *err = NULL;
>>  
>>      /* the model has to be realized before qemu_init_vcpu() due to kvm */
> 
> .
>

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by Stefan Hajnoczi 5 years, 8 months ago

On Thu, Feb 27, 2020 at 2:42 AM Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: qemu-s390x@nongnu.org
> ---
> v2->v1:
> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)

Hi,
This email is invalid and cannot be parsed by the patches
(https://github.com/stefanha/patches) tool that is used by some QEMU
maintainers to apply patches.

The character set is incorrectly set to "base64", which is a content
transfer encoding and not a character set:

  Content-Type: text/plain; charset="base64"
  Content-Transfer-Encoding: quoted-printable

There is a UTF-8 é character here:

  - Similarly to other cleanups, move timer_new into realize(Suggested by Phi=
  lippe Mathieu-Daud=C3=A9)

Since there is no valid charset the é character cannot be decoded.

This might be a mail server problem but it could also be due to your
git-send-email(1) configuration.

Did you set the charset to "base64" or override the content transfer
encoding?  I think other people on the list will have trouble
receiving emails like this too.

Stefan

Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize

Posted by Pan Nengyuan 5 years, 8 months ago


On 3/2/2020 10:34 PM, Stefan Hajnoczi wrote:
> On Thu, Feb 27, 2020 at 2:42 AM Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>
>> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>>
>> Direct leak of 48 byte(s) in 1 object(s) allocated from:
>>     #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>>     #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>>     #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
>>     #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
>>     #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
>>     #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
>>     #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
>>     #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
>>     #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
>>     #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
>>     #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
>>     #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
>>     #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Cornelia Huck <cohuck@redhat.com>
>> Cc: qemu-s390x@nongnu.org
>> ---
>> v2->v1:
>> - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé)
> 
> Hi,
> This email is invalid and cannot be parsed by the patches
> (https://github.com/stefanha/patches) tool that is used by some QEMU
> maintainers to apply patches.
> 
> The character set is incorrectly set to "base64", which is a content
> transfer encoding and not a character set:
> 
>   Content-Type: text/plain; charset="base64"
>   Content-Transfer-Encoding: quoted-printable
> 
> There is a UTF-8 é character here:
> 
>   - Similarly to other cleanups, move timer_new into realize(Suggested by Phi=
>   lippe Mathieu-Daud=C3=A9)
> 
> Since there is no valid charset the é character cannot be decoded.
> 
> This might be a mail server problem but it could also be due to your
> git-send-email(1) configuration.
> 
> Did you set the charset to "base64" or override the content transfer
> encoding?  I think other people on the list will have trouble
> receiving emails like this too.

Yes, it's set to "base64", I will correct it.

Thanks.

> 
> Stefan
> .
>