diff mbox

[dpdk-dev] lpm/lpm6: fix missing free of rules_tbl and lpm

Message ID 1457098145-13663-1-git-send-email-christian.ehrhardt@canonical.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Christian Ehrhardt March 4, 2016, 1:29 p.m. UTC 
  lpm6 autotests failed with the default alloc of 512M Memory.
While >=2500M was a workaround it became clear while debugging that it
had a leak.
One could see a lot of output like:
  LPM Test tests6[i]: FAIL
  LPM: LPM memory allocation failed

It turned out that in rte_lpm6_free
- lpm might not be freed if it didn't find a te (early return)
- lpm->rules_tbl was not freed ever

The first of the two also applies to lpm.
---
 lib/librte_lpm/rte_lpm.c  | 7 ++-----
 lib/librte_lpm/rte_lpm6.c | 9 ++++-----
 2 files changed, 6 insertions(+), 10 deletions(-)

Comments

Christian Ehrhardt March 8, 2016, 4:38 p.m. UTC | #1 
Hi,
Stephen acked the other LPM patch I had last week (thanks).
There was no feedback to this one so far and none of the two patches is
committed yet.

So I wanted to give this another "ping" for feedback or acceptance.

Thanks in advance,
Christian

Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Fri, Mar 4, 2016 at 2:29 PM, Christian Ehrhardt <
christian.ehrhardt@canonical.com> wrote:

> lpm6 autotests failed with the default alloc of 512M Memory.
> While >=2500M was a workaround it became clear while debugging that it
> had a leak.
> One could see a lot of output like:
>   LPM Test tests6[i]: FAIL
>   LPM: LPM memory allocation failed
>
> It turned out that in rte_lpm6_free
> - lpm might not be freed if it didn't find a te (early return)
> - lpm->rules_tbl was not freed ever
>
> The first of the two also applies to lpm.
> ---
>  lib/librte_lpm/rte_lpm.c  | 7 ++-----
>  lib/librte_lpm/rte_lpm6.c | 9 ++++-----
>  2 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> index cfdf959..941720d 100644
> --- a/lib/librte_lpm/rte_lpm.c
> +++ b/lib/librte_lpm/rte_lpm.c
> @@ -236,13 +236,10 @@ rte_lpm_free(struct rte_lpm *lpm)
>                 if (te->data == (void *) lpm)
>                         break;
>         }
> -       if (te == NULL) {
> -               rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> -               return;
> +       if (te != NULL) {
> +               TAILQ_REMOVE(lpm_list, te, next);
>         }
>
> -       TAILQ_REMOVE(lpm_list, te, next);
> -
>         rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>
>         rte_free(lpm);
> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> index 48931cc..5abfc78 100644
> --- a/lib/librte_lpm/rte_lpm6.c
> +++ b/lib/librte_lpm/rte_lpm6.c
> @@ -278,15 +278,14 @@ rte_lpm6_free(struct rte_lpm6 *lpm)
>                 if (te->data == (void *) lpm)
>                         break;
>         }
> -       if (te == NULL) {
> -               rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> -               return;
> -       }
>
> -       TAILQ_REMOVE(lpm_list, te, next);
> +       if (te != NULL) {
> +               TAILQ_REMOVE(lpm_list, te, next);
> +       }
>
>         rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>
> +       rte_free(lpm->rules_tbl);
>         rte_free(lpm);
>         rte_free(te);
>  }
> --
> 2.7.0
>
>
Bruce Richardson March 11, 2016, 2 p.m. UTC | #2 
On Tue, Mar 08, 2016 at 05:38:28PM +0100, Christian Ehrhardt wrote:
> Hi,
> Stephen acked the other LPM patch I had last week (thanks).
> There was no feedback to this one so far and none of the two patches is
> committed yet.
> 
> So I wanted to give this another "ping" for feedback or acceptance.
> 
> Thanks in advance,
> Christian
> 
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
>
Hi Christian,

sorry for the delayed comments. This fix looks ok to me, but the patch appears
to be missing your signoff.

Two minor comments
	* This is probably better as two patches, one for lpm6, other for lpm
	* Coding standards state that we don't use "{}" for blocks with only
	  a single line.

Otherwise:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Regards,
/Bruce

> On Fri, Mar 4, 2016 at 2:29 PM, Christian Ehrhardt <
> christian.ehrhardt@canonical.com> wrote:
> 
> > lpm6 autotests failed with the default alloc of 512M Memory.
> > While >=2500M was a workaround it became clear while debugging that it
> > had a leak.
> > One could see a lot of output like:
> >   LPM Test tests6[i]: FAIL
> >   LPM: LPM memory allocation failed
> >
> > It turned out that in rte_lpm6_free
> > - lpm might not be freed if it didn't find a te (early return)
> > - lpm->rules_tbl was not freed ever
> >
> > The first of the two also applies to lpm.
> > ---
> >  lib/librte_lpm/rte_lpm.c  | 7 ++-----
> >  lib/librte_lpm/rte_lpm6.c | 9 ++++-----
> >  2 files changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> > index cfdf959..941720d 100644
> > --- a/lib/librte_lpm/rte_lpm.c
> > +++ b/lib/librte_lpm/rte_lpm.c
> > @@ -236,13 +236,10 @@ rte_lpm_free(struct rte_lpm *lpm)
> >                 if (te->data == (void *) lpm)
> >                         break;
> >         }
> > -       if (te == NULL) {
> > -               rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > -               return;
> > +       if (te != NULL) {
> > +               TAILQ_REMOVE(lpm_list, te, next);
> >         }
> >
> > -       TAILQ_REMOVE(lpm_list, te, next);
> > -
> >         rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> >
> >         rte_free(lpm);
> > diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> > index 48931cc..5abfc78 100644
> > --- a/lib/librte_lpm/rte_lpm6.c
> > +++ b/lib/librte_lpm/rte_lpm6.c
> > @@ -278,15 +278,14 @@ rte_lpm6_free(struct rte_lpm6 *lpm)
> >                 if (te->data == (void *) lpm)
> >                         break;
> >         }
> > -       if (te == NULL) {
> > -               rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > -               return;
> > -       }
> >
> > -       TAILQ_REMOVE(lpm_list, te, next);
> > +       if (te != NULL) {
> > +               TAILQ_REMOVE(lpm_list, te, next);
> > +       }
> >
> >         rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> >
> > +       rte_free(lpm->rules_tbl);
> >         rte_free(lpm);
> >         rte_free(te);
> >  }
> > --
> > 2.7.0
> >
> >
Christian Ehrhardt March 11, 2016, 3:02 p.m. UTC | #3 
Hi Bruce,
thanks I'll split into two patches and ensure there is no sign-off missing.
While doing so I'll also rebase to latest master to get rid of the offset a
patch would report.
I'll send all three lpm related patches as one series then adding your acks
to the respective patches.

Thanks,
Christian


Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Fri, Mar 11, 2016 at 3:00 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Tue, Mar 08, 2016 at 05:38:28PM +0100, Christian Ehrhardt wrote:
> > Hi,
> > Stephen acked the other LPM patch I had last week (thanks).
> > There was no feedback to this one so far and none of the two patches is
> > committed yet.
> >
> > So I wanted to give this another "ping" for feedback or acceptance.
> >
> > Thanks in advance,
> > Christian
> >
> > Christian Ehrhardt
> > Software Engineer, Ubuntu Server
> > Canonical Ltd
> >
> Hi Christian,
>
> sorry for the delayed comments. This fix looks ok to me, but the patch
> appears
> to be missing your signoff.
>
> Two minor comments
>         * This is probably better as two patches, one for lpm6, other for
> lpm
>         * Coding standards state that we don't use "{}" for blocks with
> only
>           a single line.
>
> Otherwise:
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
> Regards,
> /Bruce
>
> > On Fri, Mar 4, 2016 at 2:29 PM, Christian Ehrhardt <
> > christian.ehrhardt@canonical.com> wrote:
> >
> > > lpm6 autotests failed with the default alloc of 512M Memory.
> > > While >=2500M was a workaround it became clear while debugging that it
> > > had a leak.
> > > One could see a lot of output like:
> > >   LPM Test tests6[i]: FAIL
> > >   LPM: LPM memory allocation failed
> > >
> > > It turned out that in rte_lpm6_free
> > > - lpm might not be freed if it didn't find a te (early return)
> > > - lpm->rules_tbl was not freed ever
> > >
> > > The first of the two also applies to lpm.
> > > ---
> > >  lib/librte_lpm/rte_lpm.c  | 7 ++-----
> > >  lib/librte_lpm/rte_lpm6.c | 9 ++++-----
> > >  2 files changed, 6 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
> > > index cfdf959..941720d 100644
> > > --- a/lib/librte_lpm/rte_lpm.c
> > > +++ b/lib/librte_lpm/rte_lpm.c
> > > @@ -236,13 +236,10 @@ rte_lpm_free(struct rte_lpm *lpm)
> > >                 if (te->data == (void *) lpm)
> > >                         break;
> > >         }
> > > -       if (te == NULL) {
> > > -               rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > > -               return;
> > > +       if (te != NULL) {
> > > +               TAILQ_REMOVE(lpm_list, te, next);
> > >         }
> > >
> > > -       TAILQ_REMOVE(lpm_list, te, next);
> > > -
> > >         rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > >
> > >         rte_free(lpm);
> > > diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> > > index 48931cc..5abfc78 100644
> > > --- a/lib/librte_lpm/rte_lpm6.c
> > > +++ b/lib/librte_lpm/rte_lpm6.c
> > > @@ -278,15 +278,14 @@ rte_lpm6_free(struct rte_lpm6 *lpm)
> > >                 if (te->data == (void *) lpm)
> > >                         break;
> > >         }
> > > -       if (te == NULL) {
> > > -               rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > > -               return;
> > > -       }
> > >
> > > -       TAILQ_REMOVE(lpm_list, te, next);
> > > +       if (te != NULL) {
> > > +               TAILQ_REMOVE(lpm_list, te, next);
> > > +       }
> > >
> > >         rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > >
> > > +       rte_free(lpm->rules_tbl);
> > >         rte_free(lpm);
> > >         rte_free(te);
> > >  }
> > > --
> > > 2.7.0
> > >
> > >
>
diff mbox

Patch

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index cfdf959..941720d 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -236,13 +236,10 @@  rte_lpm_free(struct rte_lpm *lpm)
 		if (te->data == (void *) lpm)
 			break;
 	}
-	if (te == NULL) {
-		rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
-		return;
+	if (te != NULL) {
+		TAILQ_REMOVE(lpm_list, te, next);
 	}
 
-	TAILQ_REMOVE(lpm_list, te, next);
-
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
 	rte_free(lpm);
diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index 48931cc..5abfc78 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -278,15 +278,14 @@  rte_lpm6_free(struct rte_lpm6 *lpm)
 		if (te->data == (void *) lpm)
 			break;
 	}
-	if (te == NULL) {
-		rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
-		return;
-	}
 
-	TAILQ_REMOVE(lpm_list, te, next);
+	if (te != NULL) {
+		TAILQ_REMOVE(lpm_list, te, next);
+	}
 
 	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
 
+	rte_free(lpm->rules_tbl);
 	rte_free(lpm);
 	rte_free(te);
 }