[dpdk-dev] [PATCH 0/5] mbuf related patches

[dpdk-dev] [PATCH 0/5] mbuf related patches

[Stephen Hemminger]

This patch set is all about improving the mbuf related cloning
and copying. They are motivated by seeing issues with mbuf copying
in rte_pdump and realized this a wider and more general problem.
The pdump copy could not handle different size pools and did
not handle meta data, etc.

They cause no functional or ABI changes. The only visible part
to older code is converting a couple of inlines to real functions.
This kind of change confuses checkpatch which thinks these new
functions should be marked experimental when they must not be.

Stephen Hemminger (5):
  mbuf: don't generate invalid mbuf in clone test
  mbuf: delinline rte_pktmbuf_linearize
  mbuf: deinline rte_pktmbuf_clone
  mbuf: add a pktmbuf copy routine
  mbuf: add pktmbuf copy test

 app/test/test_mbuf.c                 | 129 +++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.c           | 149 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 102 ++++++------------
 lib/librte_mbuf/rte_mbuf_version.map |   8 ++
 4 files changed, 315 insertions(+), 73 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/5] mbuf: don't generate invalid mbuf in clone test

[Stephen Hemminger]

The test for cloning changed mbuf would generate an mbuf
whose length and segments were invalid. This would cause a crash
if test was run with mbuf debugging enabled.

Fixes: f1022aba76a5 ("app/test: rename mbuf variable")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 2a97afe2044a..aafad0cf6206 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -332,8 +332,11 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
 	if (m->next == NULL)
 		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
 
 	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+
 	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
 	*data = MAGIC_DATA;
 
-- 
2.20.1

[dpdk-dev] [PATCH 2/5] mbuf: delinline rte_pktmbuf_linearize

[Stephen Hemminger]

This function is too big to be put inline. The places it
is used are only in special exception paths where a highly fragmented
mbuf arrives at a device that can't handle it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 40 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 40 ++--------------------------
 lib/librte_mbuf/rte_mbuf_version.map |  6 +++++
 3 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 37718d49c148..922bce6f0f93 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,46 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* convert multi-segment mbuf to single mbuf */
+int
+rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
+{
+	size_t seg_len, copy_len;
+	struct rte_mbuf *m;
+	struct rte_mbuf *m_next;
+	char *buffer;
+
+	if (rte_pktmbuf_is_contiguous(mbuf))
+		return 0;
+
+	/* Extend first segment to the total packet length */
+	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
+
+	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
+		return -1;
+
+	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
+	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
+
+	/* Append data from next segments to the first one */
+	m = mbuf->next;
+	while (m != NULL) {
+		m_next = m->next;
+
+		seg_len = rte_pktmbuf_data_len(m);
+		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
+		buffer += seg_len;
+
+		rte_pktmbuf_free_seg(m);
+		m = m_next;
+	}
+
+	mbuf->next = NULL;
+	mbuf->nb_segs = 1;
+
+	return 0;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 98225ec80bf1..d25356b58cba 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2412,44 +2412,8 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
  *   - 0, on success
  *   - -1, on error
  */
-static inline int
-rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
-{
-	size_t seg_len, copy_len;
-	struct rte_mbuf *m;
-	struct rte_mbuf *m_next;
-	char *buffer;
-
-	if (rte_pktmbuf_is_contiguous(mbuf))
-		return 0;
-
-	/* Extend first segment to the total packet length */
-	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
-
-	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
-		return -1;
-
-	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
-	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
-
-	/* Append data from next segments to the first one */
-	m = mbuf->next;
-	while (m != NULL) {
-		m_next = m->next;
-
-		seg_len = rte_pktmbuf_data_len(m);
-		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
-		buffer += seg_len;
-
-		rte_pktmbuf_free_seg(m);
-		m = m_next;
-	}
-
-	mbuf->next = NULL;
-	mbuf->nb_segs = 1;
-
-	return 0;
-}
+int
+rte_pktmbuf_linearize(struct rte_mbuf *mbuf);
 
 /**
  * Dump an mbuf structure to a file.
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 2662a37bf674..7af410a4f687 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -46,6 +46,12 @@ DPDK_18.08 {
 	rte_pktmbuf_pool_create_by_ops;
 } DPDK_16.11;
 
+DPDK_18.11 {
+	global:
+
+	rte_pktmbuf_linearize;
+} DPDK_18.08;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.20.1

[dpdk-dev] [PATCH 3/5] mbuf: deinline rte_pktmbuf_clone

[Stephen Hemminger]

Cloning mbufs requires allocations and iteration
and therefore should not be an inline.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 39 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 ++-------------------------
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 922bce6f0f93..12d0258a120d 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,45 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* Creates a shallow copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp)
+{
+	struct rte_mbuf *mc, *mi, **prev;
+	uint32_t pktlen;
+	uint16_t nseg;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	mi = mc;
+	prev = &mi->next;
+	pktlen = md->pkt_len;
+	nseg = 0;
+
+	do {
+		nseg++;
+		rte_pktmbuf_attach(mi, md);
+		*prev = mi;
+		prev = &mi->next;
+	} while ((md = md->next) != NULL &&
+	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
+
+	*prev = NULL;
+	mc->nb_segs = nseg;
+	mc->pkt_len = pktlen;
+
+	/* Allocation of new indirect segment failed */
+	if (unlikely(mi == NULL)) {
+		rte_pktmbuf_free(mc);
+		return NULL;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* convert multi-segment mbuf to single mbuf */
 int
 rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index d25356b58cba..a6e78e4ea7a6 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1924,42 +1924,8 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
  *   - The pointer to the new "clone" mbuf on success.
  *   - NULL if allocation fails.
  */
-static inline struct rte_mbuf *rte_pktmbuf_clone(struct rte_mbuf *md,
-		struct rte_mempool *mp)
-{
-	struct rte_mbuf *mc, *mi, **prev;
-	uint32_t pktlen;
-	uint16_t nseg;
-
-	if (unlikely ((mc = rte_pktmbuf_alloc(mp)) == NULL))
-		return NULL;
-
-	mi = mc;
-	prev = &mi->next;
-	pktlen = md->pkt_len;
-	nseg = 0;
-
-	do {
-		nseg++;
-		rte_pktmbuf_attach(mi, md);
-		*prev = mi;
-		prev = &mi->next;
-	} while ((md = md->next) != NULL &&
-	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
-
-	*prev = NULL;
-	mc->nb_segs = nseg;
-	mc->pkt_len = pktlen;
-
-	/* Allocation of new indirect segment failed */
-	if (unlikely (mi == NULL)) {
-		rte_pktmbuf_free(mc);
-		return NULL;
-	}
-
-	__rte_mbuf_sanity_check(mc, 1);
-	return mc;
-}
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 7af410a4f687..9cc43e24bfa4 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -49,6 +49,7 @@ DPDK_18.08 {
 DPDK_18.11 {
 	global:
 
+	rte_pktmbuf_clone;
 	rte_pktmbuf_linearize;
 } DPDK_18.08;
 
-- 
2.20.1

[dpdk-dev] [PATCH 4/5] mbuf: add a pktmbuf copy routine

[Stephen Hemminger]

This is a commonly used operation that surprisingly the
DPDK has not supported. The new rte_pktmbuf_copy does a
deep copy of packet. This is a complete copy including
meta-data.

It handles the case where the source mbuf comes from a pool
with larger data area than the destination pool. The routine
also has options for skipping data, or truncating at a fixed
length.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 70 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 26 +++++++++++
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 97 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 12d0258a120d..6888d6bd5dfc 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -324,6 +324,76 @@ rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 	return 0;
 }
 
+/* Create a deep copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t off, uint32_t len)
+{
+	const struct rte_mbuf *seg = m;
+	struct rte_mbuf *mc, *m_last, **prev;
+
+	if (unlikely(off >= m->pkt_len))
+		return NULL;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	if (len > m->pkt_len - off)
+		len = m->pkt_len - off;
+
+	/* clone meta data from original */
+	mc->port = m->port;
+	mc->vlan_tci = m->vlan_tci;
+	mc->vlan_tci_outer = m->vlan_tci_outer;
+	mc->tx_offload = m->tx_offload;
+	mc->hash = m->hash;
+	mc->packet_type = m->packet_type;
+	mc->timestamp = m->timestamp;
+
+	prev = &mc->next;
+	m_last = mc;
+	while (len > 0) {
+		uint32_t copy_len;
+
+		while (off >= seg->data_len) {
+			off -= seg->data_len;
+			seg = seg->next;
+		}
+
+		/* current buffer is full, chain a new one */
+		if (rte_pktmbuf_tailroom(m_last) == 0) {
+			m_last = rte_pktmbuf_alloc(mp);
+			if (unlikely(m_last == NULL)) {
+				rte_pktmbuf_free(mc);
+				return NULL;
+			}
+			++mc->nb_segs;
+			*prev = m_last;
+			prev = &m_last->next;
+		}
+
+		copy_len = RTE_MIN(seg->data_len - off, len);
+		if (copy_len > rte_pktmbuf_tailroom(m_last))
+			copy_len = rte_pktmbuf_tailroom(m_last);
+
+		/* append from seg to m_last */
+		rte_memcpy(rte_pktmbuf_mtod_offset(m_last, char *,
+						   m_last->data_len),
+			   rte_pktmbuf_mtod_offset(seg, char *,
+						   off),
+			   copy_len);
+
+		m_last->data_len += copy_len;
+		mc->pkt_len += copy_len;
+		off += copy_len;
+		len -= copy_len;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index a6e78e4ea7a6..77266a07c75b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1927,6 +1927,32 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 struct rte_mbuf *
 rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
+/**
+ * Creates a full copy of a given packet mbuf.
+ *
+ * Copies all the data from a given packet mbuf to a newly allocated
+ * set of mbufs.
+ *
+ * @param m
+ *   The packet mbuf to be cloned.
+ * @param mp
+ *   The mempool from which the "clone" mbufs are allocated.
+ * @param offset
+ *   The number of bytes to skip before copying.
+ *   If the mbuf does not have that many bytes, it is an error
+ *   and NULL is returned.
+ * @param length
+ *   The upper limit on bytes to copy.  Passing UINT32_MAX
+ *   means all data (after offset).
+ * @return
+ *   - The pointer to the new "clone" mbuf on success.
+ *   - NULL if allocation fails.
+ */
+__rte_experimental
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t offset, uint32_t length);
+
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
  *
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 9cc43e24bfa4..3742ed114e7c 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -57,4 +57,5 @@ EXPERIMENTAL {
 	global:
 
 	rte_mbuf_check;
+	rte_pktmbuf_copy;
 } DPDK_18.08;
-- 
2.20.1

[dpdk-dev] [PATCH 5/5] mbuf: add pktmbuf copy test

[Stephen Hemminger]

New test for rte_pktmbuf_copy based of the clone tests.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 126 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index aafad0cf6206..49c3a5f7893c 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -399,6 +399,127 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	return -1;
 }
 
+static int
+test_pktmbuf_copy(struct rte_mempool *pktmbuf_pool)
+{
+	struct rte_mbuf *m = NULL;
+	struct rte_mbuf *copy = NULL;
+	struct rte_mbuf *copy2 = NULL;
+	unaligned_uint32_t *data;
+
+	/* alloc a mbuf */
+	m = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m == NULL)
+		GOTO_FAIL("ooops not allocating mbuf");
+
+	if (rte_pktmbuf_pkt_len(m) != 0)
+		GOTO_FAIL("Bad length");
+
+	rte_pktmbuf_append(m, sizeof(uint32_t));
+	data = rte_pktmbuf_mtod(m, unaligned_uint32_t *);
+	*data = MAGIC_DATA;
+
+	/* copy the allocated mbuf */
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (*data != MAGIC_DATA)
+		GOTO_FAIL("invalid data in copy\n");
+
+	/* free the copy */
+	rte_pktmbuf_free(copy);
+	copy = NULL;
+
+	/* same test with a chained mbuf */
+	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m->next == NULL)
+		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
+
+	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
+	*data = MAGIC_DATA + 1;
+
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA || data[1] != MAGIC_DATA + 1)
+		GOTO_FAIL("invalid data in copy\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test offset copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 sizeof(uint32_t), UINT32_MAX);
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA + 1)
+		GOTO_FAIL("copy with offset, invalid data\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test truncation copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 0, sizeof(uint32_t));
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA)
+		GOTO_FAIL("copy with truncate, invalid data\n");
+
+	/* free mbuf */
+	rte_pktmbuf_free(m);
+	rte_pktmbuf_free(copy);
+	rte_pktmbuf_free(copy2);
+
+	m = NULL;
+	copy = NULL;
+	copy2 = NULL;
+	printf("%s ok\n", __func__);
+	return 0;
+
+fail:
+	if (m)
+		rte_pktmbuf_free(m);
+	if (copy)
+		rte_pktmbuf_free(copy);
+	if (copy2)
+		rte_pktmbuf_free(copy2);
+	return -1;
+}
+
 static int
 test_attach_from_different_pool(struct rte_mempool *pktmbuf_pool,
 				struct rte_mempool *pktmbuf_pool2)
@@ -1203,6 +1324,11 @@ test_mbuf(void)
 		goto err;
 	}
 
+	if (test_pktmbuf_copy(pktmbuf_pool) < 0) {
+		printf("test_pktmbuf_copy() failed.\n");
+		goto err;
+	}
+
 	if (test_attach_from_different_pool(pktmbuf_pool, pktmbuf_pool2) < 0) {
 		printf("test_attach_from_different_pool() failed\n");
 		goto err;
-- 
2.20.1

Re: [dpdk-dev] [PATCH 2/5] mbuf: delinline rte_pktmbuf_linearize

[Stephen Hemminger]

On Fri, 27 Sep 2019 17:37:55 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
> index 2662a37bf674..7af410a4f687 100644
> --- a/lib/librte_mbuf/rte_mbuf_version.map
> +++ b/lib/librte_mbuf/rte_mbuf_version.map
> @@ -46,6 +46,12 @@ DPDK_18.08 {
>  	rte_pktmbuf_pool_create_by_ops;
>  } DPDK_16.11;
>  
> +DPDK_18.11 {

Typo. Should be 19.11 will fix in v2

Re: [dpdk-dev] [PATCH 2/5] mbuf: delinline rte_pktmbuf_linearize

[Morten Brørup]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Saturday, September 28, 2019 2:38 AM
> 
> This function is too big to be put inline. The places it
> is used are only in special exception paths where a highly fragmented
> mbuf arrives at a device that can't handle it.

This assumption only true for the actual linearization. The application may not know if a device can handle it or not. So from an application perspective, calling rte_pktmbuf_linearize() may not be a special exception, e.g. in DPI applications.

I suggest keeping the rte_pktmbuf_is_contiguous(mbuf) test in an inlined part of rte_pktmbuf_linearize(), and only deinline the actual linearization.

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_mbuf/rte_mbuf.c           | 40 ++++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h           | 40 ++--------------------------
>  lib/librte_mbuf/rte_mbuf_version.map |  6 +++++
>  3 files changed, 48 insertions(+), 38 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 37718d49c148..922bce6f0f93 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -245,6 +245,46 @@ int rte_mbuf_check(const struct rte_mbuf *m, int
> is_header,
>  	return 0;
>  }
> 
> +/* convert multi-segment mbuf to single mbuf */
> +int
> +rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
> +{
> +	size_t seg_len, copy_len;
> +	struct rte_mbuf *m;
> +	struct rte_mbuf *m_next;
> +	char *buffer;
> +
> +	if (rte_pktmbuf_is_contiguous(mbuf))
> +		return 0;
> +
> +	/* Extend first segment to the total packet length */
> +	copy_len = rte_pktmbuf_pkt_len(mbuf) -
> rte_pktmbuf_data_len(mbuf);
> +
> +	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
> +		return -1;
> +
> +	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
> +	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
> +
> +	/* Append data from next segments to the first one */
> +	m = mbuf->next;
> +	while (m != NULL) {
> +		m_next = m->next;
> +
> +		seg_len = rte_pktmbuf_data_len(m);
> +		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
> +		buffer += seg_len;
> +
> +		rte_pktmbuf_free_seg(m);
> +		m = m_next;
> +	}
> +
> +	mbuf->next = NULL;
> +	mbuf->nb_segs = 1;
> +
> +	return 0;
> +}
> +
>  /* dump a mbuf on console */
>  void
>  rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 98225ec80bf1..d25356b58cba 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -2412,44 +2412,8 @@ rte_validate_tx_offload(const struct rte_mbuf
> *m)
>   *   - 0, on success
>   *   - -1, on error
>   */
> -static inline int
> -rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
> -{
> -	size_t seg_len, copy_len;
> -	struct rte_mbuf *m;
> -	struct rte_mbuf *m_next;
> -	char *buffer;
> -
> -	if (rte_pktmbuf_is_contiguous(mbuf))
> -		return 0;
> -
> -	/* Extend first segment to the total packet length */
> -	copy_len = rte_pktmbuf_pkt_len(mbuf) -
> rte_pktmbuf_data_len(mbuf);
> -
> -	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
> -		return -1;
> -
> -	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
> -	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
> -
> -	/* Append data from next segments to the first one */
> -	m = mbuf->next;
> -	while (m != NULL) {
> -		m_next = m->next;
> -
> -		seg_len = rte_pktmbuf_data_len(m);
> -		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
> -		buffer += seg_len;
> -
> -		rte_pktmbuf_free_seg(m);
> -		m = m_next;
> -	}
> -
> -	mbuf->next = NULL;
> -	mbuf->nb_segs = 1;
> -
> -	return 0;
> -}
> +int
> +rte_pktmbuf_linearize(struct rte_mbuf *mbuf);
> 
>  /**
>   * Dump an mbuf structure to a file.
> diff --git a/lib/librte_mbuf/rte_mbuf_version.map
> b/lib/librte_mbuf/rte_mbuf_version.map
> index 2662a37bf674..7af410a4f687 100644
> --- a/lib/librte_mbuf/rte_mbuf_version.map
> +++ b/lib/librte_mbuf/rte_mbuf_version.map
> @@ -46,6 +46,12 @@ DPDK_18.08 {
>  	rte_pktmbuf_pool_create_by_ops;
>  } DPDK_16.11;
> 
> +DPDK_18.11 {
> +	global:
> +
> +	rte_pktmbuf_linearize;
> +} DPDK_18.08;
> +
>  EXPERIMENTAL {
>  	global:
> 
> --
> 2.20.1
> 

Re: [dpdk-dev] [PATCH 4/5] mbuf: add a pktmbuf copy routine

[Morten Brørup]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Saturday, September 28, 2019 2:38 AM
> 
> This is a commonly used operation that surprisingly the
> DPDK has not supported. The new rte_pktmbuf_copy does a
> deep copy of packet. This is a complete copy including
> meta-data.
> 
> It handles the case where the source mbuf comes from a pool
> with larger data area than the destination pool. The routine
> also has options for skipping data, or truncating at a fixed
> length.

Great initiative, and the offset/length params makes it even better!

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_mbuf/rte_mbuf.c           | 70 ++++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h           | 26 +++++++++++
>  lib/librte_mbuf/rte_mbuf_version.map |  1 +
>  3 files changed, 97 insertions(+)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 12d0258a120d..6888d6bd5dfc 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -324,6 +324,76 @@ rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
>  	return 0;
>  }
> 
> +/* Create a deep copy of mbuf */
> +struct rte_mbuf *
> +rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
> +		 uint32_t off, uint32_t len)
> +{
> +	const struct rte_mbuf *seg = m;
> +	struct rte_mbuf *mc, *m_last, **prev;
> +
> +	if (unlikely(off >= m->pkt_len))
> +		return NULL;
> +
> +	mc = rte_pktmbuf_alloc(mp);
> +	if (unlikely(mc == NULL))
> +		return NULL;
> +
> +	if (len > m->pkt_len - off)
> +		len = m->pkt_len - off;
> +
> +	/* clone meta data from original */
> +	mc->port = m->port;
> +	mc->vlan_tci = m->vlan_tci;
> +	mc->vlan_tci_outer = m->vlan_tci_outer;
> +	mc->tx_offload = m->tx_offload;
> +	mc->hash = m->hash;
> +	mc->packet_type = m->packet_type;
> +	mc->timestamp = m->timestamp;

At the dynamic mbuf presentation at DPDK Userspace, it was mentioned that the last 8 bytes of the mbuf could be used by applications. Consider if their values should be copied too, and thus if they should be explicitly define in the mbuf, e.g. dynfield[1,2] like in the dynamic mbuf patch. I don't have an opinion on this, I'm just bringing it to your attention.


Med venlig hilsen / kind regards
- Morten Brørup

[dpdk-dev] [PATCH v2 0/6] mbuf copy related enhancements

[Stephen Hemminger]

This patch set is all about improving the mbuf related cloning
and copying. They are motivated by seeing issues with mbuf copying
in rte_pdump and realized this a wider and more general problem.
The pdump copy could not handle different size pools and did
not handle meta data, etc.

They cause no functional or ABI changes. The only visible part
to older code is converting a couple of inlines to real functions.
This kind of change confuses checkpatch which thinks these new
functions should be marked experimental when they must not be.

v2 - add pdump use of pktmbuf_copy
     fix version in map

Stephen Hemminger (6):
  mbuf: don't generate invalid mbuf in clone test
  mbuf: delinline rte_pktmbuf_linearize
  mbuf: deinline rte_pktmbuf_clone
  mbuf: add a pktmbuf copy routine
  mbuf: add pktmbuf copy test
  pdump: use new pktmbuf copy function

 app/test/test_mbuf.c                 | 129 +++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.c           | 149 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 102 ++++++------------
 lib/librte_mbuf/rte_mbuf_version.map |   8 ++
 lib/librte_pdump/rte_pdump.c         |  69 +------------
 5 files changed, 316 insertions(+), 141 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v2 1/6] mbuf: don't generate invalid mbuf in clone test

[Stephen Hemminger]

The test for cloning changed mbuf would generate an mbuf
whose length and segments were invalid. This would cause a crash
if test was run with mbuf debugging enabled.

Fixes: f1022aba76a5 ("app/test: rename mbuf variable")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 2a97afe2044a..aafad0cf6206 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -332,8 +332,11 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
 	if (m->next == NULL)
 		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
 
 	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+
 	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
 	*data = MAGIC_DATA;
 
-- 
2.20.1

[dpdk-dev] [PATCH v2 2/6] mbuf: delinline rte_pktmbuf_linearize

[Stephen Hemminger]

This function is too big to be put inline. The places it
is used are only in special exception paths where a highly fragmented
mbuf arrives at a device that can't handle it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 40 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 40 ++--------------------------
 lib/librte_mbuf/rte_mbuf_version.map |  6 +++++
 3 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 37718d49c148..922bce6f0f93 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,46 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* convert multi-segment mbuf to single mbuf */
+int
+rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
+{
+	size_t seg_len, copy_len;
+	struct rte_mbuf *m;
+	struct rte_mbuf *m_next;
+	char *buffer;
+
+	if (rte_pktmbuf_is_contiguous(mbuf))
+		return 0;
+
+	/* Extend first segment to the total packet length */
+	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
+
+	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
+		return -1;
+
+	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
+	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
+
+	/* Append data from next segments to the first one */
+	m = mbuf->next;
+	while (m != NULL) {
+		m_next = m->next;
+
+		seg_len = rte_pktmbuf_data_len(m);
+		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
+		buffer += seg_len;
+
+		rte_pktmbuf_free_seg(m);
+		m = m_next;
+	}
+
+	mbuf->next = NULL;
+	mbuf->nb_segs = 1;
+
+	return 0;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 98225ec80bf1..d25356b58cba 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2412,44 +2412,8 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
  *   - 0, on success
  *   - -1, on error
  */
-static inline int
-rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
-{
-	size_t seg_len, copy_len;
-	struct rte_mbuf *m;
-	struct rte_mbuf *m_next;
-	char *buffer;
-
-	if (rte_pktmbuf_is_contiguous(mbuf))
-		return 0;
-
-	/* Extend first segment to the total packet length */
-	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
-
-	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
-		return -1;
-
-	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
-	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
-
-	/* Append data from next segments to the first one */
-	m = mbuf->next;
-	while (m != NULL) {
-		m_next = m->next;
-
-		seg_len = rte_pktmbuf_data_len(m);
-		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
-		buffer += seg_len;
-
-		rte_pktmbuf_free_seg(m);
-		m = m_next;
-	}
-
-	mbuf->next = NULL;
-	mbuf->nb_segs = 1;
-
-	return 0;
-}
+int
+rte_pktmbuf_linearize(struct rte_mbuf *mbuf);
 
 /**
  * Dump an mbuf structure to a file.
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 2662a37bf674..528681ba263a 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -46,6 +46,12 @@ DPDK_18.08 {
 	rte_pktmbuf_pool_create_by_ops;
 } DPDK_16.11;
 
+DPDK_19.11 {
+	global:
+
+	rte_pktmbuf_linearize;
+} DPDK_18.08;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.20.1

[dpdk-dev] [PATCH v2 3/6] mbuf: deinline rte_pktmbuf_clone

[Stephen Hemminger]

Cloning mbufs requires allocations and iteration
and therefore should not be an inline.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 39 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 ++-------------------------
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 922bce6f0f93..12d0258a120d 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,45 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* Creates a shallow copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp)
+{
+	struct rte_mbuf *mc, *mi, **prev;
+	uint32_t pktlen;
+	uint16_t nseg;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	mi = mc;
+	prev = &mi->next;
+	pktlen = md->pkt_len;
+	nseg = 0;
+
+	do {
+		nseg++;
+		rte_pktmbuf_attach(mi, md);
+		*prev = mi;
+		prev = &mi->next;
+	} while ((md = md->next) != NULL &&
+	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
+
+	*prev = NULL;
+	mc->nb_segs = nseg;
+	mc->pkt_len = pktlen;
+
+	/* Allocation of new indirect segment failed */
+	if (unlikely(mi == NULL)) {
+		rte_pktmbuf_free(mc);
+		return NULL;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* convert multi-segment mbuf to single mbuf */
 int
 rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index d25356b58cba..a6e78e4ea7a6 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1924,42 +1924,8 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
  *   - The pointer to the new "clone" mbuf on success.
  *   - NULL if allocation fails.
  */
-static inline struct rte_mbuf *rte_pktmbuf_clone(struct rte_mbuf *md,
-		struct rte_mempool *mp)
-{
-	struct rte_mbuf *mc, *mi, **prev;
-	uint32_t pktlen;
-	uint16_t nseg;
-
-	if (unlikely ((mc = rte_pktmbuf_alloc(mp)) == NULL))
-		return NULL;
-
-	mi = mc;
-	prev = &mi->next;
-	pktlen = md->pkt_len;
-	nseg = 0;
-
-	do {
-		nseg++;
-		rte_pktmbuf_attach(mi, md);
-		*prev = mi;
-		prev = &mi->next;
-	} while ((md = md->next) != NULL &&
-	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
-
-	*prev = NULL;
-	mc->nb_segs = nseg;
-	mc->pkt_len = pktlen;
-
-	/* Allocation of new indirect segment failed */
-	if (unlikely (mi == NULL)) {
-		rte_pktmbuf_free(mc);
-		return NULL;
-	}
-
-	__rte_mbuf_sanity_check(mc, 1);
-	return mc;
-}
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 528681ba263a..aeec3f90f3fe 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -49,6 +49,7 @@ DPDK_18.08 {
 DPDK_19.11 {
 	global:
 
+	rte_pktmbuf_clone;
 	rte_pktmbuf_linearize;
 } DPDK_18.08;
 
-- 
2.20.1

[dpdk-dev] [PATCH v2 4/6] mbuf: add a pktmbuf copy routine

[Stephen Hemminger]

This is a commonly used operation that surprisingly the
DPDK has not supported. The new rte_pktmbuf_copy does a
deep copy of packet. This is a complete copy including
meta-data.

It handles the case where the source mbuf comes from a pool
with larger data area than the destination pool. The routine
also has options for skipping data, or truncating at a fixed
length.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 70 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 26 +++++++++++
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 97 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 12d0258a120d..6888d6bd5dfc 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -324,6 +324,76 @@ rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 	return 0;
 }
 
+/* Create a deep copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t off, uint32_t len)
+{
+	const struct rte_mbuf *seg = m;
+	struct rte_mbuf *mc, *m_last, **prev;
+
+	if (unlikely(off >= m->pkt_len))
+		return NULL;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	if (len > m->pkt_len - off)
+		len = m->pkt_len - off;
+
+	/* clone meta data from original */
+	mc->port = m->port;
+	mc->vlan_tci = m->vlan_tci;
+	mc->vlan_tci_outer = m->vlan_tci_outer;
+	mc->tx_offload = m->tx_offload;
+	mc->hash = m->hash;
+	mc->packet_type = m->packet_type;
+	mc->timestamp = m->timestamp;
+
+	prev = &mc->next;
+	m_last = mc;
+	while (len > 0) {
+		uint32_t copy_len;
+
+		while (off >= seg->data_len) {
+			off -= seg->data_len;
+			seg = seg->next;
+		}
+
+		/* current buffer is full, chain a new one */
+		if (rte_pktmbuf_tailroom(m_last) == 0) {
+			m_last = rte_pktmbuf_alloc(mp);
+			if (unlikely(m_last == NULL)) {
+				rte_pktmbuf_free(mc);
+				return NULL;
+			}
+			++mc->nb_segs;
+			*prev = m_last;
+			prev = &m_last->next;
+		}
+
+		copy_len = RTE_MIN(seg->data_len - off, len);
+		if (copy_len > rte_pktmbuf_tailroom(m_last))
+			copy_len = rte_pktmbuf_tailroom(m_last);
+
+		/* append from seg to m_last */
+		rte_memcpy(rte_pktmbuf_mtod_offset(m_last, char *,
+						   m_last->data_len),
+			   rte_pktmbuf_mtod_offset(seg, char *,
+						   off),
+			   copy_len);
+
+		m_last->data_len += copy_len;
+		mc->pkt_len += copy_len;
+		off += copy_len;
+		len -= copy_len;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index a6e78e4ea7a6..77266a07c75b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1927,6 +1927,32 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 struct rte_mbuf *
 rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
+/**
+ * Creates a full copy of a given packet mbuf.
+ *
+ * Copies all the data from a given packet mbuf to a newly allocated
+ * set of mbufs.
+ *
+ * @param m
+ *   The packet mbuf to be cloned.
+ * @param mp
+ *   The mempool from which the "clone" mbufs are allocated.
+ * @param offset
+ *   The number of bytes to skip before copying.
+ *   If the mbuf does not have that many bytes, it is an error
+ *   and NULL is returned.
+ * @param length
+ *   The upper limit on bytes to copy.  Passing UINT32_MAX
+ *   means all data (after offset).
+ * @return
+ *   - The pointer to the new "clone" mbuf on success.
+ *   - NULL if allocation fails.
+ */
+__rte_experimental
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t offset, uint32_t length);
+
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
  *
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index aeec3f90f3fe..f471bbcaa2df 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -57,4 +57,5 @@ EXPERIMENTAL {
 	global:
 
 	rte_mbuf_check;
+	rte_pktmbuf_copy;
 } DPDK_18.08;
-- 
2.20.1

[dpdk-dev] [PATCH v2 5/6] mbuf: add pktmbuf copy test

[Stephen Hemminger]

New test for rte_pktmbuf_copy based of the clone tests.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 126 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index aafad0cf6206..49c3a5f7893c 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -399,6 +399,127 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	return -1;
 }
 
+static int
+test_pktmbuf_copy(struct rte_mempool *pktmbuf_pool)
+{
+	struct rte_mbuf *m = NULL;
+	struct rte_mbuf *copy = NULL;
+	struct rte_mbuf *copy2 = NULL;
+	unaligned_uint32_t *data;
+
+	/* alloc a mbuf */
+	m = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m == NULL)
+		GOTO_FAIL("ooops not allocating mbuf");
+
+	if (rte_pktmbuf_pkt_len(m) != 0)
+		GOTO_FAIL("Bad length");
+
+	rte_pktmbuf_append(m, sizeof(uint32_t));
+	data = rte_pktmbuf_mtod(m, unaligned_uint32_t *);
+	*data = MAGIC_DATA;
+
+	/* copy the allocated mbuf */
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (*data != MAGIC_DATA)
+		GOTO_FAIL("invalid data in copy\n");
+
+	/* free the copy */
+	rte_pktmbuf_free(copy);
+	copy = NULL;
+
+	/* same test with a chained mbuf */
+	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m->next == NULL)
+		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
+
+	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
+	*data = MAGIC_DATA + 1;
+
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA || data[1] != MAGIC_DATA + 1)
+		GOTO_FAIL("invalid data in copy\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test offset copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 sizeof(uint32_t), UINT32_MAX);
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA + 1)
+		GOTO_FAIL("copy with offset, invalid data\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test truncation copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 0, sizeof(uint32_t));
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA)
+		GOTO_FAIL("copy with truncate, invalid data\n");
+
+	/* free mbuf */
+	rte_pktmbuf_free(m);
+	rte_pktmbuf_free(copy);
+	rte_pktmbuf_free(copy2);
+
+	m = NULL;
+	copy = NULL;
+	copy2 = NULL;
+	printf("%s ok\n", __func__);
+	return 0;
+
+fail:
+	if (m)
+		rte_pktmbuf_free(m);
+	if (copy)
+		rte_pktmbuf_free(copy);
+	if (copy2)
+		rte_pktmbuf_free(copy2);
+	return -1;
+}
+
 static int
 test_attach_from_different_pool(struct rte_mempool *pktmbuf_pool,
 				struct rte_mempool *pktmbuf_pool2)
@@ -1203,6 +1324,11 @@ test_mbuf(void)
 		goto err;
 	}
 
+	if (test_pktmbuf_copy(pktmbuf_pool) < 0) {
+		printf("test_pktmbuf_copy() failed.\n");
+		goto err;
+	}
+
 	if (test_attach_from_different_pool(pktmbuf_pool, pktmbuf_pool2) < 0) {
 		printf("test_attach_from_different_pool() failed\n");
 		goto err;
-- 
2.20.1

[dpdk-dev] [PATCH v2 6/6] pdump: use new pktmbuf copy function

[Stephen Hemminger]

The rte_pktmbuf_copy handles varying size mbuf pools correctly.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_pdump/rte_pdump.c | 69 +-----------------------------------
 1 file changed, 1 insertion(+), 68 deletions(-)

diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index cd24dd010951..c665cf237f65 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -64,73 +64,6 @@ static struct pdump_rxtx_cbs {
 } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
 tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
 
-static inline int
-pdump_pktmbuf_copy_data(struct rte_mbuf *seg, const struct rte_mbuf *m)
-{
-	if (rte_pktmbuf_tailroom(seg) < m->data_len) {
-		RTE_LOG(ERR, PDUMP,
-			"User mempool: insufficient data_len of mbuf\n");
-		return -EINVAL;
-	}
-
-	seg->port = m->port;
-	seg->vlan_tci = m->vlan_tci;
-	seg->hash = m->hash;
-	seg->tx_offload = m->tx_offload;
-	seg->ol_flags = m->ol_flags;
-	seg->packet_type = m->packet_type;
-	seg->vlan_tci_outer = m->vlan_tci_outer;
-	seg->data_len = m->data_len;
-	seg->pkt_len = seg->data_len;
-	rte_memcpy(rte_pktmbuf_mtod(seg, void *),
-			rte_pktmbuf_mtod(m, void *),
-			rte_pktmbuf_data_len(seg));
-
-	return 0;
-}
-
-static inline struct rte_mbuf *
-pdump_pktmbuf_copy(struct rte_mbuf *m, struct rte_mempool *mp)
-{
-	struct rte_mbuf *m_dup, *seg, **prev;
-	uint32_t pktlen;
-	uint16_t nseg;
-
-	m_dup = rte_pktmbuf_alloc(mp);
-	if (unlikely(m_dup == NULL))
-		return NULL;
-
-	seg = m_dup;
-	prev = &seg->next;
-	pktlen = m->pkt_len;
-	nseg = 0;
-
-	do {
-		nseg++;
-		if (pdump_pktmbuf_copy_data(seg, m) < 0) {
-			if (seg != m_dup)
-				rte_pktmbuf_free_seg(seg);
-			rte_pktmbuf_free(m_dup);
-			return NULL;
-		}
-		*prev = seg;
-		prev = &seg->next;
-	} while ((m = m->next) != NULL &&
-			(seg = rte_pktmbuf_alloc(mp)) != NULL);
-
-	*prev = NULL;
-	m_dup->nb_segs = nseg;
-	m_dup->pkt_len = pktlen;
-
-	/* Allocation of new indirect segment failed */
-	if (unlikely(seg == NULL)) {
-		rte_pktmbuf_free(m_dup);
-		return NULL;
-	}
-
-	__rte_mbuf_sanity_check(m_dup, 1);
-	return m_dup;
-}
 
 static inline void
 pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
@@ -148,7 +81,7 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
 	ring = cbs->ring;
 	mp = cbs->mp;
 	for (i = 0; i < nb_pkts; i++) {
-		p = pdump_pktmbuf_copy(pkts[i], mp);
+		p = rte_pktmbuf_copy(pkts[i], mp, 0, UINT32_MAX);
 		if (p)
 			dup_bufs[d_pkts++] = p;
 	}
-- 
2.20.1

[dpdk-dev] [PATCH v3 0/6] mbuf copy/cloning enhancements

[Stephen Hemminger]

This patch set is all about improving the mbuf related cloning
and copying. They are motivated by seeing issues with mbuf copying
in rte_pdump and realized this a wider and more general problem.
The pdump copy could not handle different size pools and did
not handle meta data, etc.

They cause no functional or ABI changes. The only visible part
to older code is converting a couple of inlines to real functions.
This kind of change confuses checkpatch which thinks these new
functions should be marked experimental when they must not be.

v3 - split linearize into internal/external
     copy private data in pktmbuf_copy
v2 - add pdump use of pktmbuf_copy
     fix version in map

Stephen Hemminger (6):
  mbuf: don't generate invalid mbuf in clone test
  mbuf: delinline rte_pktmbuf_linearize
  mbuf: deinline rte_pktmbuf_clone
  mbuf: add a pktmbuf copy routine
  mbuf: add pktmbuf copy test
  pdump: use new pktmbuf copy function

 app/test/test_mbuf.c                 | 129 +++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.c           | 150 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 100 ++++++------------
 lib/librte_mbuf/rte_mbuf_version.map |   8 ++
 lib/librte_pdump/rte_pdump.c         |  69 +-----------
 5 files changed, 321 insertions(+), 135 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v3 1/6] mbuf: don't generate invalid mbuf in clone test

[Stephen Hemminger]

The test for cloning changed mbuf would generate an mbuf
whose length and segments were invalid. This would cause a crash
if test was run with mbuf debugging enabled.

Fixes: f1022aba76a5 ("app/test: rename mbuf variable")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 2a97afe2044a..aafad0cf6206 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -332,8 +332,11 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
 	if (m->next == NULL)
 		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
 
 	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+
 	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
 	*data = MAGIC_DATA;
 
-- 
2.20.1

[dpdk-dev] [PATCH v3 2/6] mbuf: delinline rte_pktmbuf_linearize

[Stephen Hemminger]

This copy part of this function is too big to be put inline.
The places it is used are only in special exception paths
where a highly fragmented mbuf arrives at a device that can't handle it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 37 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 +++++-----------------------
 lib/librte_mbuf/rte_mbuf_version.map |  6 +++++
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 37718d49c148..e2c661c97522 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,43 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* convert multi-segment mbuf to single mbuf */
+int
+__rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
+{
+	size_t seg_len, copy_len;
+	struct rte_mbuf *m;
+	struct rte_mbuf *m_next;
+	char *buffer;
+
+	/* Extend first segment to the total packet length */
+	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
+
+	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
+		return -1;
+
+	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
+	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
+
+	/* Append data from next segments to the first one */
+	m = mbuf->next;
+	while (m != NULL) {
+		m_next = m->next;
+
+		seg_len = rte_pktmbuf_data_len(m);
+		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
+		buffer += seg_len;
+
+		rte_pktmbuf_free_seg(m);
+		m = m_next;
+	}
+
+	mbuf->next = NULL;
+	mbuf->nb_segs = 1;
+
+	return 0;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 98225ec80bf1..bffda1c81fbd 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2400,6 +2400,11 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 	return 0;
 }
 
+/**
+ * @internal used by rte_pktmbuf_linearize().
+ */
+int __rte_pktmbuf_linearize(struct rte_mbuf *mbuf);
+
 /**
  * Linearize data in mbuf.
  *
@@ -2415,40 +2420,9 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 static inline int
 rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 {
-	size_t seg_len, copy_len;
-	struct rte_mbuf *m;
-	struct rte_mbuf *m_next;
-	char *buffer;
-
 	if (rte_pktmbuf_is_contiguous(mbuf))
 		return 0;
-
-	/* Extend first segment to the total packet length */
-	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
-
-	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
-		return -1;
-
-	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
-	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
-
-	/* Append data from next segments to the first one */
-	m = mbuf->next;
-	while (m != NULL) {
-		m_next = m->next;
-
-		seg_len = rte_pktmbuf_data_len(m);
-		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
-		buffer += seg_len;
-
-		rte_pktmbuf_free_seg(m);
-		m = m_next;
-	}
-
-	mbuf->next = NULL;
-	mbuf->nb_segs = 1;
-
-	return 0;
+	return __rte_pktmbuf_linearize(mbuf);
 }
 
 /**
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 2662a37bf674..4d0bc9772769 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -46,6 +46,12 @@ DPDK_18.08 {
 	rte_pktmbuf_pool_create_by_ops;
 } DPDK_16.11;
 
+DPDK_19.11 {
+	global:
+
+	__rte_pktmbuf_linearize;
+} DPDK_18.08;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.20.1

[dpdk-dev] [PATCH v3 3/6] mbuf: deinline rte_pktmbuf_clone

[Stephen Hemminger]

Cloning mbufs requires allocations and iteration
and therefore should not be an inline.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 39 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 ++-------------------------
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index e2c661c97522..9a1a1b5f9468 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,45 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* Creates a shallow copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp)
+{
+	struct rte_mbuf *mc, *mi, **prev;
+	uint32_t pktlen;
+	uint16_t nseg;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	mi = mc;
+	prev = &mi->next;
+	pktlen = md->pkt_len;
+	nseg = 0;
+
+	do {
+		nseg++;
+		rte_pktmbuf_attach(mi, md);
+		*prev = mi;
+		prev = &mi->next;
+	} while ((md = md->next) != NULL &&
+	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
+
+	*prev = NULL;
+	mc->nb_segs = nseg;
+	mc->pkt_len = pktlen;
+
+	/* Allocation of new indirect segment failed */
+	if (unlikely(mi == NULL)) {
+		rte_pktmbuf_free(mc);
+		return NULL;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* convert multi-segment mbuf to single mbuf */
 int
 __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bffda1c81fbd..6133f12172ae 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1924,42 +1924,8 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
  *   - The pointer to the new "clone" mbuf on success.
  *   - NULL if allocation fails.
  */
-static inline struct rte_mbuf *rte_pktmbuf_clone(struct rte_mbuf *md,
-		struct rte_mempool *mp)
-{
-	struct rte_mbuf *mc, *mi, **prev;
-	uint32_t pktlen;
-	uint16_t nseg;
-
-	if (unlikely ((mc = rte_pktmbuf_alloc(mp)) == NULL))
-		return NULL;
-
-	mi = mc;
-	prev = &mi->next;
-	pktlen = md->pkt_len;
-	nseg = 0;
-
-	do {
-		nseg++;
-		rte_pktmbuf_attach(mi, md);
-		*prev = mi;
-		prev = &mi->next;
-	} while ((md = md->next) != NULL &&
-	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
-
-	*prev = NULL;
-	mc->nb_segs = nseg;
-	mc->pkt_len = pktlen;
-
-	/* Allocation of new indirect segment failed */
-	if (unlikely (mi == NULL)) {
-		rte_pktmbuf_free(mc);
-		return NULL;
-	}
-
-	__rte_mbuf_sanity_check(mc, 1);
-	return mc;
-}
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 4d0bc9772769..ff5c18a5559b 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -50,6 +50,7 @@ DPDK_19.11 {
 	global:
 
 	__rte_pktmbuf_linearize;
+	rte_pktmbuf_clone;
 } DPDK_18.08;
 
 EXPERIMENTAL {
-- 
2.20.1

[dpdk-dev] [PATCH v3 4/6] mbuf: add a pktmbuf copy routine

[Stephen Hemminger]

This is a commonly used operation that surprisingly the
DPDK has not supported. The new rte_pktmbuf_copy does a
deep copy of packet. This is a complete copy including
meta-data.

It handles the case where the source mbuf comes from a pool
with larger data area than the destination pool. The routine
also has options for skipping data, or truncating at a fixed
length.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 74 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 26 ++++++++++
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 101 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 9a1a1b5f9468..901df0192d2e 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -321,6 +321,80 @@ __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 	return 0;
 }
 
+/* Create a deep copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t off, uint32_t len)
+{
+	const struct rte_mbuf *seg = m;
+	struct rte_mbuf *mc, *m_last, **prev;
+
+	if (unlikely(off >= m->pkt_len))
+		return NULL;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	if (len > m->pkt_len - off)
+		len = m->pkt_len - off;
+
+	/* clone meta data from original */
+	mc->port = m->port;
+	mc->vlan_tci = m->vlan_tci;
+	mc->vlan_tci_outer = m->vlan_tci_outer;
+	mc->tx_offload = m->tx_offload;
+	mc->hash = m->hash;
+	mc->packet_type = m->packet_type;
+	mc->timestamp = m->timestamp;
+
+	/* copy private data (if any) */
+	rte_memcpy(mc + 1, m + 1,
+		   rte_pktmbuf_priv_size(mp));
+
+	prev = &mc->next;
+	m_last = mc;
+	while (len > 0) {
+		uint32_t copy_len;
+
+		while (off >= seg->data_len) {
+			off -= seg->data_len;
+			seg = seg->next;
+		}
+
+		/* current buffer is full, chain a new one */
+		if (rte_pktmbuf_tailroom(m_last) == 0) {
+			m_last = rte_pktmbuf_alloc(mp);
+			if (unlikely(m_last == NULL)) {
+				rte_pktmbuf_free(mc);
+				return NULL;
+			}
+			++mc->nb_segs;
+			*prev = m_last;
+			prev = &m_last->next;
+		}
+
+		copy_len = RTE_MIN(seg->data_len - off, len);
+		if (copy_len > rte_pktmbuf_tailroom(m_last))
+			copy_len = rte_pktmbuf_tailroom(m_last);
+
+		/* append from seg to m_last */
+		rte_memcpy(rte_pktmbuf_mtod_offset(m_last, char *,
+						   m_last->data_len),
+			   rte_pktmbuf_mtod_offset(seg, char *,
+						   off),
+			   copy_len);
+
+		m_last->data_len += copy_len;
+		mc->pkt_len += copy_len;
+		off += copy_len;
+		len -= copy_len;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 6133f12172ae..b860d570ef20 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1927,6 +1927,32 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 struct rte_mbuf *
 rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
+/**
+ * Creates a full copy of a given packet mbuf.
+ *
+ * Copies all the data from a given packet mbuf to a newly allocated
+ * set of mbufs.
+ *
+ * @param m
+ *   The packet mbuf to be cloned.
+ * @param mp
+ *   The mempool from which the "clone" mbufs are allocated.
+ * @param offset
+ *   The number of bytes to skip before copying.
+ *   If the mbuf does not have that many bytes, it is an error
+ *   and NULL is returned.
+ * @param length
+ *   The upper limit on bytes to copy.  Passing UINT32_MAX
+ *   means all data (after offset).
+ * @return
+ *   - The pointer to the new "clone" mbuf on success.
+ *   - NULL if allocation fails.
+ */
+__rte_experimental
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t offset, uint32_t length);
+
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
  *
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index ff5c18a5559b..a50dcb6db9ec 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -57,4 +57,5 @@ EXPERIMENTAL {
 	global:
 
 	rte_mbuf_check;
+	rte_pktmbuf_copy;
 } DPDK_18.08;
-- 
2.20.1

[dpdk-dev] [PATCH v3 5/6] mbuf: add pktmbuf copy test

[Stephen Hemminger]

New test for rte_pktmbuf_copy based of the clone tests.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 126 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index aafad0cf6206..49c3a5f7893c 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -399,6 +399,127 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	return -1;
 }
 
+static int
+test_pktmbuf_copy(struct rte_mempool *pktmbuf_pool)
+{
+	struct rte_mbuf *m = NULL;
+	struct rte_mbuf *copy = NULL;
+	struct rte_mbuf *copy2 = NULL;
+	unaligned_uint32_t *data;
+
+	/* alloc a mbuf */
+	m = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m == NULL)
+		GOTO_FAIL("ooops not allocating mbuf");
+
+	if (rte_pktmbuf_pkt_len(m) != 0)
+		GOTO_FAIL("Bad length");
+
+	rte_pktmbuf_append(m, sizeof(uint32_t));
+	data = rte_pktmbuf_mtod(m, unaligned_uint32_t *);
+	*data = MAGIC_DATA;
+
+	/* copy the allocated mbuf */
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (*data != MAGIC_DATA)
+		GOTO_FAIL("invalid data in copy\n");
+
+	/* free the copy */
+	rte_pktmbuf_free(copy);
+	copy = NULL;
+
+	/* same test with a chained mbuf */
+	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m->next == NULL)
+		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
+
+	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
+	*data = MAGIC_DATA + 1;
+
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA || data[1] != MAGIC_DATA + 1)
+		GOTO_FAIL("invalid data in copy\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test offset copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 sizeof(uint32_t), UINT32_MAX);
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA + 1)
+		GOTO_FAIL("copy with offset, invalid data\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test truncation copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 0, sizeof(uint32_t));
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA)
+		GOTO_FAIL("copy with truncate, invalid data\n");
+
+	/* free mbuf */
+	rte_pktmbuf_free(m);
+	rte_pktmbuf_free(copy);
+	rte_pktmbuf_free(copy2);
+
+	m = NULL;
+	copy = NULL;
+	copy2 = NULL;
+	printf("%s ok\n", __func__);
+	return 0;
+
+fail:
+	if (m)
+		rte_pktmbuf_free(m);
+	if (copy)
+		rte_pktmbuf_free(copy);
+	if (copy2)
+		rte_pktmbuf_free(copy2);
+	return -1;
+}
+
 static int
 test_attach_from_different_pool(struct rte_mempool *pktmbuf_pool,
 				struct rte_mempool *pktmbuf_pool2)
@@ -1203,6 +1324,11 @@ test_mbuf(void)
 		goto err;
 	}
 
+	if (test_pktmbuf_copy(pktmbuf_pool) < 0) {
+		printf("test_pktmbuf_copy() failed.\n");
+		goto err;
+	}
+
 	if (test_attach_from_different_pool(pktmbuf_pool, pktmbuf_pool2) < 0) {
 		printf("test_attach_from_different_pool() failed\n");
 		goto err;
-- 
2.20.1

[dpdk-dev] [PATCH v3 6/6] pdump: use new pktmbuf copy function

[Stephen Hemminger]

The rte_pktmbuf_copy handles varying size mbuf pools correctly.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_pdump/rte_pdump.c | 69 +-----------------------------------
 1 file changed, 1 insertion(+), 68 deletions(-)

diff --git a/lib/librte_pdump/rte_pdump.c b/lib/librte_pdump/rte_pdump.c
index cd24dd010951..c665cf237f65 100644
--- a/lib/librte_pdump/rte_pdump.c
+++ b/lib/librte_pdump/rte_pdump.c
@@ -64,73 +64,6 @@ static struct pdump_rxtx_cbs {
 } rx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT],
 tx_cbs[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
 
-static inline int
-pdump_pktmbuf_copy_data(struct rte_mbuf *seg, const struct rte_mbuf *m)
-{
-	if (rte_pktmbuf_tailroom(seg) < m->data_len) {
-		RTE_LOG(ERR, PDUMP,
-			"User mempool: insufficient data_len of mbuf\n");
-		return -EINVAL;
-	}
-
-	seg->port = m->port;
-	seg->vlan_tci = m->vlan_tci;
-	seg->hash = m->hash;
-	seg->tx_offload = m->tx_offload;
-	seg->ol_flags = m->ol_flags;
-	seg->packet_type = m->packet_type;
-	seg->vlan_tci_outer = m->vlan_tci_outer;
-	seg->data_len = m->data_len;
-	seg->pkt_len = seg->data_len;
-	rte_memcpy(rte_pktmbuf_mtod(seg, void *),
-			rte_pktmbuf_mtod(m, void *),
-			rte_pktmbuf_data_len(seg));
-
-	return 0;
-}
-
-static inline struct rte_mbuf *
-pdump_pktmbuf_copy(struct rte_mbuf *m, struct rte_mempool *mp)
-{
-	struct rte_mbuf *m_dup, *seg, **prev;
-	uint32_t pktlen;
-	uint16_t nseg;
-
-	m_dup = rte_pktmbuf_alloc(mp);
-	if (unlikely(m_dup == NULL))
-		return NULL;
-
-	seg = m_dup;
-	prev = &seg->next;
-	pktlen = m->pkt_len;
-	nseg = 0;
-
-	do {
-		nseg++;
-		if (pdump_pktmbuf_copy_data(seg, m) < 0) {
-			if (seg != m_dup)
-				rte_pktmbuf_free_seg(seg);
-			rte_pktmbuf_free(m_dup);
-			return NULL;
-		}
-		*prev = seg;
-		prev = &seg->next;
-	} while ((m = m->next) != NULL &&
-			(seg = rte_pktmbuf_alloc(mp)) != NULL);
-
-	*prev = NULL;
-	m_dup->nb_segs = nseg;
-	m_dup->pkt_len = pktlen;
-
-	/* Allocation of new indirect segment failed */
-	if (unlikely(seg == NULL)) {
-		rte_pktmbuf_free(m_dup);
-		return NULL;
-	}
-
-	__rte_mbuf_sanity_check(m_dup, 1);
-	return m_dup;
-}
 
 static inline void
 pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
@@ -148,7 +81,7 @@ pdump_copy(struct rte_mbuf **pkts, uint16_t nb_pkts, void *user_params)
 	ring = cbs->ring;
 	mp = cbs->mp;
 	for (i = 0; i < nb_pkts; i++) {
-		p = pdump_pktmbuf_copy(pkts[i], mp);
+		p = rte_pktmbuf_copy(pkts[i], mp, 0, UINT32_MAX);
 		if (p)
 			dup_bufs[d_pkts++] = p;
 	}
-- 
2.20.1

Re: [dpdk-dev] [PATCH v3 2/6] mbuf: delinline rte_pktmbuf_linearize

[Andrew Rybchenko]

On 9/30/19 10:20 PM, Stephen Hemminger wrote:
> This copy part of this function is too big to be put inline.
> The places it is used are only in special exception paths
> where a highly fragmented mbuf arrives at a device that can't handle it.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH v3 3/6] mbuf: deinline rte_pktmbuf_clone

[Andrew Rybchenko]

On 9/30/19 10:20 PM, Stephen Hemminger wrote:
> Cloning mbufs requires allocations and iteration
> and therefore should not be an inline.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH v3 4/6] mbuf: add a pktmbuf copy routine

[Andrew Rybchenko]

On 9/30/19 10:20 PM, Stephen Hemminger wrote:
> This is a commonly used operation that surprisingly the
> DPDK has not supported. The new rte_pktmbuf_copy does a
> deep copy of packet. This is a complete copy including
> meta-data.
>
> It handles the case where the source mbuf comes from a pool
> with larger data area than the destination pool. The routine
> also has options for skipping data, or truncating at a fixed
> length.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   lib/librte_mbuf/rte_mbuf.c           | 74 ++++++++++++++++++++++++++++
>   lib/librte_mbuf/rte_mbuf.h           | 26 ++++++++++
>   lib/librte_mbuf/rte_mbuf_version.map |  1 +
>   3 files changed, 101 insertions(+)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 9a1a1b5f9468..901df0192d2e 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -321,6 +321,80 @@ __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
>   	return 0;
>   }
>   
> +/* Create a deep copy of mbuf */
> +struct rte_mbuf *
> +rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
> +		 uint32_t off, uint32_t len)
> +{
> +	const struct rte_mbuf *seg = m;
> +	struct rte_mbuf *mc, *m_last, **prev;
> +
> +	if (unlikely(off >= m->pkt_len))
> +		return NULL;
> +
> +	mc = rte_pktmbuf_alloc(mp);
> +	if (unlikely(mc == NULL))
> +		return NULL;
> +
> +	if (len > m->pkt_len - off)
> +		len = m->pkt_len - off;
> +
> +	/* clone meta data from original */
> +	mc->port = m->port;
> +	mc->vlan_tci = m->vlan_tci;
> +	mc->vlan_tci_outer = m->vlan_tci_outer;
> +	mc->tx_offload = m->tx_offload;
> +	mc->hash = m->hash;
> +	mc->packet_type = m->packet_type;
> +	mc->timestamp = m->timestamp;

The same is done in rte_pktmbuf_attach(). May be we need a helper
function to copy meta data? Just to avoid duplication in many places.

> +
> +	/* copy private data (if any) */
> +	rte_memcpy(mc + 1, m + 1,
> +		   rte_pktmbuf_priv_size(mp));

priv_size is mempool specific and original mbuf mempool
may have smaller priv_size. I'm not sure that it is safe to copy
outsize of priv_size at least from security point of view.
So, I think it should be RTE_MIN here.

> +
> +	prev = &mc->next;
> +	m_last = mc;
> +	while (len > 0) {
> +		uint32_t copy_len;
> +
> +		while (off >= seg->data_len) {
> +			off -= seg->data_len;
> +			seg = seg->next;
> +		}
> +
> +		/* current buffer is full, chain a new one */
> +		if (rte_pktmbuf_tailroom(m_last) == 0) {
> +			m_last = rte_pktmbuf_alloc(mp);
> +			if (unlikely(m_last == NULL)) {
> +				rte_pktmbuf_free(mc);
> +				return NULL;
> +			}
> +			++mc->nb_segs;
> +			*prev = m_last;
> +			prev = &m_last->next;
> +		}
> +
> +		copy_len = RTE_MIN(seg->data_len - off, len);
> +		if (copy_len > rte_pktmbuf_tailroom(m_last))
> +			copy_len = rte_pktmbuf_tailroom(m_last);
> +
> +		/* append from seg to m_last */
> +		rte_memcpy(rte_pktmbuf_mtod_offset(m_last, char *,
> +						   m_last->data_len),
> +			   rte_pktmbuf_mtod_offset(seg, char *,
> +						   off),
> +			   copy_len);
> +
> +		m_last->data_len += copy_len;
> +		mc->pkt_len += copy_len;
> +		off += copy_len;
> +		len -= copy_len;
> +	}
> +
> +	__rte_mbuf_sanity_check(mc, 1);
> +	return mc;
> +}
> +
>   /* dump a mbuf on console */
>   void
>   rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)

[snip]

Re: [dpdk-dev] [PATCH v3 4/6] mbuf: add a pktmbuf copy routine

[Slava Ovsiienko]

Hi, Stephen

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Monday, September 30, 2019 22:21
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [PATCH v3 4/6] mbuf: add a pktmbuf copy routine
> 
> This is a commonly used operation that surprisingly the DPDK has not
> supported. The new rte_pktmbuf_copy does a deep copy of packet. This is a
> complete copy including meta-data.

There is mbuf dynamic fields coming [1], these ones also should be copied.
Is this patch planned to be updated?

[1] http://patches.dpdk.org/patch/59343/

[snip]

WBR, Slava

Re: [dpdk-dev] [PATCH v3 4/6] mbuf: add a pktmbuf copy routine

[Stephen Hemminger]

On Tue, 1 Oct 2019 17:36:10 +0000
Slava Ovsiienko <viacheslavo@mellanox.com> wrote:

> Hi, Stephen
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> > Sent: Monday, September 30, 2019 22:21
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>
> > Subject: [dpdk-dev] [PATCH v3 4/6] mbuf: add a pktmbuf copy routine
> > 
> > This is a commonly used operation that surprisingly the DPDK has not
> > supported. The new rte_pktmbuf_copy does a deep copy of packet. This is a
> > complete copy including meta-data.  
> 
> There is mbuf dynamic fields coming [1], these ones also should be copied.
> Is this patch planned to be updated?
> 
> [1] http://patches.dpdk.org/patch/59343/
> 
> [snip]
> 
> WBR, Slava
> 

If this is merged, then it is responsibility of the dynamic mbuf
patch submitter to update. If dynamic mbuf makes it in, then this
patch can be updated.

[dpdk-dev] [PATCH v4 0/4] mbuf copy/cloning enhancements

[Stephen Hemminger]

This patch set is all about improving the mbuf related cloning
and copying. They are motivated by seeing issues with mbuf copying
in rte_pdump and realized this a wider and more general problem.
The pdump copy could not handle different size pools and did
not handle meta data, etc.

They cause no functional or ABI changes. The only visible part
to older code is converting a couple of inlines to real functions.
This kind of change confuses checkpatch which thinks these new
functions should be marked experimental when they must not be.

v4 - common mbuf header fields copy routine
v3 - split linearize into internal/external
     copy private data in pktmbuf_copy
v2 - add pdump use of pktmbuf_copy
     fix version in map


Stephen Hemminger (4):
  mbuf: don't generate invalid mbuf in clone test
  mbuf: delinline rte_pktmbuf_linearize
  mbuf: deinline rte_pktmbuf_clone
  mbuf: add a pktmbuf copy routine

 app/test/test_mbuf.c                 |   3 +
 lib/librte_mbuf/rte_mbuf.c           | 145 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 127 +++++++++--------------
 lib/librte_mbuf/rte_mbuf_version.map |   8 ++
 4 files changed, 206 insertions(+), 77 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v4 1/4] mbuf: don't generate invalid mbuf in clone test

[Stephen Hemminger]

The test for cloning changed mbuf would generate an mbuf
whose length and segments were invalid. This would cause a crash
if test was run with mbuf debugging enabled.

Fixes: f1022aba76a5 ("app/test: rename mbuf variable")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 2a97afe2044a..aafad0cf6206 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -332,8 +332,11 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
 	if (m->next == NULL)
 		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
 
 	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+
 	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
 	*data = MAGIC_DATA;
 
-- 
2.20.1

[dpdk-dev] [PATCH v4 2/4] mbuf: delinline rte_pktmbuf_linearize

[Stephen Hemminger]

This copy part of this function is too big to be put inline.
The places it is used are only in special exception paths
where a highly fragmented mbuf arrives at a device that can't handle it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_mbuf/rte_mbuf.c           | 37 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 +++++-----------------------
 lib/librte_mbuf/rte_mbuf_version.map |  6 +++++
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 37718d49c148..e2c661c97522 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,43 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* convert multi-segment mbuf to single mbuf */
+int
+__rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
+{
+	size_t seg_len, copy_len;
+	struct rte_mbuf *m;
+	struct rte_mbuf *m_next;
+	char *buffer;
+
+	/* Extend first segment to the total packet length */
+	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
+
+	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
+		return -1;
+
+	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
+	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
+
+	/* Append data from next segments to the first one */
+	m = mbuf->next;
+	while (m != NULL) {
+		m_next = m->next;
+
+		seg_len = rte_pktmbuf_data_len(m);
+		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
+		buffer += seg_len;
+
+		rte_pktmbuf_free_seg(m);
+		m = m_next;
+	}
+
+	mbuf->next = NULL;
+	mbuf->nb_segs = 1;
+
+	return 0;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 98225ec80bf1..bffda1c81fbd 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2400,6 +2400,11 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 	return 0;
 }
 
+/**
+ * @internal used by rte_pktmbuf_linearize().
+ */
+int __rte_pktmbuf_linearize(struct rte_mbuf *mbuf);
+
 /**
  * Linearize data in mbuf.
  *
@@ -2415,40 +2420,9 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 static inline int
 rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 {
-	size_t seg_len, copy_len;
-	struct rte_mbuf *m;
-	struct rte_mbuf *m_next;
-	char *buffer;
-
 	if (rte_pktmbuf_is_contiguous(mbuf))
 		return 0;
-
-	/* Extend first segment to the total packet length */
-	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
-
-	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
-		return -1;
-
-	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
-	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
-
-	/* Append data from next segments to the first one */
-	m = mbuf->next;
-	while (m != NULL) {
-		m_next = m->next;
-
-		seg_len = rte_pktmbuf_data_len(m);
-		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
-		buffer += seg_len;
-
-		rte_pktmbuf_free_seg(m);
-		m = m_next;
-	}
-
-	mbuf->next = NULL;
-	mbuf->nb_segs = 1;
-
-	return 0;
+	return __rte_pktmbuf_linearize(mbuf);
 }
 
 /**
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 2662a37bf674..4d0bc9772769 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -46,6 +46,12 @@ DPDK_18.08 {
 	rte_pktmbuf_pool_create_by_ops;
 } DPDK_16.11;
 
+DPDK_19.11 {
+	global:
+
+	__rte_pktmbuf_linearize;
+} DPDK_18.08;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.20.1

[dpdk-dev] [PATCH v4 3/4] mbuf: deinline rte_pktmbuf_clone

[Stephen Hemminger]

Cloning mbufs requires allocations and iteration
and therefore should not be an inline.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_mbuf/rte_mbuf.c           | 39 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 ++-------------------------
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index e2c661c97522..9a1a1b5f9468 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,45 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* Creates a shallow copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp)
+{
+	struct rte_mbuf *mc, *mi, **prev;
+	uint32_t pktlen;
+	uint16_t nseg;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	mi = mc;
+	prev = &mi->next;
+	pktlen = md->pkt_len;
+	nseg = 0;
+
+	do {
+		nseg++;
+		rte_pktmbuf_attach(mi, md);
+		*prev = mi;
+		prev = &mi->next;
+	} while ((md = md->next) != NULL &&
+	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
+
+	*prev = NULL;
+	mc->nb_segs = nseg;
+	mc->pkt_len = pktlen;
+
+	/* Allocation of new indirect segment failed */
+	if (unlikely(mi == NULL)) {
+		rte_pktmbuf_free(mc);
+		return NULL;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* convert multi-segment mbuf to single mbuf */
 int
 __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bffda1c81fbd..6133f12172ae 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1924,42 +1924,8 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
  *   - The pointer to the new "clone" mbuf on success.
  *   - NULL if allocation fails.
  */
-static inline struct rte_mbuf *rte_pktmbuf_clone(struct rte_mbuf *md,
-		struct rte_mempool *mp)
-{
-	struct rte_mbuf *mc, *mi, **prev;
-	uint32_t pktlen;
-	uint16_t nseg;
-
-	if (unlikely ((mc = rte_pktmbuf_alloc(mp)) == NULL))
-		return NULL;
-
-	mi = mc;
-	prev = &mi->next;
-	pktlen = md->pkt_len;
-	nseg = 0;
-
-	do {
-		nseg++;
-		rte_pktmbuf_attach(mi, md);
-		*prev = mi;
-		prev = &mi->next;
-	} while ((md = md->next) != NULL &&
-	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
-
-	*prev = NULL;
-	mc->nb_segs = nseg;
-	mc->pkt_len = pktlen;
-
-	/* Allocation of new indirect segment failed */
-	if (unlikely (mi == NULL)) {
-		rte_pktmbuf_free(mc);
-		return NULL;
-	}
-
-	__rte_mbuf_sanity_check(mc, 1);
-	return mc;
-}
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 4d0bc9772769..ff5c18a5559b 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -50,6 +50,7 @@ DPDK_19.11 {
 	global:
 
 	__rte_pktmbuf_linearize;
+	rte_pktmbuf_clone;
 } DPDK_18.08;
 
 EXPERIMENTAL {
-- 
2.20.1

[dpdk-dev] [PATCH v4 4/4] mbuf: add a pktmbuf copy routine

[Stephen Hemminger]

This is a commonly used operation that surprisingly the
DPDK has not supported. The new rte_pktmbuf_copy does a
deep copy of packet. This is a complete copy including
meta-data.

It handles the case where the source mbuf comes from a pool
with larger data area than the destination pool. The routine
also has options for skipping data, or truncating at a fixed
length.

This patch also introduces internal inline to copy the
metadata fields of mbuf.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 69 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 53 +++++++++++++++++----
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 113 insertions(+), 10 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 9a1a1b5f9468..d5c9488fa213 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -321,6 +321,75 @@ __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 	return 0;
 }
 
+/* Create a deep copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t off, uint32_t len)
+{
+	const struct rte_mbuf *seg = m;
+	struct rte_mbuf *mc, *m_last, **prev;
+	uint16_t priv_size;
+
+	if (unlikely(off >= m->pkt_len))
+		return NULL;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	if (len > m->pkt_len - off)
+		len = m->pkt_len - off;
+
+	__rte_pktmbuf_copy_hdr(mc, m);
+
+	/* copy private area (can be 0) */
+	priv_size = RTE_MIN(rte_pktmbuf_priv_size(mp),
+			    rte_pktmbuf_priv_size(m->pool));
+	rte_memcpy(mc + 1, m + 1, priv_size);
+
+	prev = &mc->next;
+	m_last = mc;
+	while (len > 0) {
+		uint32_t copy_len;
+
+		while (off >= seg->data_len) {
+			off -= seg->data_len;
+			seg = seg->next;
+		}
+
+		/* current buffer is full, chain a new one */
+		if (rte_pktmbuf_tailroom(m_last) == 0) {
+			m_last = rte_pktmbuf_alloc(mp);
+			if (unlikely(m_last == NULL)) {
+				rte_pktmbuf_free(mc);
+				return NULL;
+			}
+			++mc->nb_segs;
+			*prev = m_last;
+			prev = &m_last->next;
+		}
+
+		copy_len = RTE_MIN(seg->data_len - off, len);
+		if (copy_len > rte_pktmbuf_tailroom(m_last))
+			copy_len = rte_pktmbuf_tailroom(m_last);
+
+		/* append from seg to m_last */
+		rte_memcpy(rte_pktmbuf_mtod_offset(m_last, char *,
+						   m_last->data_len),
+			   rte_pktmbuf_mtod_offset(seg, char *,
+						   off),
+			   copy_len);
+
+		m_last->data_len += copy_len;
+		mc->pkt_len += copy_len;
+		off += copy_len;
+		len -= copy_len;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 6133f12172ae..803ddf28496e 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1684,6 +1684,19 @@ rte_pktmbuf_attach_extbuf(struct rte_mbuf *m, void *buf_addr,
  */
 #define rte_pktmbuf_detach_extbuf(m) rte_pktmbuf_detach(m)
 
+/* internal */
+static inline void
+__rte_pktmbuf_copy_hdr(struct rte_mbuf *mi, const struct rte_mbuf *m)
+{
+	mi->port = m->port;
+	mi->vlan_tci = m->vlan_tci;
+	mi->vlan_tci_outer = m->vlan_tci_outer;
+	mi->tx_offload = m->tx_offload;
+	mi->hash = m->hash;
+	mi->packet_type = m->packet_type;
+	mi->timestamp = m->timestamp;
+}
+
 /**
  * Attach packet mbuf to another packet mbuf.
  *
@@ -1721,23 +1734,17 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
 		mi->ol_flags = m->ol_flags | IND_ATTACHED_MBUF;
 	}
 
-	mi->buf_iova = m->buf_iova;
-	mi->buf_addr = m->buf_addr;
-	mi->buf_len = m->buf_len;
+	__rte_pktmbuf_copy_hdr(mi, m);
 
 	mi->data_off = m->data_off;
 	mi->data_len = m->data_len;
-	mi->port = m->port;
-	mi->vlan_tci = m->vlan_tci;
-	mi->vlan_tci_outer = m->vlan_tci_outer;
-	mi->tx_offload = m->tx_offload;
-	mi->hash = m->hash;
+	mi->buf_iova = m->buf_iova;
+	mi->buf_addr = m->buf_addr;
+	mi->buf_len = m->buf_len;
 
 	mi->next = NULL;
 	mi->pkt_len = mi->data_len;
 	mi->nb_segs = 1;
-	mi->packet_type = m->packet_type;
-	mi->timestamp = m->timestamp;
 
 	__rte_mbuf_sanity_check(mi, 1);
 	__rte_mbuf_sanity_check(m, 0);
@@ -1927,6 +1934,32 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 struct rte_mbuf *
 rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
+/**
+ * Creates a full copy of a given packet mbuf.
+ *
+ * Copies all the data from a given packet mbuf to a newly allocated
+ * set of mbufs.
+ *
+ * @param m
+ *   The packet mbuf to be cloned.
+ * @param mp
+ *   The mempool from which the "clone" mbufs are allocated.
+ * @param offset
+ *   The number of bytes to skip before copying.
+ *   If the mbuf does not have that many bytes, it is an error
+ *   and NULL is returned.
+ * @param length
+ *   The upper limit on bytes to copy.  Passing UINT32_MAX
+ *   means all data (after offset).
+ * @return
+ *   - The pointer to the new "clone" mbuf on success.
+ *   - NULL if allocation fails.
+ */
+__rte_experimental
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t offset, uint32_t length);
+
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
  *
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index ff5c18a5559b..a50dcb6db9ec 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -57,4 +57,5 @@ EXPERIMENTAL {
 	global:
 
 	rte_mbuf_check;
+	rte_pktmbuf_copy;
 } DPDK_18.08;
-- 
2.20.1

[dpdk-dev] [PATCH v5 0/5] mbuf copy/cloning enhancements

[Stephen Hemminger]

This patch set is all about improving the mbuf related cloning
and copying. They are motivated by seeing issues with mbuf copying
in rte_pdump and realized this a wider and more general problem.
The pdump copy could not handle different size pools and did
not handle meta data, etc.

They cause no functional or ABI changes. The only visible part
to older code is converting a couple of inlines to real functions.
This kind of change confuses checkpatch which thinks these new
functions should be marked experimental when they must not be.

v5 - add back the test (dropped by accident in v4)
v4 - common mbuf header fields copy routine
v3 - split linearize into internal/external
     copy private data in pktmbuf_copy
v2 - add pdump use of pktmbuf_copy
     fix version in map

Stephen Hemminger (5):
  mbuf: don't generate invalid mbuf in clone test
  mbuf: delinline rte_pktmbuf_linearize
  mbuf: deinline rte_pktmbuf_clone
  mbuf: add a pktmbuf copy routine
  mbuf: add pktmbuf copy test

 app/test/test_mbuf.c                 | 129 ++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.c           | 145 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 127 +++++++++--------------
 lib/librte_mbuf/rte_mbuf_version.map |   8 ++
 4 files changed, 332 insertions(+), 77 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v5 1/5] mbuf: don't generate invalid mbuf in clone test

[Stephen Hemminger]

The test for cloning changed mbuf would generate an mbuf
whose length and segments were invalid. This would cause a crash
if test was run with mbuf debugging enabled.

Fixes: f1022aba76a5 ("app/test: rename mbuf variable")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 2a97afe2044a..aafad0cf6206 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -332,8 +332,11 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
 	if (m->next == NULL)
 		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
 
 	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+
 	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
 	*data = MAGIC_DATA;
 
-- 
2.20.1

[dpdk-dev] [PATCH v5 2/5] mbuf: delinline rte_pktmbuf_linearize

[Stephen Hemminger]

This copy part of this function is too big to be put inline.
The places it is used are only in special exception paths
where a highly fragmented mbuf arrives at a device that can't handle it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_mbuf/rte_mbuf.c           | 37 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 +++++-----------------------
 lib/librte_mbuf/rte_mbuf_version.map |  6 +++++
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 37718d49c148..e2c661c97522 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,43 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* convert multi-segment mbuf to single mbuf */
+int
+__rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
+{
+	size_t seg_len, copy_len;
+	struct rte_mbuf *m;
+	struct rte_mbuf *m_next;
+	char *buffer;
+
+	/* Extend first segment to the total packet length */
+	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
+
+	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
+		return -1;
+
+	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
+	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
+
+	/* Append data from next segments to the first one */
+	m = mbuf->next;
+	while (m != NULL) {
+		m_next = m->next;
+
+		seg_len = rte_pktmbuf_data_len(m);
+		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
+		buffer += seg_len;
+
+		rte_pktmbuf_free_seg(m);
+		m = m_next;
+	}
+
+	mbuf->next = NULL;
+	mbuf->nb_segs = 1;
+
+	return 0;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 98225ec80bf1..bffda1c81fbd 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2400,6 +2400,11 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 	return 0;
 }
 
+/**
+ * @internal used by rte_pktmbuf_linearize().
+ */
+int __rte_pktmbuf_linearize(struct rte_mbuf *mbuf);
+
 /**
  * Linearize data in mbuf.
  *
@@ -2415,40 +2420,9 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 static inline int
 rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 {
-	size_t seg_len, copy_len;
-	struct rte_mbuf *m;
-	struct rte_mbuf *m_next;
-	char *buffer;
-
 	if (rte_pktmbuf_is_contiguous(mbuf))
 		return 0;
-
-	/* Extend first segment to the total packet length */
-	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
-
-	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
-		return -1;
-
-	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
-	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
-
-	/* Append data from next segments to the first one */
-	m = mbuf->next;
-	while (m != NULL) {
-		m_next = m->next;
-
-		seg_len = rte_pktmbuf_data_len(m);
-		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
-		buffer += seg_len;
-
-		rte_pktmbuf_free_seg(m);
-		m = m_next;
-	}
-
-	mbuf->next = NULL;
-	mbuf->nb_segs = 1;
-
-	return 0;
+	return __rte_pktmbuf_linearize(mbuf);
 }
 
 /**
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 2662a37bf674..4d0bc9772769 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -46,6 +46,12 @@ DPDK_18.08 {
 	rte_pktmbuf_pool_create_by_ops;
 } DPDK_16.11;
 
+DPDK_19.11 {
+	global:
+
+	__rte_pktmbuf_linearize;
+} DPDK_18.08;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.20.1

[dpdk-dev] [PATCH v5 3/5] mbuf: deinline rte_pktmbuf_clone

[Stephen Hemminger]

Cloning mbufs requires allocations and iteration
and therefore should not be an inline.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_mbuf/rte_mbuf.c           | 39 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 ++-------------------------
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index e2c661c97522..9a1a1b5f9468 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,45 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* Creates a shallow copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp)
+{
+	struct rte_mbuf *mc, *mi, **prev;
+	uint32_t pktlen;
+	uint16_t nseg;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	mi = mc;
+	prev = &mi->next;
+	pktlen = md->pkt_len;
+	nseg = 0;
+
+	do {
+		nseg++;
+		rte_pktmbuf_attach(mi, md);
+		*prev = mi;
+		prev = &mi->next;
+	} while ((md = md->next) != NULL &&
+	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
+
+	*prev = NULL;
+	mc->nb_segs = nseg;
+	mc->pkt_len = pktlen;
+
+	/* Allocation of new indirect segment failed */
+	if (unlikely(mi == NULL)) {
+		rte_pktmbuf_free(mc);
+		return NULL;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* convert multi-segment mbuf to single mbuf */
 int
 __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bffda1c81fbd..6133f12172ae 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1924,42 +1924,8 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
  *   - The pointer to the new "clone" mbuf on success.
  *   - NULL if allocation fails.
  */
-static inline struct rte_mbuf *rte_pktmbuf_clone(struct rte_mbuf *md,
-		struct rte_mempool *mp)
-{
-	struct rte_mbuf *mc, *mi, **prev;
-	uint32_t pktlen;
-	uint16_t nseg;
-
-	if (unlikely ((mc = rte_pktmbuf_alloc(mp)) == NULL))
-		return NULL;
-
-	mi = mc;
-	prev = &mi->next;
-	pktlen = md->pkt_len;
-	nseg = 0;
-
-	do {
-		nseg++;
-		rte_pktmbuf_attach(mi, md);
-		*prev = mi;
-		prev = &mi->next;
-	} while ((md = md->next) != NULL &&
-	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
-
-	*prev = NULL;
-	mc->nb_segs = nseg;
-	mc->pkt_len = pktlen;
-
-	/* Allocation of new indirect segment failed */
-	if (unlikely (mi == NULL)) {
-		rte_pktmbuf_free(mc);
-		return NULL;
-	}
-
-	__rte_mbuf_sanity_check(mc, 1);
-	return mc;
-}
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 4d0bc9772769..ff5c18a5559b 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -50,6 +50,7 @@ DPDK_19.11 {
 	global:
 
 	__rte_pktmbuf_linearize;
+	rte_pktmbuf_clone;
 } DPDK_18.08;
 
 EXPERIMENTAL {
-- 
2.20.1

[dpdk-dev] [PATCH v5 4/5] mbuf: add a pktmbuf copy routine

[Stephen Hemminger]

This is a commonly used operation that surprisingly the
DPDK has not supported. The new rte_pktmbuf_copy does a
deep copy of packet. This is a complete copy including
meta-data.

It handles the case where the source mbuf comes from a pool
with larger data area than the destination pool. The routine
also has options for skipping data, or truncating at a fixed
length.

This patch also introduces internal inline to copy the
metadata fields of mbuf.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 69 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 53 +++++++++++++++++----
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 113 insertions(+), 10 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 9a1a1b5f9468..d5c9488fa213 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -321,6 +321,75 @@ __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 	return 0;
 }
 
+/* Create a deep copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t off, uint32_t len)
+{
+	const struct rte_mbuf *seg = m;
+	struct rte_mbuf *mc, *m_last, **prev;
+	uint16_t priv_size;
+
+	if (unlikely(off >= m->pkt_len))
+		return NULL;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	if (len > m->pkt_len - off)
+		len = m->pkt_len - off;
+
+	__rte_pktmbuf_copy_hdr(mc, m);
+
+	/* copy private area (can be 0) */
+	priv_size = RTE_MIN(rte_pktmbuf_priv_size(mp),
+			    rte_pktmbuf_priv_size(m->pool));
+	rte_memcpy(mc + 1, m + 1, priv_size);
+
+	prev = &mc->next;
+	m_last = mc;
+	while (len > 0) {
+		uint32_t copy_len;
+
+		while (off >= seg->data_len) {
+			off -= seg->data_len;
+			seg = seg->next;
+		}
+
+		/* current buffer is full, chain a new one */
+		if (rte_pktmbuf_tailroom(m_last) == 0) {
+			m_last = rte_pktmbuf_alloc(mp);
+			if (unlikely(m_last == NULL)) {
+				rte_pktmbuf_free(mc);
+				return NULL;
+			}
+			++mc->nb_segs;
+			*prev = m_last;
+			prev = &m_last->next;
+		}
+
+		copy_len = RTE_MIN(seg->data_len - off, len);
+		if (copy_len > rte_pktmbuf_tailroom(m_last))
+			copy_len = rte_pktmbuf_tailroom(m_last);
+
+		/* append from seg to m_last */
+		rte_memcpy(rte_pktmbuf_mtod_offset(m_last, char *,
+						   m_last->data_len),
+			   rte_pktmbuf_mtod_offset(seg, char *,
+						   off),
+			   copy_len);
+
+		m_last->data_len += copy_len;
+		mc->pkt_len += copy_len;
+		off += copy_len;
+		len -= copy_len;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 6133f12172ae..803ddf28496e 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1684,6 +1684,19 @@ rte_pktmbuf_attach_extbuf(struct rte_mbuf *m, void *buf_addr,
  */
 #define rte_pktmbuf_detach_extbuf(m) rte_pktmbuf_detach(m)
 
+/* internal */
+static inline void
+__rte_pktmbuf_copy_hdr(struct rte_mbuf *mi, const struct rte_mbuf *m)
+{
+	mi->port = m->port;
+	mi->vlan_tci = m->vlan_tci;
+	mi->vlan_tci_outer = m->vlan_tci_outer;
+	mi->tx_offload = m->tx_offload;
+	mi->hash = m->hash;
+	mi->packet_type = m->packet_type;
+	mi->timestamp = m->timestamp;
+}
+
 /**
  * Attach packet mbuf to another packet mbuf.
  *
@@ -1721,23 +1734,17 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
 		mi->ol_flags = m->ol_flags | IND_ATTACHED_MBUF;
 	}
 
-	mi->buf_iova = m->buf_iova;
-	mi->buf_addr = m->buf_addr;
-	mi->buf_len = m->buf_len;
+	__rte_pktmbuf_copy_hdr(mi, m);
 
 	mi->data_off = m->data_off;
 	mi->data_len = m->data_len;
-	mi->port = m->port;
-	mi->vlan_tci = m->vlan_tci;
-	mi->vlan_tci_outer = m->vlan_tci_outer;
-	mi->tx_offload = m->tx_offload;
-	mi->hash = m->hash;
+	mi->buf_iova = m->buf_iova;
+	mi->buf_addr = m->buf_addr;
+	mi->buf_len = m->buf_len;
 
 	mi->next = NULL;
 	mi->pkt_len = mi->data_len;
 	mi->nb_segs = 1;
-	mi->packet_type = m->packet_type;
-	mi->timestamp = m->timestamp;
 
 	__rte_mbuf_sanity_check(mi, 1);
 	__rte_mbuf_sanity_check(m, 0);
@@ -1927,6 +1934,32 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 struct rte_mbuf *
 rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
+/**
+ * Creates a full copy of a given packet mbuf.
+ *
+ * Copies all the data from a given packet mbuf to a newly allocated
+ * set of mbufs.
+ *
+ * @param m
+ *   The packet mbuf to be cloned.
+ * @param mp
+ *   The mempool from which the "clone" mbufs are allocated.
+ * @param offset
+ *   The number of bytes to skip before copying.
+ *   If the mbuf does not have that many bytes, it is an error
+ *   and NULL is returned.
+ * @param length
+ *   The upper limit on bytes to copy.  Passing UINT32_MAX
+ *   means all data (after offset).
+ * @return
+ *   - The pointer to the new "clone" mbuf on success.
+ *   - NULL if allocation fails.
+ */
+__rte_experimental
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t offset, uint32_t length);
+
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
  *
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index ff5c18a5559b..a50dcb6db9ec 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -57,4 +57,5 @@ EXPERIMENTAL {
 	global:
 
 	rte_mbuf_check;
+	rte_pktmbuf_copy;
 } DPDK_18.08;
-- 
2.20.1

[dpdk-dev] [PATCH v5 5/5] mbuf: add pktmbuf copy test

[Stephen Hemminger]

New test for rte_pktmbuf_copy based of the clone tests.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test/test_mbuf.c | 126 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index aafad0cf6206..49c3a5f7893c 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -399,6 +399,127 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	return -1;
 }
 
+static int
+test_pktmbuf_copy(struct rte_mempool *pktmbuf_pool)
+{
+	struct rte_mbuf *m = NULL;
+	struct rte_mbuf *copy = NULL;
+	struct rte_mbuf *copy2 = NULL;
+	unaligned_uint32_t *data;
+
+	/* alloc a mbuf */
+	m = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m == NULL)
+		GOTO_FAIL("ooops not allocating mbuf");
+
+	if (rte_pktmbuf_pkt_len(m) != 0)
+		GOTO_FAIL("Bad length");
+
+	rte_pktmbuf_append(m, sizeof(uint32_t));
+	data = rte_pktmbuf_mtod(m, unaligned_uint32_t *);
+	*data = MAGIC_DATA;
+
+	/* copy the allocated mbuf */
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (*data != MAGIC_DATA)
+		GOTO_FAIL("invalid data in copy\n");
+
+	/* free the copy */
+	rte_pktmbuf_free(copy);
+	copy = NULL;
+
+	/* same test with a chained mbuf */
+	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m->next == NULL)
+		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
+
+	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
+	*data = MAGIC_DATA + 1;
+
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA || data[1] != MAGIC_DATA + 1)
+		GOTO_FAIL("invalid data in copy\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test offset copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 sizeof(uint32_t), UINT32_MAX);
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA + 1)
+		GOTO_FAIL("copy with offset, invalid data\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test truncation copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 0, sizeof(uint32_t));
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA)
+		GOTO_FAIL("copy with truncate, invalid data\n");
+
+	/* free mbuf */
+	rte_pktmbuf_free(m);
+	rte_pktmbuf_free(copy);
+	rte_pktmbuf_free(copy2);
+
+	m = NULL;
+	copy = NULL;
+	copy2 = NULL;
+	printf("%s ok\n", __func__);
+	return 0;
+
+fail:
+	if (m)
+		rte_pktmbuf_free(m);
+	if (copy)
+		rte_pktmbuf_free(copy);
+	if (copy2)
+		rte_pktmbuf_free(copy2);
+	return -1;
+}
+
 static int
 test_attach_from_different_pool(struct rte_mempool *pktmbuf_pool,
 				struct rte_mempool *pktmbuf_pool2)
@@ -1203,6 +1324,11 @@ test_mbuf(void)
 		goto err;
 	}
 
+	if (test_pktmbuf_copy(pktmbuf_pool) < 0) {
+		printf("test_pktmbuf_copy() failed.\n");
+		goto err;
+	}
+
 	if (test_attach_from_different_pool(pktmbuf_pool, pktmbuf_pool2) < 0) {
 		printf("test_attach_from_different_pool() failed\n");
 		goto err;
-- 
2.20.1

Re: [dpdk-dev] [PATCH v5 1/5] mbuf: don't generate invalid mbuf in clone test

[Olivier Matz]

On Mon, Oct 07, 2019 at 08:43:39AM -0700, Stephen Hemminger wrote:
> The test for cloning changed mbuf would generate an mbuf
> whose length and segments were invalid. This would cause a crash
> if test was run with mbuf debugging enabled.
> 
> Fixes: f1022aba76a5 ("app/test: rename mbuf variable")
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v5 2/5] mbuf: delinline rte_pktmbuf_linearize

[Olivier Matz]

On Mon, Oct 07, 2019 at 08:43:40AM -0700, Stephen Hemminger wrote:
> This copy part of this function is too big to be put inline.
> The places it is used are only in special exception paths
> where a highly fragmented mbuf arrives at a device that can't handle it.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v5 3/5] mbuf: deinline rte_pktmbuf_clone

[Olivier Matz]

On Mon, Oct 07, 2019 at 08:43:41AM -0700, Stephen Hemminger wrote:
> Cloning mbufs requires allocations and iteration
> and therefore should not be an inline.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v5 4/5] mbuf: add a pktmbuf copy routine

[Olivier Matz]

Hi Stephen,

Thank you for this patch. Few comments below.

On Mon, Oct 07, 2019 at 08:43:42AM -0700, Stephen Hemminger wrote:
> This is a commonly used operation that surprisingly the
> DPDK has not supported. The new rte_pktmbuf_copy does a
> deep copy of packet. This is a complete copy including
> meta-data.
> 
> It handles the case where the source mbuf comes from a pool
> with larger data area than the destination pool. The routine
> also has options for skipping data, or truncating at a fixed
> length.
> 
> This patch also introduces internal inline to copy the
> metadata fields of mbuf.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_mbuf/rte_mbuf.c           | 69 ++++++++++++++++++++++++++++
>  lib/librte_mbuf/rte_mbuf.h           | 53 +++++++++++++++++----
>  lib/librte_mbuf/rte_mbuf_version.map |  1 +
>  3 files changed, 113 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> index 9a1a1b5f9468..d5c9488fa213 100644
> --- a/lib/librte_mbuf/rte_mbuf.c
> +++ b/lib/librte_mbuf/rte_mbuf.c
> @@ -321,6 +321,75 @@ __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
>  	return 0;
>  }
>  
> +/* Create a deep copy of mbuf */
> +struct rte_mbuf *
> +rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
> +		 uint32_t off, uint32_t len)
> +{
> +	const struct rte_mbuf *seg = m;
> +	struct rte_mbuf *mc, *m_last, **prev;
> +	uint16_t priv_size;
> +
> +	if (unlikely(off >= m->pkt_len))
> +		return NULL;
> +
> +	mc = rte_pktmbuf_alloc(mp);
> +	if (unlikely(mc == NULL))
> +		return NULL;
> +
> +	if (len > m->pkt_len - off)
> +		len = m->pkt_len - off;
> +
> +	__rte_pktmbuf_copy_hdr(mc, m);

I think ol_flags should be copied too.
Take care to not copy the IND_ATTACHED_MBUF bit.

> +
> +	/* copy private area (can be 0) */
> +	priv_size = RTE_MIN(rte_pktmbuf_priv_size(mp),
> +			    rte_pktmbuf_priv_size(m->pool));
> +	rte_memcpy(mc + 1, m + 1, priv_size);

I'm not sure doing the copy on the MIN() length is a good solution.
Nothing states that the priv area of different mbuf pools are compatible.

I suggest to drop the priv copy, like it's done for the clone. An
application can provide its own functions for copy and clone, and can
adapt the priv copy to its needs.

An alternative is to allocate from the same pool than m, but I prefer
the first solution.

> +
> +	prev = &mc->next;
> +	m_last = mc;
> +	while (len > 0) {
> +		uint32_t copy_len;
> +
> +		while (off >= seg->data_len) {
> +			off -= seg->data_len;
> +			seg = seg->next;
> +		}
> +
> +		/* current buffer is full, chain a new one */
> +		if (rte_pktmbuf_tailroom(m_last) == 0) {
> +			m_last = rte_pktmbuf_alloc(mp);
> +			if (unlikely(m_last == NULL)) {
> +				rte_pktmbuf_free(mc);
> +				return NULL;
> +			}

For segments 2 to n, the headroom is useless. An optimization could
be to remove the headroom here.

> +			++mc->nb_segs;
> +			*prev = m_last;
> +			prev = &m_last->next;
> +		}
> +
> +		copy_len = RTE_MIN(seg->data_len - off, len);
> +		if (copy_len > rte_pktmbuf_tailroom(m_last))
> +			copy_len = rte_pktmbuf_tailroom(m_last);
> +		/* append from seg to m_last */
> +		rte_memcpy(rte_pktmbuf_mtod_offset(m_last, char *,
> +						   m_last->data_len),
> +			   rte_pktmbuf_mtod_offset(seg, char *,
> +						   off),
> +			   copy_len);
> +
> +		m_last->data_len += copy_len;
> +		mc->pkt_len += copy_len;
> +		off += copy_len;
> +		len -= copy_len;
> +	}
> +
> +	__rte_mbuf_sanity_check(mc, 1);
> +	return mc;
> +}
> +
>  /* dump a mbuf on console */
>  void
>  rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 6133f12172ae..803ddf28496e 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -1684,6 +1684,19 @@ rte_pktmbuf_attach_extbuf(struct rte_mbuf *m, void *buf_addr,
>   */
>  #define rte_pktmbuf_detach_extbuf(m) rte_pktmbuf_detach(m)
>  
> +/* internal */
> +static inline void
> +__rte_pktmbuf_copy_hdr(struct rte_mbuf *mi, const struct rte_mbuf *m)
> +{
> +	mi->port = m->port;
> +	mi->vlan_tci = m->vlan_tci;
> +	mi->vlan_tci_outer = m->vlan_tci_outer;
> +	mi->tx_offload = m->tx_offload;
> +	mi->hash = m->hash;
> +	mi->packet_type = m->packet_type;
> +	mi->timestamp = m->timestamp;
> +}
> +

The names "dst" and "src" looks clearer than "mi" (for indirect mbuf)
and "m", knowing that this function can deal with 2 direct mbufs.


>  /**
>   * Attach packet mbuf to another packet mbuf.
>   *
> @@ -1721,23 +1734,17 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
>  		mi->ol_flags = m->ol_flags | IND_ATTACHED_MBUF;
>  	}
>  
> -	mi->buf_iova = m->buf_iova;
> -	mi->buf_addr = m->buf_addr;
> -	mi->buf_len = m->buf_len;
> +	__rte_pktmbuf_copy_hdr(mi, m);
>  
>  	mi->data_off = m->data_off;
>  	mi->data_len = m->data_len;
> -	mi->port = m->port;
> -	mi->vlan_tci = m->vlan_tci;
> -	mi->vlan_tci_outer = m->vlan_tci_outer;
> -	mi->tx_offload = m->tx_offload;
> -	mi->hash = m->hash;
> +	mi->buf_iova = m->buf_iova;
> +	mi->buf_addr = m->buf_addr;
> +	mi->buf_len = m->buf_len;
>  
>  	mi->next = NULL;
>  	mi->pkt_len = mi->data_len;
>  	mi->nb_segs = 1;
> -	mi->packet_type = m->packet_type;
> -	mi->timestamp = m->timestamp;
>  
>  	__rte_mbuf_sanity_check(mi, 1);
>  	__rte_mbuf_sanity_check(m, 0);
> @@ -1927,6 +1934,32 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
>  struct rte_mbuf *
>  rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
>  
> +/**
> + * Creates a full copy of a given packet mbuf.

Although it's not consistent everywhere, I think the imperative
form is preferred ("Create" instead of "Creates").

> + * Copies all the data from a given packet mbuf to a newly allocated
> + * set of mbufs.

Same here

> + *
> + * @param m
> + *   The packet mbuf to be cloned.

cloned -> copied (there are some other occurrences of "clone" below)

> + * @param mp
> + *   The mempool from which the "clone" mbufs are allocated.
> + * @param offset
> + *   The number of bytes to skip before copying.
> + *   If the mbuf does not have that many bytes, it is an error
> + *   and NULL is returned.
> + * @param length
> + *   The upper limit on bytes to copy.  Passing UINT32_MAX
> + *   means all data (after offset).
> + * @return
> + *   - The pointer to the new "clone" mbuf on success.
> + *   - NULL if allocation fails.
> + */
> +__rte_experimental
> +struct rte_mbuf *
> +rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
> +		 uint32_t offset, uint32_t length);
> +
>  /**
>   * Adds given value to the refcnt of all packet mbuf segments.
>   *
> diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
> index ff5c18a5559b..a50dcb6db9ec 100644
> --- a/lib/librte_mbuf/rte_mbuf_version.map
> +++ b/lib/librte_mbuf/rte_mbuf_version.map
> @@ -57,4 +57,5 @@ EXPERIMENTAL {
>  	global:
>  
>  	rte_mbuf_check;
> +	rte_pktmbuf_copy;
>  } DPDK_18.08;
> -- 
> 2.20.1
> 

Re: [dpdk-dev] [PATCH v5 5/5] mbuf: add pktmbuf copy test

[Olivier Matz]

On Mon, Oct 07, 2019 at 08:43:43AM -0700, Stephen Hemminger wrote:
> New test for rte_pktmbuf_copy based of the clone tests.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v5 4/5] mbuf: add a pktmbuf copy routine

[Stephen Hemminger]

On Tue, 8 Oct 2019 11:03:34 +0200
Olivier Matz <olivier.matz@6wind.com> wrote:

> >  
> > +/**
> > + * Creates a full copy of a given packet mbuf.  
> 
> Although it's not consistent everywhere, I think the imperative
> form is preferred ("Create" instead of "Creates").
> 
> > + * Copies all the data from a given packet mbuf to a newly allocated
> > + * set of mbufs.  
> 
> Same here
> 
> > + *
> > + * @param m
> > + *   The packet mbuf to be cloned.  
> 
> cloned -> copied (there are some other occurrences of "clone" below)


This comment was modified from the mbuf_clone description.
The two should use same grammar. Don't really care which way
but should be consistent between these two.

Will fix both to match your suggestion.

[dpdk-dev] [PATCH v6 0/5] mbuf: copy/cloning enhancements

[Stephen Hemminger]

This patch set is all about improving the mbuf related cloning
and copying. They are motivated by seeing issues with mbuf copying
in rte_pdump and realized this a wider and more general problem.
The pdump copy could not handle different size pools and did
not handle meta data, etc.

They cause no functional or ABI changes. The only visible part
to older code is converting a couple of inlines to real functions.
This kind of change confuses checkpatch which thinks these new
functions should be marked experimental when they must not be.

v6 - incorporate feedback from Oliver for pktmbuf_copy
     add more comments and tests for pktmbuf_copy

v5 - add back the test (dropped by accident in v4)
v4 - common mbuf header fields copy routine
v3 - split linearize into internal/external
     copy private data in pktmbuf_copy
v2 - add pdump use of pktmbuf_copy
     fix version in map

Stephen Hemminger (5):
  mbuf: don't generate invalid mbuf in clone test
  mbuf: delinline rte_pktmbuf_linearize
  mbuf: deinline rte_pktmbuf_clone
  mbuf: add a pktmbuf copy routine
  mbuf: add pktmbuf copy test

 app/test/test_mbuf.c                 | 160 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.c           | 153 +++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 129 +++++++++------------
 lib/librte_mbuf/rte_mbuf_version.map |   8 ++
 4 files changed, 372 insertions(+), 78 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v6 1/5] mbuf: don't generate invalid mbuf in clone test

[Stephen Hemminger]

The test for cloning changed mbuf would generate an mbuf
whose length and segments were invalid. This would cause a crash
if test was run with mbuf debugging enabled.

Fixes: f1022aba76a5 ("app/test: rename mbuf variable")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_mbuf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 2a97afe2044a..aafad0cf6206 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -332,8 +332,11 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
 	if (m->next == NULL)
 		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
 
 	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+
 	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
 	*data = MAGIC_DATA;
 
-- 
2.20.1

[dpdk-dev] [PATCH v6 2/5] mbuf: delinline rte_pktmbuf_linearize

[Stephen Hemminger]

This copy part of this function is too big to be put inline.
The places it is used are only in special exception paths
where a highly fragmented mbuf arrives at a device that can't handle it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_mbuf/rte_mbuf.c           | 37 +++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 +++++-----------------------
 lib/librte_mbuf/rte_mbuf_version.map |  6 +++++
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 37718d49c148..e2c661c97522 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,43 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* convert multi-segment mbuf to single mbuf */
+int
+__rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
+{
+	size_t seg_len, copy_len;
+	struct rte_mbuf *m;
+	struct rte_mbuf *m_next;
+	char *buffer;
+
+	/* Extend first segment to the total packet length */
+	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
+
+	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
+		return -1;
+
+	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
+	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
+
+	/* Append data from next segments to the first one */
+	m = mbuf->next;
+	while (m != NULL) {
+		m_next = m->next;
+
+		seg_len = rte_pktmbuf_data_len(m);
+		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
+		buffer += seg_len;
+
+		rte_pktmbuf_free_seg(m);
+		m = m_next;
+	}
+
+	mbuf->next = NULL;
+	mbuf->nb_segs = 1;
+
+	return 0;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 98225ec80bf1..bffda1c81fbd 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2400,6 +2400,11 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 	return 0;
 }
 
+/**
+ * @internal used by rte_pktmbuf_linearize().
+ */
+int __rte_pktmbuf_linearize(struct rte_mbuf *mbuf);
+
 /**
  * Linearize data in mbuf.
  *
@@ -2415,40 +2420,9 @@ rte_validate_tx_offload(const struct rte_mbuf *m)
 static inline int
 rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 {
-	size_t seg_len, copy_len;
-	struct rte_mbuf *m;
-	struct rte_mbuf *m_next;
-	char *buffer;
-
 	if (rte_pktmbuf_is_contiguous(mbuf))
 		return 0;
-
-	/* Extend first segment to the total packet length */
-	copy_len = rte_pktmbuf_pkt_len(mbuf) - rte_pktmbuf_data_len(mbuf);
-
-	if (unlikely(copy_len > rte_pktmbuf_tailroom(mbuf)))
-		return -1;
-
-	buffer = rte_pktmbuf_mtod_offset(mbuf, char *, mbuf->data_len);
-	mbuf->data_len = (uint16_t)(mbuf->pkt_len);
-
-	/* Append data from next segments to the first one */
-	m = mbuf->next;
-	while (m != NULL) {
-		m_next = m->next;
-
-		seg_len = rte_pktmbuf_data_len(m);
-		rte_memcpy(buffer, rte_pktmbuf_mtod(m, char *), seg_len);
-		buffer += seg_len;
-
-		rte_pktmbuf_free_seg(m);
-		m = m_next;
-	}
-
-	mbuf->next = NULL;
-	mbuf->nb_segs = 1;
-
-	return 0;
+	return __rte_pktmbuf_linearize(mbuf);
 }
 
 /**
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 2662a37bf674..4d0bc9772769 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -46,6 +46,12 @@ DPDK_18.08 {
 	rte_pktmbuf_pool_create_by_ops;
 } DPDK_16.11;
 
+DPDK_19.11 {
+	global:
+
+	__rte_pktmbuf_linearize;
+} DPDK_18.08;
+
 EXPERIMENTAL {
 	global:
 
-- 
2.20.1

[dpdk-dev] [PATCH v6 3/5] mbuf: deinline rte_pktmbuf_clone

[Stephen Hemminger]

Cloning mbufs requires allocations and iteration
and therefore should not be an inline.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_mbuf/rte_mbuf.c           | 39 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 38 ++-------------------------
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index e2c661c97522..9a1a1b5f9468 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -245,6 +245,45 @@ int rte_mbuf_check(const struct rte_mbuf *m, int is_header,
 	return 0;
 }
 
+/* Creates a shallow copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp)
+{
+	struct rte_mbuf *mc, *mi, **prev;
+	uint32_t pktlen;
+	uint16_t nseg;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	mi = mc;
+	prev = &mi->next;
+	pktlen = md->pkt_len;
+	nseg = 0;
+
+	do {
+		nseg++;
+		rte_pktmbuf_attach(mi, md);
+		*prev = mi;
+		prev = &mi->next;
+	} while ((md = md->next) != NULL &&
+	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
+
+	*prev = NULL;
+	mc->nb_segs = nseg;
+	mc->pkt_len = pktlen;
+
+	/* Allocation of new indirect segment failed */
+	if (unlikely(mi == NULL)) {
+		rte_pktmbuf_free(mc);
+		return NULL;
+	}
+
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* convert multi-segment mbuf to single mbuf */
 int
 __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bffda1c81fbd..6133f12172ae 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1924,42 +1924,8 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
  *   - The pointer to the new "clone" mbuf on success.
  *   - NULL if allocation fails.
  */
-static inline struct rte_mbuf *rte_pktmbuf_clone(struct rte_mbuf *md,
-		struct rte_mempool *mp)
-{
-	struct rte_mbuf *mc, *mi, **prev;
-	uint32_t pktlen;
-	uint16_t nseg;
-
-	if (unlikely ((mc = rte_pktmbuf_alloc(mp)) == NULL))
-		return NULL;
-
-	mi = mc;
-	prev = &mi->next;
-	pktlen = md->pkt_len;
-	nseg = 0;
-
-	do {
-		nseg++;
-		rte_pktmbuf_attach(mi, md);
-		*prev = mi;
-		prev = &mi->next;
-	} while ((md = md->next) != NULL &&
-	    (mi = rte_pktmbuf_alloc(mp)) != NULL);
-
-	*prev = NULL;
-	mc->nb_segs = nseg;
-	mc->pkt_len = pktlen;
-
-	/* Allocation of new indirect segment failed */
-	if (unlikely (mi == NULL)) {
-		rte_pktmbuf_free(mc);
-		return NULL;
-	}
-
-	__rte_mbuf_sanity_check(mc, 1);
-	return mc;
-}
+struct rte_mbuf *
+rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index 4d0bc9772769..ff5c18a5559b 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -50,6 +50,7 @@ DPDK_19.11 {
 	global:
 
 	__rte_pktmbuf_linearize;
+	rte_pktmbuf_clone;
 } DPDK_18.08;
 
 EXPERIMENTAL {
-- 
2.20.1

[dpdk-dev] [PATCH v6 4/5] mbuf: add a pktmbuf copy routine

[Stephen Hemminger]

This is a commonly used operation that surprisingly the
DPDK has not supported. The new rte_pktmbuf_copy does a
deep copy of packet. This is a complete copy including
meta-data.

It handles the case where the source mbuf comes from a pool
with larger data area than the destination pool. The routine
also has options for skipping data, or truncating at a fixed
length.

This patch also introduces internal inline to copy the
metadata fields of mbuf.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_mbuf/rte_mbuf.c           | 77 ++++++++++++++++++++++++++++
 lib/librte_mbuf/rte_mbuf.h           | 55 ++++++++++++++++----
 lib/librte_mbuf/rte_mbuf_version.map |  1 +
 3 files changed, 122 insertions(+), 11 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 9a1a1b5f9468..0236fba76bbc 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -321,6 +321,83 @@ __rte_pktmbuf_linearize(struct rte_mbuf *mbuf)
 	return 0;
 }
 
+/* Create a deep copy of mbuf */
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t off, uint32_t len)
+{
+	const struct rte_mbuf *seg = m;
+	struct rte_mbuf *mc, *m_last, **prev;
+
+	/* garbage in check */
+	__rte_mbuf_sanity_check(m, 1);
+
+	/* check for request to copy at offset past end of mbuf */
+	if (unlikely(off >= m->pkt_len))
+		return NULL;
+
+	mc = rte_pktmbuf_alloc(mp);
+	if (unlikely(mc == NULL))
+		return NULL;
+
+	/* truncate requested length to available data */
+	if (len > m->pkt_len - off)
+		len = m->pkt_len - off;
+
+	__rte_pktmbuf_copy_hdr(mc, m);
+
+	/* copied mbuf is not indirect or external */
+	mc->ol_flags = m->ol_flags & ~(IND_ATTACHED_MBUF|EXT_ATTACHED_MBUF);
+
+	prev = &mc->next;
+	m_last = mc;
+	while (len > 0) {
+		uint32_t copy_len;
+
+		/* skip leading mbuf segments */
+		while (off >= seg->data_len) {
+			off -= seg->data_len;
+			seg = seg->next;
+		}
+
+		/* current buffer is full, chain a new one */
+		if (rte_pktmbuf_tailroom(m_last) == 0) {
+			m_last = rte_pktmbuf_alloc(mp);
+			if (unlikely(m_last == NULL)) {
+				rte_pktmbuf_free(mc);
+				return NULL;
+			}
+			++mc->nb_segs;
+			*prev = m_last;
+			prev = &m_last->next;
+		}
+
+		/*
+		 * copy the min of data in input segment (seg)
+		 * vs space available in output (m_last)
+		 */
+		copy_len = RTE_MIN(seg->data_len - off, len);
+		if (copy_len > rte_pktmbuf_tailroom(m_last))
+			copy_len = rte_pktmbuf_tailroom(m_last);
+
+		/* append from seg to m_last */
+		rte_memcpy(rte_pktmbuf_mtod_offset(m_last, char *,
+						   m_last->data_len),
+			   rte_pktmbuf_mtod_offset(seg, char *, off),
+			   copy_len);
+
+		/* update offsets and lengths */
+		m_last->data_len += copy_len;
+		mc->pkt_len += copy_len;
+		off += copy_len;
+		len -= copy_len;
+	}
+
+	/* garbage out check */
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /* dump a mbuf on console */
 void
 rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len)
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 6133f12172ae..fb0849ac1473 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1684,6 +1684,19 @@ rte_pktmbuf_attach_extbuf(struct rte_mbuf *m, void *buf_addr,
  */
 #define rte_pktmbuf_detach_extbuf(m) rte_pktmbuf_detach(m)
 
+/* internal */
+static inline void
+__rte_pktmbuf_copy_hdr(struct rte_mbuf *mdst, const struct rte_mbuf *msrc)
+{
+	mdst->port = msrc->port;
+	mdst->vlan_tci = msrc->vlan_tci;
+	mdst->vlan_tci_outer = msrc->vlan_tci_outer;
+	mdst->tx_offload = msrc->tx_offload;
+	mdst->hash = msrc->hash;
+	mdst->packet_type = msrc->packet_type;
+	mdst->timestamp = msrc->timestamp;
+}
+
 /**
  * Attach packet mbuf to another packet mbuf.
  *
@@ -1721,23 +1734,17 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
 		mi->ol_flags = m->ol_flags | IND_ATTACHED_MBUF;
 	}
 
-	mi->buf_iova = m->buf_iova;
-	mi->buf_addr = m->buf_addr;
-	mi->buf_len = m->buf_len;
+	__rte_pktmbuf_copy_hdr(mi, m);
 
 	mi->data_off = m->data_off;
 	mi->data_len = m->data_len;
-	mi->port = m->port;
-	mi->vlan_tci = m->vlan_tci;
-	mi->vlan_tci_outer = m->vlan_tci_outer;
-	mi->tx_offload = m->tx_offload;
-	mi->hash = m->hash;
+	mi->buf_iova = m->buf_iova;
+	mi->buf_addr = m->buf_addr;
+	mi->buf_len = m->buf_len;
 
 	mi->next = NULL;
 	mi->pkt_len = mi->data_len;
 	mi->nb_segs = 1;
-	mi->packet_type = m->packet_type;
-	mi->timestamp = m->timestamp;
 
 	__rte_mbuf_sanity_check(mi, 1);
 	__rte_mbuf_sanity_check(m, 0);
@@ -1908,7 +1915,7 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 }
 
 /**
- * Creates a "clone" of the given packet mbuf.
+ * Create a "clone" of the given packet mbuf.
  *
  * Walks through all segments of the given packet mbuf, and for each of them:
  *  - Creates a new packet mbuf from the given pool.
@@ -1927,6 +1934,32 @@ static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 struct rte_mbuf *
 rte_pktmbuf_clone(struct rte_mbuf *md, struct rte_mempool *mp);
 
+/**
+ * Create a full copy of a given packet mbuf.
+ *
+ * Copies all the data from a given packet mbuf to a newly allocated
+ * set of mbufs. The private data are is not copied.
+ *
+ * @param m
+ *   The packet mbuf to be copiedd.
+ * @param mp
+ *   The mempool from which the "clone" mbufs are allocated.
+ * @param offset
+ *   The number of bytes to skip before copying.
+ *   If the mbuf does not have that many bytes, it is an error
+ *   and NULL is returned.
+ * @param length
+ *   The upper limit on bytes to copy.  Passing UINT32_MAX
+ *   means all data (after offset).
+ * @return
+ *   - The pointer to the new "clone" mbuf on success.
+ *   - NULL if allocation fails.
+ */
+__rte_experimental
+struct rte_mbuf *
+rte_pktmbuf_copy(const struct rte_mbuf *m, struct rte_mempool *mp,
+		 uint32_t offset, uint32_t length);
+
 /**
  * Adds given value to the refcnt of all packet mbuf segments.
  *
diff --git a/lib/librte_mbuf/rte_mbuf_version.map b/lib/librte_mbuf/rte_mbuf_version.map
index ff5c18a5559b..a50dcb6db9ec 100644
--- a/lib/librte_mbuf/rte_mbuf_version.map
+++ b/lib/librte_mbuf/rte_mbuf_version.map
@@ -57,4 +57,5 @@ EXPERIMENTAL {
 	global:
 
 	rte_mbuf_check;
+	rte_pktmbuf_copy;
 } DPDK_18.08;
-- 
2.20.1

[dpdk-dev] [PATCH v6 5/5] mbuf: add pktmbuf copy test

[Stephen Hemminger]

New test for rte_pktmbuf_copy based of the clone tests.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_mbuf.c | 157 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 157 insertions(+)

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index aafad0cf6206..0df6b0e12a39 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -399,6 +399,158 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
 	return -1;
 }
 
+static int
+test_pktmbuf_copy(struct rte_mempool *pktmbuf_pool)
+{
+	struct rte_mbuf *m = NULL;
+	struct rte_mbuf *copy = NULL;
+	struct rte_mbuf *copy2 = NULL;
+	struct rte_mbuf *clone = NULL;
+	unaligned_uint32_t *data;
+
+	/* alloc a mbuf */
+	m = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m == NULL)
+		GOTO_FAIL("ooops not allocating mbuf");
+
+	if (rte_pktmbuf_pkt_len(m) != 0)
+		GOTO_FAIL("Bad length");
+
+	rte_pktmbuf_append(m, sizeof(uint32_t));
+	data = rte_pktmbuf_mtod(m, unaligned_uint32_t *);
+	*data = MAGIC_DATA;
+
+	/* copy the allocated mbuf */
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (*data != MAGIC_DATA)
+		GOTO_FAIL("invalid data in copy\n");
+
+	/* free the copy */
+	rte_pktmbuf_free(copy);
+	copy = NULL;
+
+	/* same test with a cloned mbuf */
+	clone = rte_pktmbuf_clone(m, pktmbuf_pool);
+	if (clone == NULL)
+		GOTO_FAIL("cannot clone data\n");
+
+	if (!RTE_MBUF_CLONED(clone))
+		GOTO_FAIL("clone did not give a cloned mbuf\n");
+
+	copy = rte_pktmbuf_copy(clone, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy cloned mbuf\n");
+
+	if (RTE_MBUF_CLONED(copy))
+		GOTO_FAIL("copy of clone is cloned?\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy clone length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != sizeof(uint32_t))
+		GOTO_FAIL("copy clone data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (*data != MAGIC_DATA)
+		GOTO_FAIL("invalid data in clone copy\n");
+	rte_pktmbuf_free(clone);
+	rte_pktmbuf_free(copy);
+	copy = NULL;
+	clone = NULL;
+
+
+	/* same test with a chained mbuf */
+	m->next = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m->next == NULL)
+		GOTO_FAIL("Next Pkt Null\n");
+	m->nb_segs = 2;
+
+	rte_pktmbuf_append(m->next, sizeof(uint32_t));
+	m->pkt_len = 2 * sizeof(uint32_t);
+	data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
+	*data = MAGIC_DATA + 1;
+
+	copy = rte_pktmbuf_copy(m, pktmbuf_pool, 0, UINT32_MAX);
+	if (copy == NULL)
+		GOTO_FAIL("cannot copy data\n");
+
+	if (rte_pktmbuf_pkt_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy) != 2 * sizeof(uint32_t))
+		GOTO_FAIL("chain copy data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA || data[1] != MAGIC_DATA + 1)
+		GOTO_FAIL("invalid data in copy\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test offset copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 sizeof(uint32_t), UINT32_MAX);
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with offset, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA + 1)
+		GOTO_FAIL("copy with offset, invalid data\n");
+
+	rte_pktmbuf_free(copy2);
+
+	/* test truncation copy */
+	copy2 = rte_pktmbuf_copy(copy, pktmbuf_pool,
+				 0, sizeof(uint32_t));
+	if (copy2 == NULL)
+		GOTO_FAIL("cannot copy the copy\n");
+
+	if (rte_pktmbuf_pkt_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, length incorrect\n");
+
+	if (rte_pktmbuf_data_len(copy2) != sizeof(uint32_t))
+		GOTO_FAIL("copy with truncate, data length incorrect\n");
+
+	data = rte_pktmbuf_mtod(copy2, unaligned_uint32_t *);
+	if (data[0] != MAGIC_DATA)
+		GOTO_FAIL("copy with truncate, invalid data\n");
+
+	/* free mbuf */
+	rte_pktmbuf_free(m);
+	rte_pktmbuf_free(copy);
+	rte_pktmbuf_free(copy2);
+
+	m = NULL;
+	copy = NULL;
+	copy2 = NULL;
+	printf("%s ok\n", __func__);
+	return 0;
+
+fail:
+	if (m)
+		rte_pktmbuf_free(m);
+	if (copy)
+		rte_pktmbuf_free(copy);
+	if (copy2)
+		rte_pktmbuf_free(copy2);
+	return -1;
+}
+
 static int
 test_attach_from_different_pool(struct rte_mempool *pktmbuf_pool,
 				struct rte_mempool *pktmbuf_pool2)
@@ -1203,6 +1355,11 @@ test_mbuf(void)
 		goto err;
 	}
 
+	if (test_pktmbuf_copy(pktmbuf_pool) < 0) {
+		printf("test_pktmbuf_copy() failed.\n");
+		goto err;
+	}
+
 	if (test_attach_from_different_pool(pktmbuf_pool, pktmbuf_pool2) < 0) {
 		printf("test_attach_from_different_pool() failed\n");
 		goto err;
-- 
2.20.1

Re: [dpdk-dev] [PATCH v6 4/5] mbuf: add a pktmbuf copy routine

[Olivier Matz]

On Tue, Oct 08, 2019 at 09:33:49AM -0700, Stephen Hemminger wrote:
> This is a commonly used operation that surprisingly the
> DPDK has not supported. The new rte_pktmbuf_copy does a
> deep copy of packet. This is a complete copy including
> meta-data.
> 
> It handles the case where the source mbuf comes from a pool
> with larger data area than the destination pool. The routine
> also has options for skipping data, or truncating at a fixed
> length.
> 
> This patch also introduces internal inline to copy the
> metadata fields of mbuf.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v6 1/5] mbuf: don't generate invalid mbuf in clone test

[David Marchand]

On Tue, Oct 8, 2019 at 6:34 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The test for cloning changed mbuf would generate an mbuf
> whose length and segments were invalid. This would cause a crash
> if test was run with mbuf debugging enabled.

It could be interesting to add calls to rte_mbuf_check() (not
dependent on the debug build option) at key points in this test.

>
> Fixes: f1022aba76a5 ("app/test: rename mbuf variable")

Wrong Fixes: tag.

Fixes: 4ccd2bb3a9e2 ("app/test: enhance mbuf refcnt check")
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  app/test/test_mbuf.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index 2a97afe2044a..aafad0cf6206 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -332,8 +332,11 @@ testclone_testupdate_testdetach(struct rte_mempool *pktmbuf_pool)
>         m->next = rte_pktmbuf_alloc(pktmbuf_pool);
>         if (m->next == NULL)
>                 GOTO_FAIL("Next Pkt Null\n");
> +       m->nb_segs = 2;
>
>         rte_pktmbuf_append(m->next, sizeof(uint32_t));
> +       m->pkt_len = 2 * sizeof(uint32_t);
> +
>         data = rte_pktmbuf_mtod(m->next, unaligned_uint32_t *);
>         *data = MAGIC_DATA;
>
> --
> 2.20.1
>

Applied, thanks.


--
David Marchand

Re: [dpdk-dev] [PATCH v6 2/5] mbuf: delinline rte_pktmbuf_linearize

[David Marchand]

On Tue, Oct 8, 2019 at 6:34 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> This copy part of this function is too big to be put inline.
> The places it is used are only in special exception paths
> where a highly fragmented mbuf arrives at a device that can't handle it.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks.


--
David Marchand

Re: [dpdk-dev] [PATCH v6 3/5] mbuf: deinline rte_pktmbuf_clone

[David Marchand]

On Tue, Oct 8, 2019 at 6:34 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> Cloning mbufs requires allocations and iteration
> and therefore should not be an inline.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks.


--
David Marchand

Re: [dpdk-dev] [PATCH v6 4/5] mbuf: add a pktmbuf copy routine

[David Marchand]

On Wed, Oct 16, 2019 at 8:58 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> On Tue, Oct 08, 2019 at 09:33:49AM -0700, Stephen Hemminger wrote:
> > This is a commonly used operation that surprisingly the
> > DPDK has not supported. The new rte_pktmbuf_copy does a
> > deep copy of packet. This is a complete copy including
> > meta-data.
> >
> > It handles the case where the source mbuf comes from a pool
> > with larger data area than the destination pool. The routine
> > also has options for skipping data, or truncating at a fixed
> > length.
> >
> > This patch also introduces internal inline to copy the
> > metadata fields of mbuf.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Squashed with next patch, since a separate patch for the test added not much.

Applied, thanks.



--
David Marchand