| Message ID | 20220329124545.529145-2-danielhb413@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix vcpu hotunplug leak in spapr_realize_vcpu | expand |
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); >
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); >
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); >
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);
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(+)