[PATCH v5 0/3] mbuf: simplify handling of reinitialized mbufs

[PATCH v5 0/3] mbuf: simplify handling of reinitialized mbufs

[Morten Brørup]

This series contains three changes:
- sanity checking a reinitialized mbuf (a.k.a. raw mbuf) was de-inlined
- reinitialized mbuf free and alloc bulk functions were promoted as stable
- resetting fields of reinitialized mbufs was simplified, so fields that
  are already reset are not reset again

Morten Brørup (3):
  mbuf: de-inline sanity checking a reinitialized mbuf
  mbuf: promote raw free and alloc bulk functions as stable
  mbuf: no need to reset all fields on reinitialized mbufs

 lib/mbuf/rte_mbuf.c |  61 +++++++++++++++++++
 lib/mbuf/rte_mbuf.h | 141 ++++++++++++++++++++++++++++----------------
 2 files changed, 150 insertions(+), 52 deletions(-)

-- 
2.43.0

[PATCH v5 1/3] mbuf: de-inline sanity checking a reinitialized mbuf

[Morten Brørup]

Sanity checking a reinitialized mbuf (a.k.a. raw mbuf) has been refactored
to follow the same design pattern as sanity checking a normal mbuf, and
now depends on RTE_LIBRTE_MBUF_DEBUG instead of RTE_ENABLE_ASSERT.

The details of the changes are as follows:

Non-inlined functions rte_mbuf_raw_sanity_check() and rte_mbuf_raw_check()
have been added.
They do for a reinitialized mbuf what rte_mbuf_sanity_check() and
rte_mbuf_check() do for a normal mbuf.
They basically check the same conditions as __rte_mbuf_raw_sanity_check()
previously did, but use "if (!condition) rte_panic(message)" instead of
RTE_ASSERT(), so they don't depend on RTE_ENABLE_ASSERT.

The inline function __rte_mbuf_raw_sanity_check() has been replaced
by the new macro __rte_mbuf_raw_sanity_check_mp(), which either calls
rte_mbuf_raw_sanity_check() or does nothing, depending on
RTE_LIBRTE_MBUF_DEBUG, just like the __rte_mbuf_sanity_check() macro does
for a normal mbuf.

Note that the new macro __rte_mbuf_raw_sanity_check_mp() takes an optional
mempool parameter to verify that the mbuf belongs to the expected mbuf
pool.
This addition is mainly relevant for sanity checking reinitialized mbufs
freed directly into a given mempool by a PMD when using
RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE.

The macro __rte_mbuf_raw_sanity_check() has been kept for backwards API
compatibility.
It simply calls __rte_mbuf_raw_sanity_check_mp() without specifying a
mempool.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
---
v4:
* No changes.
v3:
* Removed experimental status for the new functions.
  Experimental is optional for new symbols, to allow for future API
  changes. But this has been around for a long time. (Stephen Hemminger)
* Consequentially, the added build check for ALLOW_EXPERIMENTAL_API with
  RTE_LIBRTE_MBUF_DEBUG is no longer needed, and was removed.
v2:
* Added explicit build check for ALLOW_EXPERIMENTAL_API being enabled when
  RTE_LIBRTE_MBUF_DEBUG is enabled, with descriptive error message if not
  so. (Ivan Malov)
* Fixed typo in patch description.
---
 lib/mbuf/rte_mbuf.c | 61 ++++++++++++++++++++++++++++
 lib/mbuf/rte_mbuf.h | 96 +++++++++++++++++++++++++++------------------
 2 files changed, 119 insertions(+), 38 deletions(-)

diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
index 9e7731a8a2..af39c13cf7 100644
--- a/lib/mbuf/rte_mbuf.c
+++ b/lib/mbuf/rte_mbuf.c
@@ -373,6 +373,67 @@ rte_pktmbuf_pool_create_extbuf(const char *name, unsigned int n,
 	return mp;
 }
 
+/* do some sanity checks on a reinitialized mbuf: panic if it fails */
+RTE_EXPORT_SYMBOL(rte_mbuf_raw_sanity_check)
+void
+rte_mbuf_raw_sanity_check(const struct rte_mbuf *m, const struct rte_mempool *mp)
+{
+	const char *reason;
+
+	if (rte_mbuf_raw_check(m, mp, &reason))
+		rte_panic("%s\n", reason);
+}
+
+RTE_EXPORT_SYMBOL(rte_mbuf_raw_check)
+int rte_mbuf_raw_check(const struct rte_mbuf *m, const struct rte_mempool *mp,
+		   const char **reason)
+{
+	/* check sanity */
+	if (rte_mbuf_check(m, 0, reason) == -1)
+		return -1;
+
+	/* check initialized */
+	if (rte_mbuf_refcnt_read(m) != 1) {
+		*reason = "uninitialized ref cnt";
+		return -1;
+	}
+	if (m->next != NULL) {
+		*reason = "uninitialized next ptr";
+		return -1;
+	}
+	if (m->nb_segs != 1) {
+		*reason = "uninitialized nb_segs";
+		return -1;
+	}
+	if (RTE_MBUF_CLONED(m)) {
+		*reason = "cloned";
+		return -1;
+	}
+	if (RTE_MBUF_HAS_EXTBUF(m)) {
+		if (!RTE_MBUF_HAS_PINNED_EXTBUF(m)) {
+			*reason = "external buffer not pinned";
+			return -1;
+		}
+
+		uint16_t cnt = rte_mbuf_ext_refcnt_read(m->shinfo);
+		if ((cnt == 0) || (cnt == UINT16_MAX)) {
+			*reason = "pinned external buffer bad ref cnt";
+			return -1;
+		}
+		if (cnt != 1) {
+			*reason = "pinned external buffer uninitialized ref cnt";
+			return -1;
+		}
+	}
+
+	if (mp != NULL && m->pool != mp) {
+		*reason = "wrong mbuf pool";
+		return -1;
+	}
+
+	return 0;
+}
+
 /* do some sanity checks on a mbuf: panic if it fails */
 RTE_EXPORT_SYMBOL(rte_mbuf_sanity_check)
 void
diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
index 06ab7502a5..552cda1ae5 100644
--- a/lib/mbuf/rte_mbuf.h
+++ b/lib/mbuf/rte_mbuf.h
@@ -339,11 +339,17 @@ rte_pktmbuf_priv_flags(struct rte_mempool *mp)
 
 #ifdef RTE_LIBRTE_MBUF_DEBUG
 
+/**  check reinitialized mbuf type in debug mode */
+#define __rte_mbuf_raw_sanity_check_mp(m, mp) rte_mbuf_raw_sanity_check(m, mp)
+
 /**  check mbuf type in debug mode */
 #define __rte_mbuf_sanity_check(m, is_h) rte_mbuf_sanity_check(m, is_h)
 
 #else /*  RTE_LIBRTE_MBUF_DEBUG */
 
+/**  check reinitialized mbuf type in debug mode */
+#define __rte_mbuf_raw_sanity_check_mp(m, mp) do { } while (0)
+
 /**  check mbuf type in debug mode */
 #define __rte_mbuf_sanity_check(m, is_h) do { } while (0)
 
@@ -513,6 +519,46 @@ rte_mbuf_ext_refcnt_update(struct rte_mbuf_ext_shared_info *shinfo,
 } while (0)
 
 
+/**
+ * Sanity checks on a reinitialized mbuf.
+ *
+ * Check the consistency of the given reinitialized mbuf.
+ * The function will cause a panic if corruption is detected.
+ *
+ * Check that the mbuf is properly reinitialized (refcnt=1, next=NULL,
+ * nb_segs=1), as done by rte_pktmbuf_prefree_seg().
+ *
+ * @param m
+ *   The mbuf to be checked.
+ * @param mp
+ *   The mempool to which the mbuf belongs.
+ *   NULL if unknown, not to be checked.
+ */
+void
+rte_mbuf_raw_sanity_check(const struct rte_mbuf *m, const struct rte_mempool *mp);
+
+/**
+ * Sanity checks on a reinitialized mbuf.
+ *
+ * Almost like rte_mbuf_raw_sanity_check(), but this function gives the reason
+ * if corruption is detected rather than panic.
+ *
+ * @param m
+ *   The mbuf to be checked.
+ * @param mp
+ *   The mempool to which the mbuf belongs.
+ *   NULL if unknown, not to be checked.
+ * @param reason
+ *   A reference to a string pointer where to store the reason why a mbuf is
+ *   considered invalid.
+ * @return
+ *   - 0 if no issue has been found, reason is left untouched.
+ *   - -1 if a problem is detected, reason then points to a string describing
+ *     the reason why the mbuf is deemed invalid.
+ */
+int rte_mbuf_raw_check(const struct rte_mbuf *m, const struct rte_mempool *mp,
+		   const char **reason);
+
 /**
  * Sanity checks on an mbuf.
  *
@@ -550,33 +596,11 @@ rte_mbuf_sanity_check(const struct rte_mbuf *m, int is_header);
 int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 		   const char **reason);
 
-/**
- * Sanity checks on a reinitialized mbuf in debug mode.
- *
- * Check the consistency of the given reinitialized mbuf.
- * The function will cause a panic if corruption is detected.
- *
- * Check that the mbuf is properly reinitialized (refcnt=1, next=NULL,
- * nb_segs=1), as done by rte_pktmbuf_prefree_seg().
- *
- * @param m
- *   The mbuf to be checked.
- */
-static __rte_always_inline void
-__rte_mbuf_raw_sanity_check(__rte_unused const struct rte_mbuf *m)
-{
-	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
-	RTE_ASSERT(m->next == NULL);
-	RTE_ASSERT(m->nb_segs == 1);
-	RTE_ASSERT(!RTE_MBUF_CLONED(m));
-	RTE_ASSERT(!RTE_MBUF_HAS_EXTBUF(m) ||
-			(RTE_MBUF_HAS_PINNED_EXTBUF(m) &&
-			rte_mbuf_ext_refcnt_read(m->shinfo) == 1));
-	__rte_mbuf_sanity_check(m, 0);
-}
+/** For backwards compatibility. */
+#define __rte_mbuf_raw_sanity_check(m) __rte_mbuf_raw_sanity_check_mp(m, NULL)
 
 /** For backwards compatibility. */
-#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check(m)
+#define MBUF_RAW_ALLOC_CHECK(m) __rte_mbuf_raw_sanity_check_mp(m, NULL)
 
 /**
  * Allocate an uninitialized mbuf from mempool *mp*.
@@ -606,7 +630,7 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
 
 	if (rte_mempool_get(mp, &ret.ptr) < 0)
 		return NULL;
-	__rte_mbuf_raw_sanity_check(ret.m);
+	__rte_mbuf_raw_sanity_check_mp(ret.m, mp);
 	return ret.m;
 }
 
@@ -644,7 +668,7 @@ rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf **mbufs, unsigne
 	int rc = rte_mempool_get_bulk(mp, (void **)mbufs, count);
 	if (likely(rc == 0))
 		for (unsigned int idx = 0; idx < count; idx++)
-			__rte_mbuf_raw_sanity_check(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], mp);
 	return rc;
 }
 
@@ -665,7 +689,7 @@ rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf **mbufs, unsigne
 static __rte_always_inline void
 rte_mbuf_raw_free(struct rte_mbuf *m)
 {
-	__rte_mbuf_raw_sanity_check(m);
+	__rte_mbuf_raw_sanity_check_mp(m, NULL);
 	rte_mempool_put(m->pool, m);
 }
 
@@ -696,12 +720,8 @@ __rte_experimental
 static __rte_always_inline void
 rte_mbuf_raw_free_bulk(struct rte_mempool *mp, struct rte_mbuf **mbufs, unsigned int count)
 {
-	for (unsigned int idx = 0; idx < count; idx++) {
-		const struct rte_mbuf *m = mbufs[idx];
-		RTE_ASSERT(m != NULL);
-		RTE_ASSERT(m->pool == mp);
-		__rte_mbuf_raw_sanity_check(m);
-	}
+	for (unsigned int idx = 0; idx < count; idx++)
+		__rte_mbuf_raw_sanity_check_mp(mbufs[idx], mp);
 
 	rte_mempool_put_bulk(mp, (void **)mbufs, count);
 }
@@ -1021,22 +1041,22 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
 	switch (count % 4) {
 	case 0:
 		while (idx != count) {
-			__rte_mbuf_raw_sanity_check(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], pool);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 3:
-			__rte_mbuf_raw_sanity_check(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], pool);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 2:
-			__rte_mbuf_raw_sanity_check(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], pool);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 1:
-			__rte_mbuf_raw_sanity_check(mbufs[idx]);
+			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], pool);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
-- 
2.43.0

[PATCH v5 2/3] promote reinitialized mbuf free and alloc bulk functions as stable

[Morten Brørup]

Ethdev drivers should use these APIs for allocating/freeing mbufs instead
of bypassing the mbuf library and accessing the mempool APIs, so they
cannot be experimental anymore.

Also updated the packet mbuf alloc bulk function to use the reinitialized
mbuf (a.k.a. raw mbuf) alloc bulk function, now that it is stable.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v5:
* Fix compiler warning.
---
 lib/mbuf/rte_mbuf.h | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
index 552cda1ae5..49c93ab356 100644
--- a/lib/mbuf/rte_mbuf.h
+++ b/lib/mbuf/rte_mbuf.h
@@ -635,9 +635,6 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
 }
 
 /**
- * @warning
- * @b EXPERIMENTAL: This API may change, or be removed, without prior notice.
- *
  * Allocate a bulk of uninitialized mbufs from mempool *mp*.
  *
  * This function can be used by PMDs (especially in Rx functions)
@@ -661,7 +658,6 @@ static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
  *   - 0: Success.
  *   - -ENOENT: Not enough entries in the mempool; no mbufs are retrieved.
  */
-__rte_experimental
 static __rte_always_inline int
 rte_mbuf_raw_alloc_bulk(struct rte_mempool *mp, struct rte_mbuf **mbufs, unsigned int count)
 {
@@ -694,9 +690,6 @@ rte_mbuf_raw_free(struct rte_mbuf *m)
 }
 
 /**
- * @warning
- * @b EXPERIMENTAL: This API may change, or be removed, without prior notice.
- *
  * Put a bulk of mbufs allocated from the same mempool back into the mempool.
  *
  * The caller must ensure that the mbufs come from the specified mempool,
@@ -716,7 +709,6 @@ rte_mbuf_raw_free(struct rte_mbuf *m)
  * @param count
  *   Array size.
  */
-__rte_experimental
 static __rte_always_inline void
 rte_mbuf_raw_free_bulk(struct rte_mempool *mp, struct rte_mbuf **mbufs, unsigned int count)
 {
@@ -1029,7 +1021,7 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
 	unsigned idx = 0;
 	int rc;
 
-	rc = rte_mempool_get_bulk(pool, (void **)mbufs, count);
+	rc = rte_mbuf_raw_alloc_bulk(pool, mbufs, count);
 	if (unlikely(rc))
 		return rc;
 
@@ -1041,22 +1033,18 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
 	switch (count % 4) {
 	case 0:
 		while (idx != count) {
-			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], pool);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 3:
-			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], pool);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 2:
-			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], pool);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 1:
-			__rte_mbuf_raw_sanity_check_mp(mbufs[idx], pool);
 			rte_pktmbuf_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
-- 
2.43.0

[PATCH v5 3/3] mbuf: no need to reset all fields on reinitialized mbufs

[Morten Brørup]

The 'next' and 'nb_segs' fields are already reset on newly allocated
reinitialized mbufs (a.k.a. raw mbufs), so a simpler reset function for
such mbufs was added.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/mbuf/rte_mbuf.h | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
index 49c93ab356..3517ca6858 100644
--- a/lib/mbuf/rte_mbuf.h
+++ b/lib/mbuf/rte_mbuf.h
@@ -954,6 +954,35 @@ static inline void rte_pktmbuf_reset_headroom(struct rte_mbuf *m)
 					(uint16_t)m->buf_len);
 }
 
+/**
+ * Reset the fields of a packet mbuf to their default values.
+ *
+ * The caller must ensure that the mbuf is direct and properly
+ * reinitialized (refcnt=1, next=NULL, nb_segs=1), as done by
+ * rte_pktmbuf_prefree_seg().
+ *
+ * This function should be used with care, when optimization is required.
+ * For standard needs, prefer rte_pktmbuf_reset().
+ *
+ * @param m
+ *   The packet mbuf to be reset.
+ */
+static inline void rte_mbuf_raw_reset(struct rte_mbuf *m)
+{
+	m->pkt_len = 0;
+	m->tx_offload = 0;
+	m->vlan_tci = 0;
+	m->vlan_tci_outer = 0;
+	m->port = RTE_MBUF_PORT_INVALID;
+
+	m->ol_flags &= RTE_MBUF_F_EXTERNAL;
+	m->packet_type = 0;
+	rte_pktmbuf_reset_headroom(m);
+
+	m->data_len = 0;
+	__rte_mbuf_sanity_check(m, 1);
+}
+
 /**
  * Reset the fields of a packet mbuf to their default values.
  *
@@ -997,7 +1026,7 @@ static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
 {
 	struct rte_mbuf *m;
 	if ((m = rte_mbuf_raw_alloc(mp)) != NULL)
-		rte_pktmbuf_reset(m);
+		rte_mbuf_raw_reset(m);
 	return m;
 }
 
@@ -1033,19 +1062,19 @@ static inline int rte_pktmbuf_alloc_bulk(struct rte_mempool *pool,
 	switch (count % 4) {
 	case 0:
 		while (idx != count) {
-			rte_pktmbuf_reset(mbufs[idx]);
+			rte_mbuf_raw_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 3:
-			rte_pktmbuf_reset(mbufs[idx]);
+			rte_mbuf_raw_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 2:
-			rte_pktmbuf_reset(mbufs[idx]);
+			rte_mbuf_raw_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 	case 1:
-			rte_pktmbuf_reset(mbufs[idx]);
+			rte_mbuf_raw_reset(mbufs[idx]);
 			idx++;
 			/* fall-through */
 		}
-- 
2.43.0