From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 3EA422BF7 for ; Fri, 11 Mar 2016 15:00:15 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 11 Mar 2016 06:00:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,320,1455004800"; d="scan'208";a="762351369" Received: from cberenge-mobl.ger.corp.intel.com ([10.252.22.153]) by orsmga003.jf.intel.com with SMTP; 11 Mar 2016 06:00:12 -0800 Received: by (sSMTP sendmail emulation); Fri, 11 Mar 2016 14:00:11 +0025 Date: Fri, 11 Mar 2016 14:00:11 +0000 From: Bruce Richardson To: Christian Ehrhardt Message-ID: <20160311140011.GA11396@bricha3-MOBL3> References: <1457098145-13663-1-git-send-email-christian.ehrhardt@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: dev Subject: Re: [dpdk-dev] [PATCH] lpm/lpm6: fix missing free of rules_tbl and lpm X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Mar 2016 14:00:15 -0000 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 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 > > > >