[v5,2/5] mbuf: detach mbuf with pinned external buffer

Message ID 1579541003-2399-3-git-send-email-viacheslavo@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series mbuf: detach mbuf with pinned external buffer |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Slava Ovsiienko Jan. 20, 2020, 5:23 p.m. UTC
  Update detach routine to check the mbuf pool type.
Introduce the special internal version of detach routine to handle
the special case of pinned external bufferon mbuf freeing.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 lib/librte_mbuf/rte_mbuf.h | 102 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 93 insertions(+), 9 deletions(-)
  

Comments

Olivier Matz Jan. 20, 2020, 5:40 p.m. UTC | #1
On Mon, Jan 20, 2020 at 05:23:20PM +0000, Viacheslav Ovsiienko wrote:
> Update detach routine to check the mbuf pool type.
> Introduce the special internal version of detach routine to handle
> the special case of pinned external bufferon mbuf freeing.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

[...]

In case there is a new version, can you please add some newlines:

> +static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
> +{
> +	struct rte_mbuf_ext_shared_info *shinfo;
> +
> +	/* Clear flags, mbuf is being freed. */
> +	m->ol_flags = EXT_ATTACHED_MBUF;
> +	shinfo = m->shinfo;

here

> +	/* Optimize for performance - do not dec/reinit */
> +	if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 1))
> +		return 0;

here

> +	/*
> +	 * Direct usage of add primitive to avoid
> +	 * duplication of comparing with one.
> +	 */
> +	if (likely(rte_atomic16_add_return
> +			(&shinfo->refcnt_atomic, -1)))
> +		return 1;

here

> +	/* Reinitialize counter before mbuf freeing. */
> +	rte_mbuf_ext_refcnt_set(shinfo, 1);
> +	return 0;
> +}
> +
> +/**
>   * Decrease reference counter and unlink a mbuf segment
>   *
>   * This function does the same than a free, except that it does not

Apart from this,
Acked-by: Olivier Matz <olivier.matz@6wind.com>
  

Patch

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 9b0691d..7a41aad 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -323,6 +323,24 @@  struct rte_pktmbuf_pool_private {
 	return mbp_priv->flags;
 }
 
+/**
+ * When set, pktmbuf mempool will hold only mbufs with pinned external
+ * buffer. The external buffer will be attached to the mbuf at the
+ * memory pool creation and will never be detached by the mbuf free calls.
+ * mbuf should not contain any room for data after the mbuf structure.
+ */
+#define RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF (1 << 0)
+
+/**
+ * Returns non zero if given mbuf has a pinned external buffer, or zero
+ * otherwise. The pinned external buffer is allocated at pool creation
+ * time and should not be freed on mbuf freeing.
+ *
+ * External buffer is a user-provided anonymous buffer.
+ */
+#define RTE_MBUF_HAS_PINNED_EXTBUF(mb) \
+	(rte_pktmbuf_priv_flags(mb->pool) & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF)
+
 #ifdef RTE_LIBRTE_MBUF_DEBUG
 
 /**  check mbuf type in debug mode */
@@ -588,7 +606,8 @@  static inline struct rte_mbuf *rte_mbuf_raw_alloc(struct rte_mempool *mp)
 static __rte_always_inline void
 rte_mbuf_raw_free(struct rte_mbuf *m)
 {
-	RTE_ASSERT(RTE_MBUF_DIRECT(m));
+	RTE_ASSERT(!RTE_MBUF_CLONED(m) &&
+		  (!RTE_MBUF_HAS_EXTBUF(m) || RTE_MBUF_HAS_PINNED_EXTBUF(m)));
 	RTE_ASSERT(rte_mbuf_refcnt_read(m) == 1);
 	RTE_ASSERT(m->next == NULL);
 	RTE_ASSERT(m->nb_segs == 1);
@@ -794,7 +813,7 @@  static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
 	m->nb_segs = 1;
 	m->port = MBUF_INVALID_PORT;
 
-	m->ol_flags = 0;
+	m->ol_flags &= EXT_ATTACHED_MBUF;
 	m->packet_type = 0;
 	rte_pktmbuf_reset_headroom(m);
 
@@ -1153,6 +1172,11 @@  static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
  *
  * All other fields of the given packet mbuf will be left intact.
  *
+ * If the packet mbuf was allocated from the pool with pinned
+ * external buffers the rte_pktmbuf_detach does nothing with the
+ * mbuf of this kind, because the pinned buffers are not supposed
+ * to be detached.
+ *
  * @param m
  *   The indirect attached packet mbuf.
  */
@@ -1162,11 +1186,26 @@  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 	uint32_t mbuf_size, buf_len;
 	uint16_t priv_size;
 
-	if (RTE_MBUF_HAS_EXTBUF(m))
+	if (RTE_MBUF_HAS_EXTBUF(m)) {
+		/*
+		 * The mbuf has the external attached buffer,
+		 * we should check the type of the memory pool where
+		 * the mbuf was allocated from to detect the pinned
+		 * external buffer.
+		 */
+		uint32_t flags = rte_pktmbuf_priv_flags(mp);
+
+		if (flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) {
+			/*
+			 * The pinned external buffer should not be
+			 * detached from its backing mbuf, just exit.
+			 */
+			return;
+		}
 		__rte_pktmbuf_free_extbuf(m);
-	else
+	} else {
 		__rte_pktmbuf_free_direct(m);
-
+	}
 	priv_size = rte_pktmbuf_priv_size(mp);
 	mbuf_size = (uint32_t)(sizeof(struct rte_mbuf) + priv_size);
 	buf_len = rte_pktmbuf_data_room_size(mp);
@@ -1181,6 +1220,41 @@  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 }
 
 /**
+ * @internal Handle the packet mbufs with attached pinned external buffer
+ * on the mbuf freeing:
+ *
+ *  - return zero if reference counter in shinfo is one. It means there is
+ *  no more reference to this pinned buffer and mbuf can be returned to
+ *  the pool
+ *
+ *  - otherwise (if reference counter is not one), decrement reference
+ *  counter and return non-zero value to prevent freeing the backing mbuf.
+ *
+ * Returns non zero if mbuf should not be freed.
+ */
+static inline int __rte_pktmbuf_pinned_extbuf_decref(struct rte_mbuf *m)
+{
+	struct rte_mbuf_ext_shared_info *shinfo;
+
+	/* Clear flags, mbuf is being freed. */
+	m->ol_flags = EXT_ATTACHED_MBUF;
+	shinfo = m->shinfo;
+	/* Optimize for performance - do not dec/reinit */
+	if (likely(rte_mbuf_ext_refcnt_read(shinfo) == 1))
+		return 0;
+	/*
+	 * Direct usage of add primitive to avoid
+	 * duplication of comparing with one.
+	 */
+	if (likely(rte_atomic16_add_return
+			(&shinfo->refcnt_atomic, -1)))
+		return 1;
+	/* Reinitialize counter before mbuf freeing. */
+	rte_mbuf_ext_refcnt_set(shinfo, 1);
+	return 0;
+}
+
+/**
  * Decrease reference counter and unlink a mbuf segment
  *
  * This function does the same than a free, except that it does not
@@ -1201,8 +1275,13 @@  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 
 	if (likely(rte_mbuf_refcnt_read(m) == 1)) {
 
-		if (!RTE_MBUF_DIRECT(m))
-			rte_pktmbuf_detach(m);
+		if (!RTE_MBUF_DIRECT(m)) {
+			if (!RTE_MBUF_HAS_EXTBUF(m) ||
+			    !RTE_MBUF_HAS_PINNED_EXTBUF(m))
+				rte_pktmbuf_detach(m);
+			else if (__rte_pktmbuf_pinned_extbuf_decref(m))
+				return NULL;
+		}
 
 		if (m->next != NULL) {
 			m->next = NULL;
@@ -1213,8 +1292,13 @@  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 
 	} else if (__rte_mbuf_refcnt_update(m, -1) == 0) {
 
-		if (!RTE_MBUF_DIRECT(m))
-			rte_pktmbuf_detach(m);
+		if (!RTE_MBUF_DIRECT(m)) {
+			if (!RTE_MBUF_HAS_EXTBUF(m) ||
+			    !RTE_MBUF_HAS_PINNED_EXTBUF(m))
+				rte_pktmbuf_detach(m);
+			else if (__rte_pktmbuf_pinned_extbuf_decref(m))
+				return NULL;
+		}
 
 		if (m->next != NULL) {
 			m->next = NULL;