RE: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci

From: gaojinhao <gaojinhao@huawei.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: "Michael S . Tsirkin" <mst@redhat.com>,
	"Jason Wang" <jasowang@redhat.com>,
	"Stefan Berger" <stefanb@linux.vnet.ibm.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"Greg Kurz" <groug@kaod.org>,
	"Juan Quintela" <quintela@redhat.com>,
	"qemu-ppc@nongnu.org" <qemu-ppc@nongnu.org>,
	"Wanghaibin (D)" <wanghaibin.wang@huawei.com>,
	"Marc-André Lureau" <marcandre.lureau@redhat.com>,
	zhukeqian <zhukeqian1@huawei.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: RE: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci
Date: Mon, 28 Dec 2020 09:31:34 +0000	[thread overview]
Message-ID: <6416730c88794a97b48ea03b2ea9416c@huawei.com> (raw)
In-Reply-To: <20201228083013.GM6952@yekko.fritz.box>

Thank you for you reply, I understand.

Jinhao Gao

-----Original Message-----
From: David Gibson [mailto:david@gibson.dropbear.id.au] 
Sent: 2020年12月28日 16:30
To: gaojinhao <gaojinhao@huawei.com>
Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; Marc-André Lureau <marcandre.lureau@redhat.com>; Stefan Berger <stefanb@linux.vnet.ibm.com>; Michael S . Tsirkin <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Greg Kurz <groug@kaod.org>; Juan Quintela <quintela@redhat.com>; Dr . David Alan Gilbert <dgilbert@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; zhukeqian <zhukeqian1@huawei.com>
Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of vmstate_spapr_pci

On Mon, Dec 28, 2020 at 08:10:31AM +0000, gaojinhao wrote:
> Hi David,
> Firstly, thank you for you review. And then for your review, I worry 
> that a memory leak will occur if QEMU exits after saves vmsd. So, we 
> free it in post_save func.

If qemu exits, all its memory will be freed, so we don't care.

> 
> Jinhao Gao
> 
> -----Original Message-----
> From: David Gibson [mailto:david@gibson.dropbear.id.au]
> Sent: 2020-12-28 14:58
> To: gaojinhao <gaojinhao@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; Marc-André Lureau 
> <marcandre.lureau@redhat.com>; Stefan Berger 
> <stefanb@linux.vnet.ibm.com>; Michael S . Tsirkin <mst@redhat.com>; 
> Jason Wang <jasowang@redhat.com>; Greg Kurz <groug@kaod.org>; Juan 
> Quintela <quintela@redhat.com>; Dr . David Alan Gilbert 
> <dgilbert@redhat.com>; Wanghaibin (D) <wanghaibin.wang@huawei.com>; 
> zhukeqian <zhukeqian1@huawei.com>
> Subject: Re: [PATCH 4/8] spapr_pci: Fix memory leak of 
> vmstate_spapr_pci
> 
> On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> > From: Jinhao Gao <gaojinhao@huawei.com>
> > 
> > When VM migrate VMState of spapr_pci, the field(msi_devs) of 
> > spapr_pci having a flag of VMS_ALLOC need to allocate memory. If the 
> > src doesn't free memory of msi_devs in SaveStateEntry of spapr_pci 
> > after QEMUFile save VMState of spapr_pci, it may result in memory leak of msi_devs.
> > We add the post_save func to free memory, which prevents memory leak.
> > 
> > Signed-off-by: Jinhao Gao <gaojinhao@huawei.com>
> 
> Not really a memory leak, since it will get freed on the next 
> pre_save.  But, we might as well free it earlier if we can ,so
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> 
> > ---
> >  hw/ppc/spapr_pci.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index
> > 76d7c91e9c..1b2b940606 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -2173,6 +2173,16 @@ static int spapr_pci_pre_save(void *opaque)
> >      return 0;
> >  }
> >  
> > +static int spapr_pci_post_save(void *opaque) {
> > +    SpaprPhbState *sphb = opaque;
> > +
> > +    g_free(sphb->msi_devs);
> > +    sphb->msi_devs = NULL;
> > +    sphb->msi_devs_num = 0;
> > +    return 0;
> > +}
> > +
> >  static int spapr_pci_post_load(void *opaque, int version_id)  {
> >      SpaprPhbState *sphb = opaque;
> > @@ -2205,6 +2215,7 @@ static const VMStateDescription vmstate_spapr_pci = {
> >      .version_id = 2,
> >      .minimum_version_id = 2,
> >      .pre_save = spapr_pci_pre_save,
> > +    .post_save = spapr_pci_post_save,
> >      .post_load = spapr_pci_post_load,
> >      .fields = (VMStateField[]) {
> >          VMSTATE_UINT64_EQUAL(buid, SpaprPhbState, NULL),
> 

-- 
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