| Message ID | 20200217032127.46508-2-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | delay timer_new from init to realize to fix memleaks. | expand |
On 2/17/20 4:21 AM, pannengyuan@huawei.com wrote: > From: Pan Nengyuan <pannengyuan@huawei.com> > > 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: Cornelia Huck <cohuck@redhat.com> > --- > Changes v2 to v1: > - Similarly to other cleanups, move timer_new into realize, then do > timer_del in unrealize. > --- > 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..f18dbc6fe4 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,16 @@ 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); > +#endif > +} > + > static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs) > { > GuestPanicInformation *panic_info; > @@ -279,10 +294,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 > } > @@ -294,6 +305,8 @@ static void s390_cpu_finalize(Object *obj) > > qemu_unregister_reset(s390_cpu_machine_reset_cb, cpu); > g_free(cpu->irqstate); > + timer_free(cpu->env.tod_timer); > + timer_free(cpu->env.cpu_timer); > #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; > > Thanks for v2. Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On Mon, 17 Feb 2020 11:21:26 +0800 <pannengyuan@huawei.com> wrote: > From: Pan Nengyuan <pannengyuan@huawei.com> > > 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: Cornelia Huck <cohuck@redhat.com> > --- > Changes v2 to v1: > - Similarly to other cleanups, move timer_new into realize, then do > timer_del in unrealize. > --- > 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..f18dbc6fe4 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); I may be missing something, but what cleans up the timers if we fail further down in this function? I don't think the unrealize callback is invoked by the core in case of error? Also, as a matter of personal preference, I think it would be better to initialize the timers in the !CONFIG_USER_ONLY section further below, rather than in the variable declaration section. > #endif > + > Error *err = NULL; > > /* the model has to be realized before qemu_init_vcpu() due to kvm */
On Thu, 20 Feb 2020 at 16:01, Cornelia Huck <cohuck@redhat.com> wrote: > I may be missing something, but what cleans up the timers if we fail > further down in this function? I don't think the unrealize callback is > invoked by the core in case of error? FWIW I sent a mail to one of these threads a few days ago where I claimed we did call unrealize if realize fails, but looking again at the device_set_realized() code I think I was misreading it. If the device's own 'realize' method fails, we go to the 'fail' label, which just reports back the error and doesn't call unrealize. It's only if the device's 'realize' method succeeds and we get a failure in some later thing like calling 'realize' on the child bus objects that we will call the device unrealize as part of cleanup. Apologies for any confusion caused. thanks -- PMM
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index cf84d307c6..f18dbc6fe4 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,16 @@ 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); +#endif +} + static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs) { GuestPanicInformation *panic_info; @@ -279,10 +294,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 } @@ -294,6 +305,8 @@ static void s390_cpu_finalize(Object *obj) qemu_unregister_reset(s390_cpu_machine_reset_cb, cpu); g_free(cpu->irqstate); + timer_free(cpu->env.tod_timer); + timer_free(cpu->env.cpu_timer); #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;