Re: [PATCH v2 1/2] s390x: fix memleaks in cpu_finalize

[Peter Maydell]

On Thu, 20 Feb 2020 at 16:01, Cornelia Huck  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

Re: [PATCH v2 1/2] s390x: fix memleaks in cpu_finalize

[Cornelia Huck]

On Mon, 17 Feb 2020 11:21:26 +0800
 wrote:

> From: Pan Nengyuan 
> 
> 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 
> Signed-off-by: Pan Nengyuan 
> Cc: Richard Henderson 
> Cc: Cornelia Huck 
> ---
> 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 */

Re: [PATCH v2 1/2] s390x: fix memleaks in cpu_finalize

[Philippe Mathieu-Daudé]

On 2/17/20 4:21 AM, pannengy...@huawei.com wrote:

From: Pan Nengyuan 

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 
Signed-off-by: Pan Nengyuan 
Cc: Richard Henderson 
Cc: Cornelia Huck 
---
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é 

[PATCH v2 1/2] s390x: fix memleaks in cpu_finalize

[pannengyuan]

From: Pan Nengyuan 

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 
Signed-off-by: Pan Nengyuan 
Cc: Richard Henderson 
Cc: Cornelia Huck 
---
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;
 
-- 
2.21.0.windows.1