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

[Christian Ehrhardt <christian.ehrhardt@canonical.com>]

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