[dpdk-dev] [PATCH] Add rte_mempool_free

[Walker, Benjamin]

On Mon, 2016-05-16 at 16:57 +0000, Wiles, Keith wrote:
> > 
> > There is no inverse of rte_mempool_create, so this patch adds one.
> > The typical usage of rte_mempool_create is to create a pool at
> > initialization time and only to free it upon program exit, so an
> > rte_mempool_free function at first seems to be of little value.
> > However, it is very useful as a sanity check for a clean shutdown
> > when used in conjunction with tools like AddressSanitizer. Further,
> > the call itself verifies that all elements have been returned to
> > the pool or it fails.
> > 
> > Signed-off-by: Ben Walker <benjamin.walker at intel.com>
> > ---
> > lib/librte_mempool/rte_dom0_mempool.c | 22 +++++++++++
> > lib/librte_mempool/rte_mempool.c      | 70 +++++++++++++++++++++++++++++++++++
> > lib/librte_mempool/rte_mempool.h      | 41 ++++++++++++++++++++
> > 3 files changed, 133 insertions(+)
> > 
> > diff --git a/lib/librte_mempool/rte_dom0_mempool.c b/lib/librte_mempool/rte_dom0_mempool.c
> > index 0d6d750..edf2d58 100644
> > --- a/lib/librte_mempool/rte_dom0_mempool.c
> > +++ b/lib/librte_mempool/rte_dom0_mempool.c
> > @@ -131,3 +131,25 @@ rte_dom0_mempool_create(const char *name, unsigned elt_num, unsigned
> > elt_size,
> > 
> > 	return mp;
> > }
> > +
> > +/* free the mempool supporting Dom0 */
> > +int
> > +rte_dom0_mempool_free(struct rte_mempool *mp)
> > +{
> > +	const struct rte_memzone *mz;
> > +	char mz_name[RTE_MEMZONE_NAMESIZE];
> > +	int rc;
> > +
> > +	rc = rte_mempool_xmem_free(mp);
> > +	if (rc) {
> > +		return rc;
> > +	}
> Remove {} on single line statements.

I'll remove braces from all of the single line conditionals and resend.

> > 
> > +
> > +	snprintf(mz_name, sizeof(mz_name), RTE_MEMPOOL_OBJ_NAME, mp->name);
> > +	mz = rte_memzone_lookup(mz_name);
> > +	if (mz) {
> > +		rte_memzone_free(mz);
> > +	}
> Here too.
> > 
> > +
> > +	return 0;
> > +}
> > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> > index 70812d9..82f645e 100644
> > --- a/lib/librte_mempool/rte_mempool.c
> > +++ b/lib/librte_mempool/rte_mempool.c
> > @@ -638,6 +638,76 @@ exit_unlock:
> > 	return NULL;
> > }
> > 
> > +#ifndef RTE_LIBRTE_XEN_DOM0
> > +/* stub if DOM0 support not configured */
> > +int
> > +rte_dom0_mempool_free(struct rte_mempool *mp __rte_unused)
> > +{
> > +	rte_errno = EINVAL;
> > +	return -1;
> I was thinking this should just return OK or zero. The chances of being called is very low and
> maybe will not be called, right? If so then do we need the function?

The user does know about this function because it is declared in the header, so they could
mistakenly call it. In that case, they'd have to pass in a mempool pointer, so I think the correct
thing to do is fail the call with EINVAL like I have it because if XEN_DOM0 is not supported then
the mempool they pass to this function is certainly not created with rte_dom0_mempool_create and is
therefore an invalid parameter.

> > 
> > +}
> > +#endif
> > +
> > +int
> > +rte_mempool_free(struct rte_mempool *mp)
> > +{
> > +	if (rte_xen_dom0_supported())
> > +		return rte_dom0_mempool_free(mp);
> > +	else
> > +		return rte_mempool_xmem_free(mp);
> > +}
> > +
> > +
> > +/* Free the memory pool */
> > +int
> > +rte_mempool_xmem_free(struct rte_mempool *mp)
> > +{
> > +	char mz_name[RTE_MEMZONE_NAMESIZE];
> > +	struct rte_mempool_list *mempool_list;
> > +	struct rte_tailq_entry *te = NULL;
> > +	const struct rte_memzone *mz;
> > +	unsigned count;
> > +
> > +	if (!mp) {
> > +		return 0;
> > +	}
> Remove the extra {}
> > 
> > +
> > +	count = rte_mempool_free_count(mp);
> > +	if (count != 0) {
> > +		/* All elements must be returned to the pool before free */
> > +		return count;
> > +	}
> This one also does not really need the {}
> > 
> > +
> > +	rte_rwlock_write_lock(RTE_EAL_MEMPOOL_RWLOCK);
> > +
> > +	/* Free the ring associated with this mempool */
> > +	if (mp->ring) {
> > +		rte_ring_free(mp->ring);
> > +	}
> This one too.
> > 
> > +
> > +	/* Remove the entry from the mempool list and free it. */
> > +	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
> > +	mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list);
> > +	TAILQ_FOREACH(te, mempool_list, next) {
> > +		if ((struct rte_mempool *)te->data == mp) {
> > +			TAILQ_REMOVE(mempool_list, te, next);
> > +			rte_free(te);
> > +			break;
> > +		}
> > +	}
> > +	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > +
> > +	snprintf(mz_name, sizeof(mz_name), RTE_MEMPOOL_MZ_FORMAT, mp->name);
> > +	mz = rte_memzone_lookup(mz_name);
> > +	if (mz) {
> > +		rte_memzone_free(mz);
> > +	}
> This one too.
> > 
> > +
> > +	rte_rwlock_write_unlock(RTE_EAL_MEMPOOL_RWLOCK);
> > +
> > +	return 0;
> > +}
> The big question is how do you know the mempool is not being used someplace?

That's the user's responsibility. Use after free is certainly possible if the user doesn't take
care, just like any alloc/free in C. This is the same situation as rte_ring_free or
rte_memzone_free. To help prevent users from shooting themselves in the foot I did add a check that
all of the elements have been freed back to the pool at the top of the function. There are certainly
potential race conditions if the user is freeing this on one thread and using it from another that I
haven't handled. I'm not sure these cases need to be handled though - they're not handled by
rte_ring_free, for example.

> 
> > 
> > +
> > /* Return the number of entries in the mempool */
> > unsigned
> > rte_mempool_count(const struct rte_mempool *mp)
> > diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> > index 9745bf0..26949c7 100644
> > --- a/lib/librte_mempool/rte_mempool.h
> > +++ b/lib/librte_mempool/rte_mempool.h
> > @@ -728,6 +728,47 @@ rte_dom0_mempool_create(const char *name, unsigned n, unsigned elt_size,
> > 		rte_mempool_obj_ctor_t *obj_init, void *obj_init_arg,
> > 		int socket_id, unsigned flags);
> > 
> > +/**
> > + * Free the memory pool created by rte_mempool_create
> > + *
> > + * All elements must be placed back in the pool prior to calling this function.
> > + *
> > + * @param mp
> > + *   A pointer to the mempool structure.
> > + * @return
> > + *   0 on success. -1 on error  with rte_errno set appropriately.
> > + *     Possible rte_errno values include:
> > + *    - EINVAL - Invalid input value.
> > + */
> > +int rte_mempool_free(struct rte_mempool *mp);
> > +
> > +/**
> > + * Free the memory pool created by rte_mempool_xmem_create.
> > + *
> > + * All elements must be placed back in the pool prior to calling this function.
> > + *
> > + * @param mp
> > + *   A pointer to the mempool structure.
> > + * @return
> > + *   0 on success. -1 on error  with rte_errno set appropriately.
> > + *     Possible rte_errno values include:
> > + *    - EINVAL - Invalid input value.
> > + */
> > +int rte_mempool_xmem_free(struct rte_mempool *mp);
> > +
> > +/**
> > + * Free the memory pool created by rte_dom0_mempool_create.
> > + *
> > + * All elements must be placed back in the pool prior to calling this function.
> > + *
> > + * @param mp
> > + *   A pointer to the mempool structure.
> > + * @return
> > + *   0 on success. -1 on error  with rte_errno set appropriately.
> > + *     Possible rte_errno values include:
> > + *    - EINVAL - Invalid input value.
> > + */
> > +int rte_dom0_mempool_free(struct rte_mempool *mp);
> > 
> > /**
> >  * Dump the status of the mempool to the console.
> > -- 
> > 2.5.5
> > 
> > 
> 
> Regards,
> Keith
> 
> 
> 
>