Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between
hotplug and coldplug paths"), the CPU DT for the DRC is always allocated.
This causes a memory leak for pseries-2.6 and older machine types, that
don't support CPU hotplug and don't allocate DRCs for CPUs.
Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
hw/ppc/spapr.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 12b3f099d4c9..4a480e1c1dd9 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
CPUState *cs = CPU(core->threads);
sPAPRDRConnector *drc;
Error *local_err = NULL;
- void *fdt = NULL;
- int fdt_offset = 0;
int smt = kvmppc_smt_threads();
CPUArchId *core_slot;
int index;
@@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
g_assert(drc || !mc->has_hotpluggable_cpus);
- fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
-
if (drc) {
+ void *fdt;
+ int fdt_offset;
+
+ fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
+
spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
if (local_err) {
g_free(fdt);
On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote: > Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between > hotplug and coldplug paths"), the CPU DT for the DRC is always allocated. > This causes a memory leak for pseries-2.6 and older machine types, that > don't support CPU hotplug and don't allocate DRCs for CPUs. > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 12b3f099d4c9..4a480e1c1dd9 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > CPUState *cs = CPU(core->threads); > sPAPRDRConnector *drc; > Error *local_err = NULL; > - void *fdt = NULL; > - int fdt_offset = 0; > int smt = kvmppc_smt_threads(); > CPUArchId *core_slot; > int index; > @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > g_assert(drc || !mc->has_hotpluggable_cpus); > > - fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > - > if (drc) { > + void *fdt; > + int fdt_offset; > + > + fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > + > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > if (local_err) { > g_free(fdt); You say this in the patch description already, but want to note explicitly that this prevents double allocation for pseries-2.6 and ealier types and not for newer machine types. Regards, Bharata.
On Wed, 12 Jul 2017 15:52:37 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote: > > Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between > > hotplug and coldplug paths"), the CPU DT for the DRC is always allocated. > > This causes a memory leak for pseries-2.6 and older machine types, that > > don't support CPU hotplug and don't allocate DRCs for CPUs. > > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/spapr.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 12b3f099d4c9..4a480e1c1dd9 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > CPUState *cs = CPU(core->threads); > > sPAPRDRConnector *drc; > > Error *local_err = NULL; > > - void *fdt = NULL; > > - int fdt_offset = 0; > > int smt = kvmppc_smt_threads(); > > CPUArchId *core_slot; > > int index; > > @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > g_assert(drc || !mc->has_hotpluggable_cpus); > > > > - fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > - > > if (drc) { > > + void *fdt; > > + int fdt_offset; > > + > > + fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > + > > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > > if (local_err) { > > g_free(fdt); > > You say this in the patch description already, but want to note explicitly > that this prevents double allocation for pseries-2.6 and ealier types and > not for newer machine types. > This DT node doesn't have the exact same use as the one allocated in spapr_populate_cpus_dt_node(). Here, it is needed by the DRC logic: $ git grep 'drc->fdt' hw/ppc/spapr_drc.c hw/ppc/spapr_drc.c: if (!drc->fdt) { hw/ppc/spapr_drc.c: fdt = drc->fdt; hw/ppc/spapr_drc.c: fdt_offset = drc->fdt_start_offset; hw/ppc/spapr_drc.c: drc->fdt = fdt; hw/ppc/spapr_drc.c: drc->fdt_start_offset = fdt_start_offset; hw/ppc/spapr_drc.c: g_free(drc->fdt); hw/ppc/spapr_drc.c: drc->fdt = NULL; hw/ppc/spapr_drc.c: drc->fdt_start_offset = 0; hw/ppc/spapr_drc.c: if (!drc->fdt) { hw/ppc/spapr_drc.c: ccs->fdt_offset = drc->fdt_start_offset; hw/ppc/spapr_drc.c: tag = fdt_next_tag(drc->fdt, ccs->fdt_offset, &fdt_offset_next); hw/ppc/spapr_drc.c: name = fdt_get_name(drc->fdt, ccs->fdt_offset, NULL); hw/ppc/spapr_drc.c: prop = fdt_get_property_by_offset(drc->fdt, ccs->fdt_offset, hw/ppc/spapr_drc.c: name = fdt_string(drc->fdt, fdt32_to_cpu(prop->nameoff)); and void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt, int fdt_start_offset, Error **errp) { ... g_assert(fdt); So I'm not sure it is worth to mention the double allocation. Cheers, -- Greg > Regards, > Bharata. >
On Wed, Jul 12, 2017 at 01:11:30PM +0200, Greg Kurz wrote: > On Wed, 12 Jul 2017 15:52:37 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote: > > > Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between > > > hotplug and coldplug paths"), the CPU DT for the DRC is always allocated. > > > This causes a memory leak for pseries-2.6 and older machine types, that > > > don't support CPU hotplug and don't allocate DRCs for CPUs. > > > > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/ppc/spapr.c | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 12b3f099d4c9..4a480e1c1dd9 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > CPUState *cs = CPU(core->threads); > > > sPAPRDRConnector *drc; > > > Error *local_err = NULL; > > > - void *fdt = NULL; > > > - int fdt_offset = 0; > > > int smt = kvmppc_smt_threads(); > > > CPUArchId *core_slot; > > > int index; > > > @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > g_assert(drc || !mc->has_hotpluggable_cpus); > > > > > > - fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > > - > > > if (drc) { > > > + void *fdt; > > > + int fdt_offset; > > > + > > > + fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > > + > > > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > > > if (local_err) { > > > g_free(fdt); > > > > You say this in the patch description already, but want to note explicitly > > that this prevents double allocation for pseries-2.6 and ealier types and > > not for newer machine types. > > > > This DT node doesn't have the exact same use as the one allocated in > spapr_populate_cpus_dt_node(). Here, it is needed by the DRC logic: What I mean to say is that even after this fix, for machines types > 2.6, we still allocate memory for FDT and go through the code that creates DT entries for CPUs twice. What we create via spapr_populate_cpus_dt_node() is used while the CPU DT entries created by spapr_populate_hotplug_cpu_dt() won't be used and corresponding drc->fdt is never freed, afaics for boot time and cold-plugged CPUs. May be this can be fixed during DT code reorg effort that David is planning. Regards, Bharata.
On Thu, Jul 13, 2017 at 08:54:58AM +0530, Bharata B Rao wrote: > On Wed, Jul 12, 2017 at 01:11:30PM +0200, Greg Kurz wrote: > > On Wed, 12 Jul 2017 15:52:37 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote: > > > > Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between > > > > hotplug and coldplug paths"), the CPU DT for the DRC is always allocated. > > > > This causes a memory leak for pseries-2.6 and older machine types, that > > > > don't support CPU hotplug and don't allocate DRCs for CPUs. > > > > > > > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > hw/ppc/spapr.c | 9 +++++---- > > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > index 12b3f099d4c9..4a480e1c1dd9 100644 > > > > --- a/hw/ppc/spapr.c > > > > +++ b/hw/ppc/spapr.c > > > > @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > CPUState *cs = CPU(core->threads); > > > > sPAPRDRConnector *drc; > > > > Error *local_err = NULL; > > > > - void *fdt = NULL; > > > > - int fdt_offset = 0; > > > > int smt = kvmppc_smt_threads(); > > > > CPUArchId *core_slot; > > > > int index; > > > > @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > > > > > > > g_assert(drc || !mc->has_hotpluggable_cpus); > > > > > > > > - fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > > > - > > > > if (drc) { > > > > + void *fdt; > > > > + int fdt_offset; > > > > + > > > > + fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > > > > + > > > > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > > > > if (local_err) { > > > > g_free(fdt); > > > > > > You say this in the patch description already, but want to note explicitly > > > that this prevents double allocation for pseries-2.6 and ealier types and > > > not for newer machine types. > > > > > > > This DT node doesn't have the exact same use as the one allocated in > > spapr_populate_cpus_dt_node(). Here, it is needed by the DRC logic: > > What I mean to say is that even after this fix, for machines types > 2.6, we > still allocate memory for FDT and go through the code that creates DT entries > for CPUs twice. What we create via spapr_populate_cpus_dt_node() is used > while the CPU DT entries created by spapr_populate_hotplug_cpu_dt() won't > be used and corresponding drc->fdt is never freed, afaics for boot time > and cold-plugged CPUs. Sure, I just don't care that much. It's not a leak in the sense that it can continue to lose memory - it just means we allocate (and track) a bit more than we need to. > May be this can be fixed during DT code reorg effort that David is > planning. Maybe. -- 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
On Wed, Jul 12, 2017 at 11:48:39AM +0200, Greg Kurz wrote: > Since commit 5c1da81215c7 ("spapr: Remove unnecessary differences between > hotplug and coldplug paths"), the CPU DT for the DRC is always allocated. > This causes a memory leak for pseries-2.6 and older machine types, that > don't support CPU hotplug and don't allocate DRCs for CPUs. > > Reported-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > Signed-off-by: Greg Kurz <groug@kaod.org> Applied, thanks. > --- > hw/ppc/spapr.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 12b3f099d4c9..4a480e1c1dd9 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2993,8 +2993,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > CPUState *cs = CPU(core->threads); > sPAPRDRConnector *drc; > Error *local_err = NULL; > - void *fdt = NULL; > - int fdt_offset = 0; > int smt = kvmppc_smt_threads(); > CPUArchId *core_slot; > int index; > @@ -3009,9 +3007,12 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev, > > g_assert(drc || !mc->has_hotpluggable_cpus); > > - fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > - > if (drc) { > + void *fdt; > + int fdt_offset; > + > + fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr); > + > spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err); > if (local_err) { > g_free(fdt); > -- 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
© 2016 - 2024 Red Hat, Inc.