* [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).