* [dpdk-dev] [PATCH] lpm/lpm6: fix missing free of rules_tbl and lpm
@ 2016-03-04 13:29 Christian Ehrhardt
2016-03-08 16:38 ` Christian Ehrhardt
0 siblings, 1 reply; 4+ messages in thread
From: Christian Ehrhardt @ 2016-03-04 13:29 UTC (permalink / raw)
To: christian.ehrhardt, dev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH] lpm/lpm6: fix missing free of rules_tbl and lpm
2016-03-04 13:29 [dpdk-dev] [PATCH] lpm/lpm6: fix missing free of rules_tbl and lpm Christian Ehrhardt
@ 2016-03-08 16:38 ` Christian Ehrhardt
2016-03-11 14:00 ` Bruce Richardson
0 siblings, 1 reply; 4+ messages in thread
From: Christian Ehrhardt @ 2016-03-08 16:38 UTC (permalink / raw)
To: Christian Ehrhardt, dev
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH] lpm/lpm6: fix missing free of rules_tbl and lpm
2016-03-08 16:38 ` Christian Ehrhardt
@ 2016-03-11 14:00 ` Bruce Richardson
2016-03-11 15:02 ` Christian Ehrhardt
0 siblings, 1 reply; 4+ messages in thread
From: Bruce Richardson @ 2016-03-11 14:00 UTC (permalink / raw)
To: Christian Ehrhardt; +Cc: dev
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
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH] lpm/lpm6: fix missing free of rules_tbl and lpm
2016-03-11 14:00 ` Bruce Richardson
@ 2016-03-11 15:02 ` Christian Ehrhardt
0 siblings, 0 replies; 4+ messages in thread
From: Christian Ehrhardt @ 2016-03-11 15:02 UTC (permalink / raw)
To: Bruce Richardson, Christian Ehrhardt; +Cc: dev
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
> > >
> > >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-03-11 15:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-04 13:29 [dpdk-dev] [PATCH] lpm/lpm6: fix missing free of rules_tbl and lpm Christian Ehrhardt
2016-03-08 16:38 ` Christian Ehrhardt
2016-03-11 14:00 ` Bruce Richardson
2016-03-11 15:02 ` Christian Ehrhardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).