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 364002A07 for ; Wed, 9 Sep 2015 09:48:50 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 09 Sep 2015 00:48:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,495,1437462000"; d="scan'208";a="641286312" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by orsmga003.jf.intel.com with ESMTP; 09 Sep 2015 00:48:48 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.12]) by IRSMSX103.ger.corp.intel.com ([169.254.3.251]) with mapi id 14.03.0224.002; Wed, 9 Sep 2015 08:48:47 +0100 From: "De Lara Guarch, Pablo" To: Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] ring: add function to free a ring Thread-Index: AQHQ2b5lDbECoGiBVkuy92xWzTuAZJ4wzsKAgAMf2OA= Date: Wed, 9 Sep 2015 07:48:46 +0000 Message-ID: References: <1439906454-12860-1-git-send-email-pablo.de.lara.guarch@intel.com> <55ED4E97.3070505@6wind.com> In-Reply-To: <55ED4E97.3070505@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] ring: add function to free a ring 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: Wed, 09 Sep 2015 07:48:51 -0000 > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Monday, September 07, 2015 9:45 AM > To: De Lara Guarch, Pablo; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] ring: add function to free a ring >=20 > Hi Pablo, >=20 > Please find some comments below. >=20 > On 08/18/2015 04:00 PM, Pablo de Lara wrote: > > When creating a ring, a memzone is created to allocate it in memory, > > but the ring could not be freed, as memzones could not be. > > > > Since memzones can be freed now, then rings can be as well, > > taking into account if they were initialized using pre-allocated memory > > (in which case, memory should be freed externally) or using > rte_memzone_reserve > > (with rte_ring_create), freeing the memory with rte_memzone_free. > > > > Signed-off-by: Pablo de Lara > > --- > > lib/librte_ring/rte_ring.c | 43 > ++++++++++++++++++++++++++++++++++++ > > lib/librte_ring/rte_ring.h | 7 ++++++ > > lib/librte_ring/rte_ring_version.map | 7 ++++++ > > 3 files changed, 57 insertions(+) > > > > diff --git a/lib/librte_ring/rte_ring.c b/lib/librte_ring/rte_ring.c > > index c9e59d4..83ce6d3 100644 > > --- a/lib/librte_ring/rte_ring.c > > +++ b/lib/librte_ring/rte_ring.c > > @@ -208,6 +208,49 @@ rte_ring_create(const char *name, unsigned > count, int socket_id, > > return r; > > } > > > > +/* free the ring */ > > +void > > +rte_ring_free(struct rte_ring *r) > > +{ > > + struct rte_ring_list *ring_list =3D NULL; > > + char mz_name[RTE_MEMZONE_NAMESIZE]; > > + struct rte_tailq_entry *te; > > + const struct rte_memzone *mz; > > + > > + if (r =3D=3D NULL) > > + return; > > + > > + snprintf(mz_name, sizeof(mz_name), "%s%s", > RTE_RING_MZ_PREFIX, r->name); > > + mz =3D rte_memzone_lookup(mz_name); > > + > > + /* > > + * Free ring memory if it was allocated with rte_memzone_reserve, > > + * otherwise it should be freed externally > > + */ > > + if (rte_memzone_free(mz) !=3D 0) > > + return; >=20 > Should we have a log here? I think it may hide important > bugs if we just return silently here. Agree. >=20 > > + > > + ring_list =3D RTE_TAILQ_CAST(rte_ring_tailq.head, rte_ring_list); > > + rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK); > > + > > + /* find out tailq entry */ > > + TAILQ_FOREACH(te, ring_list, next) { > > + if (te->data =3D=3D (void *) r) > > + break; > > + } > > + > > + if (te =3D=3D NULL) { > > + rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK); > > + return; > > + } >=20 > If I understand well, a ring is in the tailq only if it was > created with rte_ring_create(). A ring that is statically created > in memory is not in the tailq. But we already returned in that > case. So (te =3D=3D NULL) should not happen here, right? We could > also add an error log then. Yes, (te =3D=3D NULL) should not happen, but not sure if there is a way where it can happen, but looking at other libraries, like rte_hash or rte_a= cl, we check for this and don't add any error log. >=20 > I'm not sure we should handle the case where the ring is not allocated > with rte_ring_create() in this function. If the ring is allocated with > another mean (either in a global variable, or with another dynamic > memory allocator), this function should not be called. You are right that that rte_ring_free should not be called if the ring was not created with rte_ring_create, but we should have a way to handle it, just in case, right? >=20 > What do you think? Thanks for the comments! Pablo >=20 >=20 > Regards, > Olivier >=20 >=20 > > + > > + TAILQ_REMOVE(ring_list, te, next); > > + > > + rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK); > > + > > + rte_free(te); > > +} > > + > > /* > > * change the high water mark. If *count* is 0, water marking is > > * disabled > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h > > index af68888..e75566f 100644 > > --- a/lib/librte_ring/rte_ring.h > > +++ b/lib/librte_ring/rte_ring.h > > @@ -300,6 +300,13 @@ int rte_ring_init(struct rte_ring *r, const char > *name, unsigned count, > > */ > > struct rte_ring *rte_ring_create(const char *name, unsigned count, > > int socket_id, unsigned flags); > > +/** > > + * De-allocate all memory used by the ring. > > + * > > + * @param r > > + * Ring to free > > + */ > > +void rte_ring_free(struct rte_ring *r); > > > > /** > > * Change the high water mark. > > diff --git a/lib/librte_ring/rte_ring_version.map > b/lib/librte_ring/rte_ring_version.map > > index 982fdd1..5474b98 100644 > > --- a/lib/librte_ring/rte_ring_version.map > > +++ b/lib/librte_ring/rte_ring_version.map > > @@ -11,3 +11,10 @@ DPDK_2.0 { > > > > local: *; > > }; > > + > > +DPDK_2.2 { > > + global: > > + > > + rte_ring_free; > > + > > +} DPDK_2.0; > >