diff mbox series

[1/4] mbuf: detach mbuf with pinned external buffer

Message ID 1578679022-25344-2-git-send-email-viacheslavo@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series mbuf: introduce pktmbuf pool with pinned external buffers |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Slava Ovsiienko Jan. 10, 2020, 5:56 p.m. UTC 
  Update detach routine to check the mbuf pool type.

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

Comments

Stephen Hemminger Jan. 10, 2020, 6:23 p.m. UTC | #1 
On Fri, 10 Jan 2020 17:56:59 +0000
Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:

> +
> +static inline uint64_t
> +rte_mbuf_has_pinned_extbuf(const struct rte_mbuf *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.
> +		 */
> +		struct rte_pktmbuf_pool_private *priv =
> +			(struct rte_pktmbuf_pool_private *)
> +				rte_mempool_get_priv(m->pool);
> +
> +		return priv->flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF;
> +	}
> +	return 0;
> +}

New functions need to be marked experimental.

The return value should be boolean not uint64_t

Why does this need to be inlined (and thereby create new ABI burden)?
Also having it inline makes making pktmbuf_pool_private really private in future.
Slava Ovsiienko Jan. 13, 2020, 5:07 p.m. UTC | #2 
Hi, Stephen

Thanks a lot for the comment.

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, January 10, 2020 20:24
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH 1/4] mbuf: detach mbuf with pinned external
> buffer
> 
> On Fri, 10 Jan 2020 17:56:59 +0000
> Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > +
> > +static inline uint64_t
> > +rte_mbuf_has_pinned_extbuf(const struct rte_mbuf *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.
> > +		 */
> > +		struct rte_pktmbuf_pool_private *priv =
> > +			(struct rte_pktmbuf_pool_private *)
> > +				rte_mempool_get_priv(m->pool);
> > +
> > +		return priv->flags &
> RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF;
> > +	}
> > +	return 0;
> > +}
> 
> New functions need to be marked experimental.
> The return value should be boolean not uint64_t
Will be fixed in v2.

> 
> Why does this need to be inlined (and thereby create new ABI burden)?
> Also having it inline makes making pktmbuf_pool_private really private in
> future.
Due to performance reasons. This routine potentially might be used in datapath.

With best regards, Slava
Slava Ovsiienko Jan. 14, 2020, 7:19 a.m. UTC | #3 
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, January 10, 2020 20:24
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH 1/4] mbuf: detach mbuf with pinned external
> buffer
> 
> On Fri, 10 Jan 2020 17:56:59 +0000
> Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > +
> > +static inline uint64_t
> > +rte_mbuf_has_pinned_extbuf(const struct rte_mbuf *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.
> > +		 */
> > +		struct rte_pktmbuf_pool_private *priv =
> > +			(struct rte_pktmbuf_pool_private *)
> > +				rte_mempool_get_priv(m->pool);
> > +
> > +		return priv->flags &
> RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF;
> > +	}
> > +	return 0;
> > +}
> 
> New functions need to be marked experimental.
> 
> The return value should be boolean not uint64_t
I intentionally avoided the "bool" in v1. This is not native C type,
requires the extra header (stdbool.h at least) and has poor portability -
we would run into conflict with AltiVec - "bool" becomes reserved keyword.
So, I'm going to change return value type to uint32_t (as private flag field has).

With best regards, Slava
diff mbox series

Patch

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 219b110..e115ae5 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -306,6 +306,41 @@  struct rte_pktmbuf_pool_private {
 	uint32_t flags; /**< reserved for future use. */
 };
 
+/**
+ * When set pktmbuf mempool will hold only mbufs with pinned external
+ * buffer. The external buffer will be attached on 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 TRUE if given mbuf has an pinned external buffer, or FALSE
+ * otherwise. The pinned external buffer is allocated at pool creation
+ * time and should not be freed.
+ *
+ * External buffer is a user-provided anonymous buffer.
+ */
+#define RTE_MBUF_HAS_PINNED_EXTBUF(mb) rte_mbuf_has_pinned_extbuf(mb)
+
+static inline uint64_t
+rte_mbuf_has_pinned_extbuf(const struct rte_mbuf *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.
+		 */
+		struct rte_pktmbuf_pool_private *priv =
+			(struct rte_pktmbuf_pool_private *)
+				rte_mempool_get_priv(m->pool);
+
+		return priv->flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF;
+	}
+	return 0;
+}
+
 #ifdef RTE_LIBRTE_MBUF_DEBUG
 
 /**  check mbuf type in debug mode */
@@ -571,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);
@@ -1141,11 +1177,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 buffed,
+		 * we should check the type of the memory pool where
+		 * the mbuf was allocated from to detect the pinned
+		 * external buffer.
+		 */
+		struct rte_pktmbuf_pool_private *priv =
+			(struct rte_pktmbuf_pool_private *)
+				rte_mempool_get_priv(mp);
+
+		if (priv->flags & RTE_PKTMBUF_POOL_F_PINNED_EXT_BUF) {
+			RTE_ASSERT(m->shinfo == NULL);
+			m->ol_flags = EXT_ATTACHED_MBUF;
+			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);