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

Commit Message

gaojinhao Dec. 26, 2020, 10:33 a.m. UTC

From: Jinhao Gao <[e-mail]>

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 <[e-mail]>
---
 hw/ppc/spapr_pci.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

David Gibson Dec. 28, 2020, 6:58 a.m. UTC | #1

On Sat, Dec 26, 2020 at 06:33:43PM +0800, g00517791 wrote:
> From: Jinhao Gao <[e-mail]>
> 
> 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 <[e-mail]>

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 <[e-mail]>

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

gaojinhao Dec. 28, 2020, 8:10 a.m. UTC | #2

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.

Jinhao Gao

-----Original Message-----
From: David Gibson [mailto:[e-mail]] 
Sent: 2020-12-28 14:58
To: gaojinhao <[e-mail]>
Cc: [e-mail]; [e-mail]; Marc-André Lureau <[e-mail]>; Stefan Berger <[e-mail]>; Michael S . Tsirkin <[e-mail]>; Jason Wang <[e-mail]>; Greg Kurz <[e-mail]>; Juan Quintela <[e-mail]>; Dr . David Alan Gilbert <[e-mail]>; Wanghaibin (D) <[e-mail]>; zhukeqian <[e-mail]>
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 <[e-mail]>
> 
> 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 <[e-mail]>

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 <[e-mail]>

> ---
>  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 Dec. 28, 2020, 8:30 a.m. UTC | #3

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:[e-mail]] 
> Sent: 2020-12-28 14:58
> To: gaojinhao <[e-mail]>
> Cc: [e-mail]; [e-mail]; Marc-André Lureau <[e-mail]>; Stefan Berger <[e-mail]>; Michael S . Tsirkin <[e-mail]>; Jason Wang <[e-mail]>; Greg Kurz <[e-mail]>; Juan Quintela <[e-mail]>; Dr . David Alan Gilbert <[e-mail]>; Wanghaibin (D) <[e-mail]>; zhukeqian <[e-mail]>
> 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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> 
> 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 <[e-mail]>
> 
> > ---
> >  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),
>

gaojinhao Dec. 28, 2020, 9:31 a.m. UTC | #4

Thank you for you reply, I understand.

Jinhao Gao

-----Original Message-----
From: David Gibson [mailto:[e-mail]] 
Sent: 2020年12月28日 16:30
To: gaojinhao <[e-mail]>
Cc: [e-mail]; [e-mail]; Marc-André Lureau <[e-mail]>; Stefan Berger <[e-mail]>; Michael S . Tsirkin <[e-mail]>; Jason Wang <[e-mail]>; Greg Kurz <[e-mail]>; Juan Quintela <[e-mail]>; Dr . David Alan Gilbert <[e-mail]>; Wanghaibin (D) <[e-mail]>; zhukeqian <[e-mail]>
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:[e-mail]]
> Sent: 2020-12-28 14:58
> To: gaojinhao <[e-mail]>
> Cc: [e-mail]; [e-mail]; Marc-André Lureau 
> <[e-mail]>; Stefan Berger 
> <[e-mail]>; Michael S . Tsirkin <[e-mail]>; 
> Jason Wang <[e-mail]>; Greg Kurz <[e-mail]>; Juan 
> Quintela <[e-mail]>; Dr . David Alan Gilbert 
> <[e-mail]>; Wanghaibin (D) <[e-mail]>; 
> zhukeqian <[e-mail]>
> 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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> 
> 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 <[e-mail]>
> 
> > ---
> >  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),
>

Patch

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