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

[Bruce Richardson <bruce.richardson@intel.com>]

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