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 <bhar...@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 <bhar...@linux.vnet.ibm.com>
> > > > Signed-off-by: Greg Kurz <gr...@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
signature.asc
Description: PGP signature
