[PATCH 0/2] ppc: fix vcpu hotunplug leak in spapr_realize_vcpu

[PATCH 0/2] ppc: fix vcpu hotunplug leak in spapr_realize_vcpu

[Daniel Henrique Barboza]

Hi,

This is a memory leak found by Valgrind when testing vcpu
hotplug/unplug in pSeries guests.

Other vcpu hotplug/unplug leaks are still present in the common code
(one in the KVM thread loop and another in cpu_address_space via
cpu->cpu_ases) but these are already being handled by Mark Kanda and
Phillipe.


Daniel Henrique Barboza (2):
  hw/ppc/ppc.c: add cpu_ppc_tb_free()
  hw/ppc: free env->tb_env in spapr_unrealize_vcpu()

 hw/ppc/ppc.c            | 7 +++++++
 hw/ppc/spapr_cpu_core.c | 3 +++
 include/hw/ppc/ppc.h    | 1 +
 3 files changed, 11 insertions(+)

-- 
2.35.1

[PATCH 1/2] hw/ppc/ppc.c: add cpu_ppc_tb_free()

[Daniel Henrique Barboza]

The timebase is created using cpu_ppc_tb_init(). At this moment we don't
have a way to free it. Add cpu_ppc_tb_free() to allow env->tb_env to be
freed.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/ppc.c         | 7 +++++++
 include/hw/ppc/ppc.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index faa02d6710..fea70df45e 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1083,6 +1083,13 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
     return &cpu_ppc_set_tb_clk;
 }
 
+void cpu_ppc_tb_free(CPUPPCState *env)
+{
+    timer_free(env->tb_env->decr_timer);
+    timer_free(env->tb_env->hdecr_timer);
+    g_free(env->tb_env);
+}
+
 /* cpu_ppc_hdecr_init may be used if the timer is not used by HDEC emulation */
 void cpu_ppc_hdecr_init(CPUPPCState *env)
 {
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index b0ba4bd6b9..364f165b4b 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -54,6 +54,7 @@ struct ppc_tb_t {
 
 uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t tb_offset);
 clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq);
+void cpu_ppc_tb_free(CPUPPCState *env);
 void cpu_ppc_hdecr_init(CPUPPCState *env);
 void cpu_ppc_hdecr_exit(CPUPPCState *env);
 
-- 
2.35.1

[PATCH 2/2] hw/ppc: free env->tb_env in spapr_unrealize_vcpu()

[Daniel Henrique Barboza]

The timebase is allocated during spapr_realize_vcpu() and it's not
freed. This results in memory leaks when doing vcpu unplugs:

==636935==
==636935== 144 (96 direct, 48 indirect) bytes in 1 blocks are definitely lost in loss record 6
,461 of 8,135
==636935==    at 0x4897468: calloc (vg_replace_malloc.c:760)
==636935==    by 0x5077213: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6400.4)
==636935==    by 0x507757F: g_malloc0_n (in /usr/lib64/libglib-2.0.so.0.6400.4)
==636935==    by 0x93C3FB: cpu_ppc_tb_init (ppc.c:1066)
==636935==    by 0x97BC2B: spapr_realize_vcpu (spapr_cpu_core.c:268)
==636935==    by 0x97C01F: spapr_cpu_core_realize (spapr_cpu_core.c:337)
==636935==    by 0xD4626F: device_set_realized (qdev.c:531)
==636935==    by 0xD55273: property_set_bool (object.c:2273)
==636935==    by 0xD523DF: object_property_set (object.c:1408)
==636935==    by 0xD588B7: object_property_set_qobject (qom-qobject.c:28)
==636935==    by 0xD52897: object_property_set_bool (object.c:1477)
==636935==    by 0xD4579B: qdev_realize (qdev.c:333)
==636935==

Fix it by calling cpu_ppc_tb_free() in spapr_unrealize_vcpu().

Fixes: 6f4b5c3ec590 ("spapr: CPU hot unplug support")
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_cpu_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index ed84713960..8a4861f45a 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -189,10 +189,13 @@ static const VMStateDescription vmstate_spapr_cpu_state = {
 
 static void spapr_unrealize_vcpu(PowerPCCPU *cpu, SpaprCpuCore *sc)
 {
+    CPUPPCState *env = &cpu->env;
+
     if (!sc->pre_3_0_migration) {
         vmstate_unregister(NULL, &vmstate_spapr_cpu_state, cpu->machine_data);
     }
     spapr_irq_cpu_intc_destroy(SPAPR_MACHINE(qdev_get_machine()), cpu);
+    cpu_ppc_tb_free(env);
     qdev_unrealize(DEVICE(cpu));
 }
 
-- 
2.35.1

Re: [PATCH 0/2] ppc: fix vcpu hotunplug leak in spapr_realize_vcpu

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

On Mon, Mar 28, 2022 at 09:59:16AM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> This is a memory leak found by Valgrind when testing vcpu
> hotplug/unplug in pSeries guests.
> 
> Other vcpu hotplug/unplug leaks are still present in the common code
> (one in the KVM thread loop and another in cpu_address_space via
> cpu->cpu_ases) but these are already being handled by Mark Kanda and
> Phillipe.

Changes LGTM, but I don't see much reason to split this into two
patches.  They're both small, and are part of the same logical change.

> 
> 
> Daniel Henrique Barboza (2):
>   hw/ppc/ppc.c: add cpu_ppc_tb_free()
>   hw/ppc: free env->tb_env in spapr_unrealize_vcpu()
> 
>  hw/ppc/ppc.c            | 7 +++++++
>  hw/ppc/spapr_cpu_core.c | 3 +++
>  include/hw/ppc/ppc.h    | 1 +
>  3 files changed, 11 insertions(+)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] ppc: fix vcpu hotunplug leak in spapr_realize_vcpu

[Cédric Le Goater]

On 3/29/22 03:24, David Gibson wrote:
> On Mon, Mar 28, 2022 at 09:59:16AM -0300, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> This is a memory leak found by Valgrind when testing vcpu
>> hotplug/unplug in pSeries guests.
>>
>> Other vcpu hotplug/unplug leaks are still present in the common code
>> (one in the KVM thread loop and another in cpu_address_space via
>> cpu->cpu_ases) but these are already being handled by Mark Kanda and
>> Phillipe.
> 
> Changes LGTM, but I don't see much reason to split this into two
> patches.  They're both small, and are part of the same logical change.

And it could be a 7.0 candidate. Are we ok with that ?

Thanks,

C.

Re: [PATCH 0/2] ppc: fix vcpu hotunplug leak in spapr_realize_vcpu

[Daniel Henrique Barboza]

On 3/29/22 05:36, Cédric Le Goater wrote:
> On 3/29/22 03:24, David Gibson wrote:
>> On Mon, Mar 28, 2022 at 09:59:16AM -0300, Daniel Henrique Barboza wrote:
>>> Hi,
>>>
>>> This is a memory leak found by Valgrind when testing vcpu
>>> hotplug/unplug in pSeries guests.
>>>
>>> Other vcpu hotplug/unplug leaks are still present in the common code
>>> (one in the KVM thread loop and another in cpu_address_space via
>>> cpu->cpu_ases) but these are already being handled by Mark Kanda and
>>> Phillipe.
>>
>> Changes LGTM, but I don't see much reason to split this into two
>> patches.  They're both small, and are part of the same logical change.


I did it in separated patches because I tried to find other instances where
the timebase would need to be freed. Didn't find any.

I am ok with squashing them in a single patch. I'll send a v2.

> 
> And it could be a 7.0 candidate. Are we ok with that ?

Since it's a memory leak we are now aware of and have a fix for, yeah, I think
it qualifies for 7.0.


Daniel

> 
> Thanks,
> 
> C.