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:
$ 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.
>
pgpsFGHDbxV1K.pgp
Description: OpenPGP digital signature
