Re: [dpdk-dev] [PATCH] Add rte_mempool_free

["Wiles, Keith" <keith.wiles@intel.com>]

>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@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.
>+
>+	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?
>+}
>+#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?

>+
> /* 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