[dpdk-dev] [PATCH] ring: add function to free a ring

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Monday, September 07, 2015 9:45 AM
> To: De Lara Guarch, Pablo; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ring: add function to free a ring
> 
> Hi Pablo,
> 
> Please find some comments below.
> 
> 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 <pablo.de.lara.guarch at intel.com>
> > ---
> >  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 = NULL;
> > +	char mz_name[RTE_MEMZONE_NAMESIZE];
> > +	struct rte_tailq_entry *te;
> > +	const struct rte_memzone *mz;
> > +
> > +	if (r == NULL)
> > +		return;
> > +
> > +	snprintf(mz_name, sizeof(mz_name), "%s%s",
> RTE_RING_MZ_PREFIX, r->name);
> > +	mz = 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) != 0)
> > +		return;
> 
> Should we have a log here? I think it may hide important
> bugs if we just return silently here.

Agree.

> 
> > +
> > +	ring_list = 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 == (void *) r)
> > +			break;
> > +	}
> > +
> > +	if (te == NULL) {
> > +		rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > +		return;
> > +	}
> 
> 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 == NULL) should not happen here, right? We could
> also add an error log then.

Yes, (te == 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_acl,
we check for this and don't add any error log.

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

> 
> What do you think?

Thanks for the comments!

Pablo
> 
> 
> Regards,
> Olivier
> 
> 
> > +
> > +	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;
> >