* Re: [PATCH v2 1/1] hw/ppc: free env->tb_env in spapr_unrealize_vcpu()
2022-03-29 12:45 ` [PATCH v2 1/1] hw/ppc: free env->tb_env in spapr_unrealize_vcpu() Daniel Henrique Barboza
@ 2022-03-29 17:32 ` Cédric Le Goater
2022-03-30 0:48 ` David Gibson
2022-04-04 6:55 ` Cédric Le Goater
2 siblings, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2022-03-29 17:32 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-ppc, david
On 3/29/22 14:45, Daniel Henrique Barboza wrote:
> 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==
>
> This patch adds a cpu_ppc_tb_free() helper in hw/ppc/ppc.c to allow us
> to free the timebase. This leak is then solved 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>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/ppc/ppc.c | 7 +++++++
> hw/ppc/spapr_cpu_core.c | 3 +++
> include/hw/ppc/ppc.h | 1 +
> 3 files changed, 11 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/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));
> }
>
> 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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/1] hw/ppc: free env->tb_env in spapr_unrealize_vcpu()
2022-03-29 12:45 ` [PATCH v2 1/1] hw/ppc: free env->tb_env in spapr_unrealize_vcpu() Daniel Henrique Barboza
2022-03-29 17:32 ` Cédric Le Goater
@ 2022-03-30 0:48 ` David Gibson
2022-04-04 6:55 ` Cédric Le Goater
2 siblings, 0 replies; 5+ messages in thread
From: David Gibson @ 2022-03-30 0:48 UTC (permalink / raw)
To: Daniel Henrique Barboza; +Cc: qemu-ppc, qemu-devel, clg
[-- Attachment #1: Type: text/plain, Size: 3659 bytes --]
On Tue, Mar 29, 2022 at 09:45:45AM -0300, Daniel Henrique Barboza wrote:
> 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==
>
> This patch adds a cpu_ppc_tb_free() helper in hw/ppc/ppc.c to allow us
> to free the timebase. This leak is then solved 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>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/ppc.c | 7 +++++++
> hw/ppc/spapr_cpu_core.c | 3 +++
> include/hw/ppc/ppc.h | 1 +
> 3 files changed, 11 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/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));
> }
>
> 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);
>
--
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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/1] hw/ppc: free env->tb_env in spapr_unrealize_vcpu()
2022-03-29 12:45 ` [PATCH v2 1/1] hw/ppc: free env->tb_env in spapr_unrealize_vcpu() Daniel Henrique Barboza
2022-03-29 17:32 ` Cédric Le Goater
2022-03-30 0:48 ` David Gibson
@ 2022-04-04 6:55 ` Cédric Le Goater
2 siblings, 0 replies; 5+ messages in thread
From: Cédric Le Goater @ 2022-04-04 6:55 UTC (permalink / raw)
To: Daniel Henrique Barboza, qemu-devel; +Cc: qemu-ppc, david
On 3/29/22 14:45, Daniel Henrique Barboza wrote:
> 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==
>
> This patch adds a cpu_ppc_tb_free() helper in hw/ppc/ppc.c to allow us
> to free the timebase. This leak is then solved 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/ppc.c | 7 +++++++
> hw/ppc/spapr_cpu_core.c | 3 +++
> include/hw/ppc/ppc.h | 1 +
> 3 files changed, 11 insertions(+)
Queued for ppc-7.0
Thanks,
C.
>
> 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/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));
> }
>
> 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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread