| Message ID | 20200227025055.14341-1-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | delay timer_new from init to realize to fix memleaks. | expand |
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?
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? > >
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. > >
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.
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.
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. >
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 */
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 */ > > . >
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
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
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 >
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 > . >
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. Pan Nengyuan (6): s390x: fix memleaks in cpu_finalize hw/arm/pxa2xx: move timer_new from init() into realize() to avoid memleaks hw/arm/spitz: move timer_new from init() into realize() to avoid memleaks hw/arm/strongarm: move timer_new from init() into realize() to avoid memleaks hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks hw/timer/cadence_ttc: move timer_new from init() into realize() to avoid memleaks hw/arm/pxa2xx.c | 17 +++++++++++------ hw/arm/spitz.c | 8 +++++++- hw/arm/strongarm.c | 18 ++++++++++++------ hw/misc/mac_via.c | 5 +++++ hw/misc/mos6522.c | 6 ++++++ hw/timer/cadence_ttc.c | 16 +++++++++++----- target/s390x/cpu.c | 22 ++++++++++++++++++---- 7 files changed, 70 insertions(+), 22 deletions(-)