* [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 2:50 [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Pan Nengyuan
@ 2020-02-27 2:50 ` Pan Nengyuan
2020-02-27 8:41 ` David Hildenbrand
` (2 more replies)
2020-02-27 2:50 ` [PATCH v3 2/6] hw/arm/pxa2xx: move timer_new from init() into realize() to avoid memleaks Pan Nengyuan
` (5 subsequent siblings)
6 siblings, 3 replies; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 2:50 UTC (permalink / raw)
To: peter.maydell
Cc: zhang.zhanghailiang, David Hildenbrand, Cornelia Huck,
Pan Nengyuan, qemu-devel, qemu-s390x, qemu-arm, euler.robot,
Richard Henderson
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="base64", Size: 3822 bytes --]
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 2:50 ` [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize Pan Nengyuan
@ 2020-02-27 8:41 ` David Hildenbrand
2020-02-27 8:55 ` Philippe Mathieu-Daudé
2020-02-27 8:58 ` Pan Nengyuan
2020-02-27 11:06 ` Cornelia Huck
2020-03-02 14:34 ` Stefan Hajnoczi
2 siblings, 2 replies; 24+ messages in thread
From: David Hildenbrand @ 2020-02-27 8:41 UTC (permalink / raw)
To: Pan Nengyuan, peter.maydell
Cc: zhang.zhanghailiang, Cornelia Huck, qemu-devel, qemu-s390x,
qemu-arm, euler.robot, Richard Henderson
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 8:41 ` David Hildenbrand
@ 2020-02-27 8:55 ` Philippe Mathieu-Daudé
2020-02-27 9:06 ` David Hildenbrand
2020-02-27 8:58 ` Pan Nengyuan
1 sibling, 1 reply; 24+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-27 8:55 UTC (permalink / raw)
To: David Hildenbrand, Pan Nengyuan, peter.maydell
Cc: zhang.zhanghailiang, Cornelia Huck, qemu-devel, qemu-s390x,
qemu-arm, euler.robot, Richard Henderson
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?
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 8:55 ` Philippe Mathieu-Daudé
@ 2020-02-27 9:06 ` David Hildenbrand
0 siblings, 0 replies; 24+ messages in thread
From: David Hildenbrand @ 2020-02-27 9:06 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Pan Nengyuan, peter.maydell
Cc: zhang.zhanghailiang, Cornelia Huck, qemu-devel, qemu-s390x,
qemu-arm, euler.robot, Richard Henderson
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 8:41 ` David Hildenbrand
2020-02-27 8:55 ` Philippe Mathieu-Daudé
@ 2020-02-27 8:58 ` Pan Nengyuan
2020-02-27 9:04 ` David Hildenbrand
1 sibling, 1 reply; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 8:58 UTC (permalink / raw)
To: David Hildenbrand, peter.maydell
Cc: zhang.zhanghailiang, Cornelia Huck, qemu-devel, qemu-s390x,
qemu-arm, euler.robot, Richard Henderson
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.
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 8:58 ` Pan Nengyuan
@ 2020-02-27 9:04 ` David Hildenbrand
2020-02-27 9:15 ` Pan Nengyuan
0 siblings, 1 reply; 24+ messages in thread
From: David Hildenbrand @ 2020-02-27 9:04 UTC (permalink / raw)
To: Pan Nengyuan, peter.maydell
Cc: zhang.zhanghailiang, Cornelia Huck, qemu-devel, qemu-s390x,
qemu-arm, euler.robot, Richard Henderson
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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 9:04 ` David Hildenbrand
@ 2020-02-27 9:15 ` Pan Nengyuan
0 siblings, 0 replies; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 9:15 UTC (permalink / raw)
To: David Hildenbrand, peter.maydell
Cc: zhang.zhanghailiang, Cornelia Huck, qemu-devel, qemu-s390x,
qemu-arm, euler.robot, Richard Henderson
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.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 2:50 ` [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize Pan Nengyuan
2020-02-27 8:41 ` David Hildenbrand
@ 2020-02-27 11:06 ` Cornelia Huck
2020-02-27 11:25 ` Pan Nengyuan
2020-03-02 14:34 ` Stefan Hajnoczi
2 siblings, 1 reply; 24+ messages in thread
From: Cornelia Huck @ 2020-02-27 11:06 UTC (permalink / raw)
To: Pan Nengyuan
Cc: peter.maydell, zhang.zhanghailiang, David Hildenbrand,
qemu-devel, qemu-s390x, qemu-arm, euler.robot, Richard Henderson
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 */
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 11:06 ` Cornelia Huck
@ 2020-02-27 11:25 ` Pan Nengyuan
0 siblings, 0 replies; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 11:25 UTC (permalink / raw)
To: Cornelia Huck
Cc: peter.maydell, zhang.zhanghailiang, David Hildenbrand,
qemu-devel, qemu-s390x, qemu-arm, euler.robot, Richard Henderson
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 */
>
> .
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-02-27 2:50 ` [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize Pan Nengyuan
2020-02-27 8:41 ` David Hildenbrand
2020-02-27 11:06 ` Cornelia Huck
@ 2020-03-02 14:34 ` Stefan Hajnoczi
2020-03-03 1:39 ` Pan Nengyuan
2 siblings, 1 reply; 24+ messages in thread
From: Stefan Hajnoczi @ 2020-03-02 14:34 UTC (permalink / raw)
To: Pan Nengyuan
Cc: Peter Maydell, Zhanghailiang, David Hildenbrand, Cornelia Huck,
qemu-devel, qemu-s390x, qemu-arm, Euler Robot, Richard Henderson
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize
2020-03-02 14:34 ` Stefan Hajnoczi
@ 2020-03-03 1:39 ` Pan Nengyuan
0 siblings, 0 replies; 24+ messages in thread
From: Pan Nengyuan @ 2020-03-03 1:39 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Peter Maydell, Zhanghailiang, David Hildenbrand, Cornelia Huck,
qemu-devel, qemu-s390x, qemu-arm, Euler Robot, Richard Henderson
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
> .
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 2/6] hw/arm/pxa2xx: move timer_new from init() into realize() to avoid memleaks
2020-02-27 2:50 [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Pan Nengyuan
2020-02-27 2:50 ` [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize Pan Nengyuan
@ 2020-02-27 2:50 ` Pan Nengyuan
2020-02-27 2:50 ` [PATCH v3 3/6] hw/arm/spitz: " Pan Nengyuan
` (4 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 2:50 UTC (permalink / raw)
To: peter.maydell
Cc: zhang.zhanghailiang, Pan Nengyuan, qemu-devel, qemu-arm, euler.robot
There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Andrzej Zaborowski <balrogg@gmail.com>
---
hw/arm/pxa2xx.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index b33f8f1351..56a36202d7 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1134,18 +1134,22 @@ static void pxa2xx_rtc_init(Object *obj)
s->last_rtcpicr = 0;
s->last_hz = s->last_sw = s->last_pi = qemu_clock_get_ms(rtc_clock);
+ sysbus_init_irq(dev, &s->rtc_irq);
+
+ memory_region_init_io(&s->iomem, obj, &pxa2xx_rtc_ops, s,
+ "pxa2xx-rtc", 0x10000);
+ sysbus_init_mmio(dev, &s->iomem);
+}
+
+static void pxa2xx_rtc_realize(DeviceState *dev, Error **errp)
+{
+ PXA2xxRTCState *s = PXA2XX_RTC(dev);
s->rtc_hz = timer_new_ms(rtc_clock, pxa2xx_rtc_hz_tick, s);
s->rtc_rdal1 = timer_new_ms(rtc_clock, pxa2xx_rtc_rdal1_tick, s);
s->rtc_rdal2 = timer_new_ms(rtc_clock, pxa2xx_rtc_rdal2_tick, s);
s->rtc_swal1 = timer_new_ms(rtc_clock, pxa2xx_rtc_swal1_tick, s);
s->rtc_swal2 = timer_new_ms(rtc_clock, pxa2xx_rtc_swal2_tick, s);
s->rtc_pi = timer_new_ms(rtc_clock, pxa2xx_rtc_pi_tick, s);
-
- sysbus_init_irq(dev, &s->rtc_irq);
-
- memory_region_init_io(&s->iomem, obj, &pxa2xx_rtc_ops, s,
- "pxa2xx-rtc", 0x10000);
- sysbus_init_mmio(dev, &s->iomem);
}
static int pxa2xx_rtc_pre_save(void *opaque)
@@ -1203,6 +1207,7 @@ static void pxa2xx_rtc_sysbus_class_init(ObjectClass *klass, void *data)
dc->desc = "PXA2xx RTC Controller";
dc->vmsd = &vmstate_pxa2xx_rtc_regs;
+ dc->realize = pxa2xx_rtc_realize;
}
static const TypeInfo pxa2xx_rtc_sysbus_info = {
--
2.18.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 3/6] hw/arm/spitz: move timer_new from init() into realize() to avoid memleaks
2020-02-27 2:50 [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Pan Nengyuan
2020-02-27 2:50 ` [PATCH v3 1/6] s390x: fix memleaks in cpu_finalize Pan Nengyuan
2020-02-27 2:50 ` [PATCH v3 2/6] hw/arm/pxa2xx: move timer_new from init() into realize() to avoid memleaks Pan Nengyuan
@ 2020-02-27 2:50 ` Pan Nengyuan
2020-02-27 2:50 ` [PATCH v3 4/6] hw/arm/strongarm: " Pan Nengyuan
` (3 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 2:50 UTC (permalink / raw)
To: peter.maydell
Cc: zhang.zhanghailiang, Pan Nengyuan, qemu-devel, qemu-arm, euler.robot
There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Andrzej Zaborowski <balrogg@gmail.com>
---
hw/arm/spitz.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/arm/spitz.c b/hw/arm/spitz.c
index e001088103..cbfa6934cf 100644
--- a/hw/arm/spitz.c
+++ b/hw/arm/spitz.c
@@ -524,11 +524,16 @@ static void spitz_keyboard_init(Object *obj)
spitz_keyboard_pre_map(s);
- s->kbdtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, spitz_keyboard_tick, s);
qdev_init_gpio_in(dev, spitz_keyboard_strobe, SPITZ_KEY_STROBE_NUM);
qdev_init_gpio_out(dev, s->sense, SPITZ_KEY_SENSE_NUM);
}
+static void spitz_keyboard_realize(DeviceState *dev, Error **errp)
+{
+ SpitzKeyboardState *s = SPITZ_KEYBOARD(dev);
+ s->kbdtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL, spitz_keyboard_tick, s);
+}
+
/* LCD backlight controller */
#define LCDTG_RESCTL 0x00
@@ -1115,6 +1120,7 @@ static void spitz_keyboard_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->vmsd = &vmstate_spitz_kbd;
+ dc->realize = spitz_keyboard_realize;
}
static const TypeInfo spitz_keyboard_info = {
--
2.18.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 4/6] hw/arm/strongarm: move timer_new from init() into realize() to avoid memleaks
2020-02-27 2:50 [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Pan Nengyuan
` (2 preceding siblings ...)
2020-02-27 2:50 ` [PATCH v3 3/6] hw/arm/spitz: " Pan Nengyuan
@ 2020-02-27 2:50 ` Pan Nengyuan
2020-02-27 2:50 ` [PATCH v3 5/6] hw/misc/mos6522: " Pan Nengyuan
` (2 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 2:50 UTC (permalink / raw)
To: peter.maydell
Cc: euler.robot, qemu-arm, Pan Nengyuan, qemu-devel, zhang.zhanghailiang
There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
hw/arm/strongarm.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index cd8a99aaf2..3010d765bb 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -399,9 +399,6 @@ static void strongarm_rtc_init(Object *obj)
s->last_rcnr = (uint32_t) mktimegm(&tm);
s->last_hz = qemu_clock_get_ms(rtc_clock);
- s->rtc_alarm = timer_new_ms(rtc_clock, strongarm_rtc_alarm_tick, s);
- s->rtc_hz = timer_new_ms(rtc_clock, strongarm_rtc_hz_tick, s);
-
sysbus_init_irq(dev, &s->rtc_irq);
sysbus_init_irq(dev, &s->rtc_hz_irq);
@@ -410,6 +407,13 @@ static void strongarm_rtc_init(Object *obj)
sysbus_init_mmio(dev, &s->iomem);
}
+static void strongarm_rtc_realize(DeviceState *dev, Error **errp)
+{
+ StrongARMRTCState *s = STRONGARM_RTC(dev);
+ s->rtc_alarm = timer_new_ms(rtc_clock, strongarm_rtc_alarm_tick, s);
+ s->rtc_hz = timer_new_ms(rtc_clock, strongarm_rtc_hz_tick, s);
+}
+
static int strongarm_rtc_pre_save(void *opaque)
{
StrongARMRTCState *s = opaque;
@@ -451,6 +455,7 @@ static void strongarm_rtc_sysbus_class_init(ObjectClass *klass, void *data)
dc->desc = "StrongARM RTC Controller";
dc->vmsd = &vmstate_strongarm_rtc_regs;
+ dc->realize = strongarm_rtc_realize;
}
static const TypeInfo strongarm_rtc_sysbus_info = {
@@ -1240,15 +1245,16 @@ static void strongarm_uart_init(Object *obj)
"uart", 0x10000);
sysbus_init_mmio(dev, &s->iomem);
sysbus_init_irq(dev, &s->irq);
-
- s->rx_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_rx_to, s);
- s->tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_tx, s);
}
static void strongarm_uart_realize(DeviceState *dev, Error **errp)
{
StrongARMUARTState *s = STRONGARM_UART(dev);
+ s->rx_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ strongarm_uart_rx_to,
+ s);
+ s->tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, strongarm_uart_tx, s);
qemu_chr_fe_set_handlers(&s->chr,
strongarm_uart_can_receive,
strongarm_uart_receive,
--
2.18.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
2020-02-27 2:50 [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Pan Nengyuan
` (3 preceding siblings ...)
2020-02-27 2:50 ` [PATCH v3 4/6] hw/arm/strongarm: " Pan Nengyuan
@ 2020-02-27 2:50 ` Pan Nengyuan
2020-03-02 13:21 ` Peter Maydell
2020-02-27 2:50 ` [PATCH v3 6/6] hw/timer/cadence_ttc: " Pan Nengyuan
2020-03-02 13:21 ` [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Peter Maydell
6 siblings, 1 reply; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 2:50 UTC (permalink / raw)
To: peter.maydell
Cc: zhang.zhanghailiang, Pan Nengyuan, Laurent Vivier, qemu-devel,
qemu-arm, euler.robot
There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: Laurent Vivier <laurent@vivier.eu>
---
v2->v1:
- no changes in this patch.
v3->v2:
- remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
---
hw/misc/mac_via.c | 5 +++++
hw/misc/mos6522.c | 6 ++++++
2 files changed, 11 insertions(+)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b7d0012794..1d72d4ef35 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
+ object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
+ &error_abort);
+ object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
+ &error_abort);
+
/* Pass through mos6522 output IRQs */
ms = MOS6522(&m->mos6522_via1);
object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 19e154b870..c1cd154a84 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
s->timers[i].index = i;
}
+}
+
+static void mos6522_realize(DeviceState *dev, Error **errp)
+{
+ MOS6522State *s = MOS6522(dev);
s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
@@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
dc->reset = mos6522_reset;
dc->vmsd = &vmstate_mos6522;
+ dc->realize = mos6522_realize;
device_class_set_props(dc, mos6522_properties);
mdc->parent_reset = dc->reset;
mdc->set_sr_int = mos6522_set_sr_int;
--
2.18.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
2020-02-27 2:50 ` [PATCH v3 5/6] hw/misc/mos6522: " Pan Nengyuan
@ 2020-03-02 13:21 ` Peter Maydell
2020-03-02 19:17 ` Mark Cave-Ayland
0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2020-03-02 13:21 UTC (permalink / raw)
To: Pan Nengyuan
Cc: Euler Robot, qemu-arm, Laurent Vivier, QEMU Developers, zhanghailiang
On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> ---
> Cc: Laurent Vivier <laurent@vivier.eu>
> ---
> v2->v1:
> - no changes in this patch.
> v3->v2:
> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
Hi; this is really fixing two bugs in one patch:
> ---
> hw/misc/mac_via.c | 5 +++++
> hw/misc/mos6522.c | 6 ++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..1d72d4ef35 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
> sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
> sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>
> + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
> + &error_abort);
> + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
> + &error_abort);
> +
> /* Pass through mos6522 output IRQs */
> ms = MOS6522(&m->mos6522_via1);
> object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
This is fixing a bug in mac_via where it failed to actually
realize devices it was using. That's a dependency for the bug
you're trying to fix, but it's a separate one and should be
in its own patch.
You also want to pass the error back up to the caller, rather
than using error_abort. To do that, at the top of the function:
Error *err = NULL;
and then here you can do:
object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err);
if (err) {
error_propagate(errp, err);
return;
}
The existing code which inits the mos6522 objects and
calls object_property_add_alias on them which is in
the mac_via realize function should be moved to the init
function. (init should init child objects and set up
properties; realize should realize them.)
This is all fixing the incorrect creation of the mos6522
devices in mac_via (which has been wrong since the mac_via
was first added in commit 6dca62a0000f9), so that can all
be one patch I think.
> +static void mos6522_realize(DeviceState *dev, Error **errp)
> +{
> + MOS6522State *s = MOS6522(dev);
>
> s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
> s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>
> dc->reset = mos6522_reset;
> dc->vmsd = &vmstate_mos6522;
> + dc->realize = mos6522_realize;
> device_class_set_props(dc, mos6522_properties);
> mdc->parent_reset = dc->reset;
> mdc->set_sr_int = mos6522_set_sr_int;
The changes to hw/misc/mos_6522.c are good, but should be in
their own patch.
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
2020-03-02 13:21 ` Peter Maydell
@ 2020-03-02 19:17 ` Mark Cave-Ayland
2020-03-03 1:36 ` Pan Nengyuan
0 siblings, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2020-03-02 19:17 UTC (permalink / raw)
To: Peter Maydell, Pan Nengyuan
Cc: QEMU Developers, zhanghailiang, qemu-arm, Laurent Vivier, Euler Robot
On 02/03/2020 13:21, Peter Maydell wrote:
> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>
>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>> ---
>> Cc: Laurent Vivier <laurent@vivier.eu>
>> ---
>> v2->v1:
>> - no changes in this patch.
>> v3->v2:
>> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>
> Hi; this is really fixing two bugs in one patch:
>
>> ---
>> hw/misc/mac_via.c | 5 +++++
>> hw/misc/mos6522.c | 6 ++++++
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index b7d0012794..1d72d4ef35 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>> sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>> sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>
>> + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
>> + &error_abort);
>> + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
>> + &error_abort);
>> +
>> /* Pass through mos6522 output IRQs */
>> ms = MOS6522(&m->mos6522_via1);
>> object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>
> This is fixing a bug in mac_via where it failed to actually
> realize devices it was using. That's a dependency for the bug
> you're trying to fix, but it's a separate one and should be
> in its own patch.
Sigh. Thanks for this - I actually discovered this a little while back and have some
local patches to do the same, but due to lack of time I never managed to tidy them up
for submission.
> You also want to pass the error back up to the caller, rather
> than using error_abort. To do that, at the top of the function:
>
> Error *err = NULL;
>
> and then here you can do:
> object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err);
> if (err) {
> error_propagate(errp, err);
> return;
> }
>
> The existing code which inits the mos6522 objects and
> calls object_property_add_alias on them which is in
> the mac_via realize function should be moved to the init
> function. (init should init child objects and set up
> properties; realize should realize them.)
And I believe at some point I had a patch lying around to do this too...
> This is all fixing the incorrect creation of the mos6522
> devices in mac_via (which has been wrong since the mac_via
> was first added in commit 6dca62a0000f9), so that can all
> be one patch I think.
>
>> +static void mos6522_realize(DeviceState *dev, Error **errp)
>> +{
>> + MOS6522State *s = MOS6522(dev);
>>
>> s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
>> s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
>> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>>
>> dc->reset = mos6522_reset;
>> dc->vmsd = &vmstate_mos6522;
>> + dc->realize = mos6522_realize;
>> device_class_set_props(dc, mos6522_properties);
>> mdc->parent_reset = dc->reset;
>> mdc->set_sr_int = mos6522_set_sr_int;
>
> The changes to hw/misc/mos_6522.c are good, but should be in
> their own patch.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
2020-03-02 19:17 ` Mark Cave-Ayland
@ 2020-03-03 1:36 ` Pan Nengyuan
2020-03-04 20:40 ` Mark Cave-Ayland
0 siblings, 1 reply; 24+ messages in thread
From: Pan Nengyuan @ 2020-03-03 1:36 UTC (permalink / raw)
To: Mark Cave-Ayland, Peter Maydell
Cc: QEMU Developers, zhanghailiang, qemu-arm, Laurent Vivier, Euler Robot
On 3/3/2020 3:17 AM, Mark Cave-Ayland wrote:
> On 02/03/2020 13:21, Peter Maydell wrote:
>
>> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>>
>>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>>> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>> ---
>>> Cc: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>> v2->v1:
>>> - no changes in this patch.
>>> v3->v2:
>>> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>
>> Hi; this is really fixing two bugs in one patch:
>>
>>> ---
>>> hw/misc/mac_via.c | 5 +++++
>>> hw/misc/mos6522.c | 6 ++++++
>>> 2 files changed, 11 insertions(+)
>>>
>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>> index b7d0012794..1d72d4ef35 100644
>>> --- a/hw/misc/mac_via.c
>>> +++ b/hw/misc/mac_via.c
>>> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>>> sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>>> sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>>
>>> + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
>>> + &error_abort);
>>> + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
>>> + &error_abort);
>>> +
>>> /* Pass through mos6522 output IRQs */
>>> ms = MOS6522(&m->mos6522_via1);
>>> object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>>
>> This is fixing a bug in mac_via where it failed to actually
>> realize devices it was using. That's a dependency for the bug
>> you're trying to fix, but it's a separate one and should be
>> in its own patch.
>
> Sigh. Thanks for this - I actually discovered this a little while back and have some
> local patches to do the same, but due to lack of time I never managed to tidy them up
> for submission.
Hmm, maybe you can take this other changes(fix memleaks) into your local patches and send it together?
Or If you have no time, I can help to do it about this device. :)
>
>> You also want to pass the error back up to the caller, rather
>> than using error_abort. To do that, at the top of the function:
>>
>> Error *err = NULL;
>>
>> and then here you can do:
>> object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err);
>> if (err) {
>> error_propagate(errp, err);
>> return;
>> }
>>
>> The existing code which inits the mos6522 objects and
>> calls object_property_add_alias on them which is in
>> the mac_via realize function should be moved to the init
>> function. (init should init child objects and set up
>> properties; realize should realize them.)
>
> And I believe at some point I had a patch lying around to do this too...
>
>> This is all fixing the incorrect creation of the mos6522
>> devices in mac_via (which has been wrong since the mac_via
>> was first added in commit 6dca62a0000f9), so that can all
>> be one patch I think.
>>
>>> +static void mos6522_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + MOS6522State *s = MOS6522(dev);
>>>
>>> s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
>>> s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
>>> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
>>>
>>> dc->reset = mos6522_reset;
>>> dc->vmsd = &vmstate_mos6522;
>>> + dc->realize = mos6522_realize;
>>> device_class_set_props(dc, mos6522_properties);
>>> mdc->parent_reset = dc->reset;
>>> mdc->set_sr_int = mos6522_set_sr_int;
>>
>> The changes to hw/misc/mos_6522.c are good, but should be in
>> their own patch.
>
>
> ATB,
>
> Mark.
> .
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 5/6] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
2020-03-03 1:36 ` Pan Nengyuan
@ 2020-03-04 20:40 ` Mark Cave-Ayland
0 siblings, 0 replies; 24+ messages in thread
From: Mark Cave-Ayland @ 2020-03-04 20:40 UTC (permalink / raw)
To: Pan Nengyuan, Peter Maydell
Cc: Laurent Vivier, Euler Robot, qemu-arm, QEMU Developers, zhanghailiang
On 03/03/2020 01:36, Pan Nengyuan wrote:
> On 3/3/2020 3:17 AM, Mark Cave-Ayland wrote:
>> On 02/03/2020 13:21, Peter Maydell wrote:
>>
>>> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>>>
>>>> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>>>> Meanwhile, add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>>>
>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
>>>> ---
>>>> Cc: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>> v2->v1:
>>>> - no changes in this patch.
>>>> v3->v2:
>>>> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>>>
>>> Hi; this is really fixing two bugs in one patch:
>>>
>>>> ---
>>>> hw/misc/mac_via.c | 5 +++++
>>>> hw/misc/mos6522.c | 6 ++++++
>>>> 2 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>> index b7d0012794..1d72d4ef35 100644
>>>> --- a/hw/misc/mac_via.c
>>>> +++ b/hw/misc/mac_via.c
>>>> @@ -879,6 +879,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>>>> sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2,
>>>> sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>>>
>>>> + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized",
>>>> + &error_abort);
>>>> + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized",
>>>> + &error_abort);
>>>> +
>>>> /* Pass through mos6522 output IRQs */
>>>> ms = MOS6522(&m->mos6522_via1);
>>>> object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>>>
>>> This is fixing a bug in mac_via where it failed to actually
>>> realize devices it was using. That's a dependency for the bug
>>> you're trying to fix, but it's a separate one and should be
>>> in its own patch.
>>
>> Sigh. Thanks for this - I actually discovered this a little while back and have some
>> local patches to do the same, but due to lack of time I never managed to tidy them up
>> for submission.
>
> Hmm, maybe you can take this other changes(fix memleaks) into your local patches and send it together?
> Or If you have no time, I can help to do it about this device. :)
My patches are part of various q800 branches I have been playing with over the past
couple of months, and so are lagging quite far behind master. If you are able to
update them based upon Peter's comments them I'm happy to review them (and perhaps
could perhaps take them along with my cmd646 patchset if required).
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v3 6/6] hw/timer/cadence_ttc: move timer_new from init() into realize() to avoid memleaks
2020-02-27 2:50 [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Pan Nengyuan
` (4 preceding siblings ...)
2020-02-27 2:50 ` [PATCH v3 5/6] hw/misc/mos6522: " Pan Nengyuan
@ 2020-02-27 2:50 ` Pan Nengyuan
2020-02-27 6:33 ` Alistair Francis
2020-03-02 13:21 ` [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Peter Maydell
6 siblings, 1 reply; 24+ messages in thread
From: Pan Nengyuan @ 2020-02-27 2:50 UTC (permalink / raw)
To: peter.maydell
Cc: zhang.zhanghailiang, Alistair Francis, Pan Nengyuan, qemu-devel,
qemu-arm, euler.robot, Edgar E. Iglesias
There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
---
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Alistair Francis <alistair@alistair23.me>
---
hw/timer/cadence_ttc.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/hw/timer/cadence_ttc.c b/hw/timer/cadence_ttc.c
index 5e3128c1e3..b0ba6b2bba 100644
--- a/hw/timer/cadence_ttc.c
+++ b/hw/timer/cadence_ttc.c
@@ -412,16 +412,21 @@ static void cadence_timer_init(uint32_t freq, CadenceTimerState *s)
static void cadence_ttc_init(Object *obj)
{
CadenceTTCState *s = CADENCE_TTC(obj);
+
+ memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
+ "timer", 0x1000);
+ sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
+}
+
+static void cadence_ttc_realize(DeviceState *dev, Error **errp)
+{
+ CadenceTTCState *s = CADENCE_TTC(dev);
int i;
for (i = 0; i < 3; ++i) {
cadence_timer_init(133000000, &s->timer[i]);
- sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->timer[i].irq);
+ sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->timer[i].irq);
}
-
- memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
- "timer", 0x1000);
- sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
}
static int cadence_timer_pre_save(void *opaque)
@@ -479,6 +484,7 @@ static void cadence_ttc_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
dc->vmsd = &vmstate_cadence_ttc;
+ dc->realize = cadence_ttc_realize;
}
static const TypeInfo cadence_ttc_info = {
--
2.18.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v3 6/6] hw/timer/cadence_ttc: move timer_new from init() into realize() to avoid memleaks
2020-02-27 2:50 ` [PATCH v3 6/6] hw/timer/cadence_ttc: " Pan Nengyuan
@ 2020-02-27 6:33 ` Alistair Francis
0 siblings, 0 replies; 24+ messages in thread
From: Alistair Francis @ 2020-02-27 6:33 UTC (permalink / raw)
To: Pan Nengyuan
Cc: Peter Maydell, zhang.zhanghailiang, Alistair Francis,
qemu-devel@nongnu.org Developers, qemu-arm, Euler Robot,
Edgar E. Iglesias
On Wed, Feb 26, 2020 at 6:37 PM Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> ---
> hw/timer/cadence_ttc.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/timer/cadence_ttc.c b/hw/timer/cadence_ttc.c
> index 5e3128c1e3..b0ba6b2bba 100644
> --- a/hw/timer/cadence_ttc.c
> +++ b/hw/timer/cadence_ttc.c
> @@ -412,16 +412,21 @@ static void cadence_timer_init(uint32_t freq, CadenceTimerState *s)
> static void cadence_ttc_init(Object *obj)
> {
> CadenceTTCState *s = CADENCE_TTC(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
> + "timer", 0x1000);
> + sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +}
> +
> +static void cadence_ttc_realize(DeviceState *dev, Error **errp)
> +{
> + CadenceTTCState *s = CADENCE_TTC(dev);
> int i;
>
> for (i = 0; i < 3; ++i) {
> cadence_timer_init(133000000, &s->timer[i]);
> - sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->timer[i].irq);
> + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->timer[i].irq);
> }
> -
> - memory_region_init_io(&s->iomem, obj, &cadence_ttc_ops, s,
> - "timer", 0x1000);
> - sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> }
>
> static int cadence_timer_pre_save(void *opaque)
> @@ -479,6 +484,7 @@ static void cadence_ttc_class_init(ObjectClass *klass, void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->vmsd = &vmstate_cadence_ttc;
> + dc->realize = cadence_ttc_realize;
> }
>
> static const TypeInfo cadence_ttc_info = {
> --
> 2.18.2
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks.
2020-02-27 2:50 [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Pan Nengyuan
` (5 preceding siblings ...)
2020-02-27 2:50 ` [PATCH v3 6/6] hw/timer/cadence_ttc: " Pan Nengyuan
@ 2020-03-02 13:21 ` Peter Maydell
2020-03-03 1:26 ` Pan Nengyuan
6 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2020-03-02 13:21 UTC (permalink / raw)
To: Pan Nengyuan; +Cc: Euler Robot, qemu-arm, QEMU Developers, zhanghailiang
On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>
> This series delay timer_new from init into realize to avoid memleaks when we call 'device_list_properties'.
> And do timer_free only in s390x_cpu_finalize because it's hotplugable. However, It's not valid in mos6522
> if we move timer_new from init to realize, because it's never called at all. So we also add calls to mos6522_realize()
> in mac_via_realize to make this move to be valid.
>
> v1:
> - Delay timer_new() from init() to realize() to fix memleaks.
> v2:
> - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé).
> - Send these two patches as a series instead of send each as a single patch but with wrong subject in v1.
> v3:
> - It's not valid in mos6522 if we move timer_new from init to realize, because it's never called at all.
> Thus, we remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
> - split patch by device to make it more clear.
Hi; I've applied patches 2, 3, 4 and 6 to target-arm.next,
since I think those ones are OK and they're all arm related.
You've already got review comment for patch 1 (s390)
and 5 (m68k mac_via/mos6522).
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks.
2020-03-02 13:21 ` [PATCH v3 0/6] delay timer_new from init to realize to fix memleaks Peter Maydell
@ 2020-03-03 1:26 ` Pan Nengyuan
0 siblings, 0 replies; 24+ messages in thread
From: Pan Nengyuan @ 2020-03-03 1:26 UTC (permalink / raw)
To: Peter Maydell; +Cc: Euler Robot, qemu-arm, QEMU Developers, zhanghailiang
On 3/2/2020 9:21 PM, Peter Maydell wrote:
> On Thu, 27 Feb 2020 at 02:35, Pan Nengyuan <pannengyuan@huawei.com> wrote:
>>
>> This series delay timer_new from init into realize to avoid memleaks when we call 'device_list_properties'.
>> And do timer_free only in s390x_cpu_finalize because it's hotplugable. However, It's not valid in mos6522
>> if we move timer_new from init to realize, because it's never called at all. So we also add calls to mos6522_realize()
>> in mac_via_realize to make this move to be valid.
>>
>> v1:
>> - Delay timer_new() from init() to realize() to fix memleaks.
>> v2:
>> - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé).
>> - Send these two patches as a series instead of send each as a single patch but with wrong subject in v1.
>> v3:
>> - It's not valid in mos6522 if we move timer_new from init to realize, because it's never called at all.
>> Thus, we remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid.
>> - split patch by device to make it more clear.
>
> Hi; I've applied patches 2, 3, 4 and 6 to target-arm.next,
> since I think those ones are OK and they're all arm related.
>
> You've already got review comment for patch 1 (s390)
> and 5 (m68k mac_via/mos6522).
Fine, thanks.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 24+ messages in thread