[dpdk-dev] [PATCH 0/2] mbuf: improvements

[dpdk-dev] [PATCH 0/2] mbuf: improvements

[Stephen Hemminger]

These are a couple of small patches we use to help
with mbuf handling.

Stephen Hemminger (2):
  mbuf: Add rte_pktmbuf_copy
  mbuf: make sure userdata is initialized

 drivers/net/cxgbe/sge.c               |  1 +
 drivers/net/e1000/em_rxtx.c           |  2 ++
 drivers/net/e1000/igb_rxtx.c          |  2 ++
 drivers/net/enic/enic_main.c          |  2 ++
 drivers/net/fm10k/fm10k.h             |  1 +
 drivers/net/i40e/i40e_rxtx.c          |  4 +++
 drivers/net/ixgbe/ixgbe_rxtx.c        |  4 +++
 drivers/net/ixgbe/ixgbe_rxtx_vec.c    |  1 +
 drivers/net/virtio/virtio_rxtx.c      |  3 ++
 drivers/net/vmxnet3/vmxnet3_rxtx.c    |  1 +
 drivers/net/xenvirt/rte_eth_xenvirt.c |  1 +
 lib/librte_mbuf/rte_mbuf.h            | 60 +++++++++++++++++++++++++++++++++++
 12 files changed, 82 insertions(+)

-- 
2.1.4

[dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

Added rte_pktmbuf_copy() function since copying multi-part
segments is common issue and can be problematic.

Signed-off-by: Mike Davison <mdavison@brocade.com>
Reviewed-by: Stephen Hemminger <shemming@brocade.com>
---
 lib/librte_mbuf/rte_mbuf.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 80419df..f0a543b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -60,6 +60,7 @@
 #include <rte_atomic.h>
 #include <rte_prefetch.h>
 #include <rte_branch_prediction.h>
+#include <rte_memcpy.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
 	return !!(m->nb_segs == 1);
 }
 
+/*
+ * Creates a copy 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.
+ *  - Copy segment to newly created mbuf.
+ * Then updates pkt_len and nb_segs of the new packet mbuf to match values
+ * from the original packet mbuf.
+ *
+ * @param md
+ *   The packet mbuf to be copied.
+ * @param mp
+ *   The mempool from which the mbufs are allocated.
+ * @return
+ *   - The pointer to the new mbuf on success.
+ *   - NULL if allocation fails.
+ */
+static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
+		struct rte_mempool *mp)
+{
+	struct rte_mbuf *mc = NULL;
+	struct rte_mbuf **prev = &mc;
+
+	do {
+		struct rte_mbuf *mi;
+
+		mi = rte_pktmbuf_alloc(mp);
+		if (unlikely(mi == NULL)) {
+			rte_pktmbuf_free(mc);
+			return NULL;
+		}
+
+		mi->data_off = md->data_off;
+		mi->data_len = md->data_len;
+		mi->port = md->port;
+		mi->vlan_tci = md->vlan_tci;
+		mi->tx_offload = md->tx_offload;
+		mi->hash = md->hash;
+
+		mi->next = NULL;
+		mi->pkt_len = md->pkt_len;
+		mi->nb_segs = md->nb_segs;
+		mi->ol_flags = md->ol_flags;
+		mi->packet_type = md->packet_type;
+
+		rte_memcpy(rte_pktmbuf_mtod(mi, char *),
+			   rte_pktmbuf_mtod(md, char *),
+			   md->data_len);
+
+		*prev = mi;
+		prev = &mi->next;
+	} while ((md = md->next) != NULL);
+
+	*prev = NULL;
+	__rte_mbuf_sanity_check(mc, 1);
+	return mc;
+}
+
 /**
  * Dump an mbuf structure to the console.
  *
-- 
2.1.4

[dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

For applications that use m->userdata the initialization can
be a signficant (10%) performance penalty.

Rather than taking the cache penalty of initializing userdata
in the receive handling, do it in the place where mbuf is
already cache hot and being setup.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/cxgbe/sge.c               | 1 +
 drivers/net/e1000/em_rxtx.c           | 2 ++
 drivers/net/e1000/igb_rxtx.c          | 2 ++
 drivers/net/enic/enic_main.c          | 2 ++
 drivers/net/fm10k/fm10k.h             | 1 +
 drivers/net/i40e/i40e_rxtx.c          | 4 ++++
 drivers/net/ixgbe/ixgbe_rxtx.c        | 4 ++++
 drivers/net/ixgbe/ixgbe_rxtx_vec.c    | 1 +
 drivers/net/virtio/virtio_rxtx.c      | 3 +++
 drivers/net/vmxnet3/vmxnet3_rxtx.c    | 1 +
 drivers/net/xenvirt/rte_eth_xenvirt.c | 1 +
 lib/librte_mbuf/rte_mbuf.h            | 1 +
 12 files changed, 23 insertions(+)

diff --git a/drivers/net/cxgbe/sge.c b/drivers/net/cxgbe/sge.c
index 359296e..c51ed39 100644
--- a/drivers/net/cxgbe/sge.c
+++ b/drivers/net/cxgbe/sge.c
@@ -406,6 +406,7 @@ static unsigned int refill_fl_usembufs(struct adapter *adap, struct sge_fl *q,
 		}
 
 		mbuf->data_off = RTE_PKTMBUF_HEADROOM;
+		mbuf->userdata = NULL;
 		mbuf->next = NULL;
 
 		mapping = (dma_addr_t)(mbuf->buf_physaddr + mbuf->data_off);
diff --git a/drivers/net/e1000/em_rxtx.c b/drivers/net/e1000/em_rxtx.c
index 38f5c3b..a2fd522 100644
--- a/drivers/net/e1000/em_rxtx.c
+++ b/drivers/net/e1000/em_rxtx.c
@@ -790,6 +790,7 @@ eth_em_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 				rxq->crc_len);
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
 		rte_packet_prefetch((char *)rxm->buf_addr + rxm->data_off);
+		rxm->userdata = NULL;
 		rxm->nb_segs = 1;
 		rxm->next = NULL;
 		rxm->pkt_len = pkt_len;
@@ -959,6 +960,7 @@ eth_em_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		data_len = rte_le_to_cpu_16(rxd.length);
 		rxm->data_len = data_len;
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 
 		/*
 		 * If this is the first buffer of the received packet,
diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index 43d6703..9fc0690 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -774,6 +774,7 @@ eth_igb_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		pkt_len = (uint16_t) (rte_le_to_cpu_16(rxd.wb.upper.length) -
 				      rxq->crc_len);
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 		rte_packet_prefetch((char *)rxm->buf_addr + rxm->data_off);
 		rxm->nb_segs = 1;
 		rxm->next = NULL;
@@ -948,6 +949,7 @@ eth_igb_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
 		rxm->data_len = data_len;
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 
 		/*
 		 * If this is the first buffer of the received packet,
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 15313c2..8cf5948 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -342,6 +342,7 @@ static int enic_rq_alloc_buf(struct vnic_rq *rq)
 		}
 
 		hdr_mbuf->data_off = RTE_PKTMBUF_HEADROOM;
+		hdr_mbuf->userdata = NULL;
 
 		hdr_mbuf->nb_segs = 2;
 		hdr_mbuf->port = enic->port_id;
@@ -363,6 +364,7 @@ static int enic_rq_alloc_buf(struct vnic_rq *rq)
 	}
 
 	mbuf->data_off = RTE_PKTMBUF_HEADROOM;
+	mbuf->userdata = NULL;
 	mbuf->next = NULL;
 
 	dma_addr = (dma_addr_t)
diff --git a/drivers/net/fm10k/fm10k.h b/drivers/net/fm10k/fm10k.h
index c089882..c9d9147 100644
--- a/drivers/net/fm10k/fm10k.h
+++ b/drivers/net/fm10k/fm10k.h
@@ -257,6 +257,7 @@ fm10k_pktmbuf_reset(struct rte_mbuf *mb, uint8_t in_port)
 	mb->data_off = (uint16_t)(RTE_PTR_ALIGN((char *)mb->buf_addr +
 			RTE_PKTMBUF_HEADROOM, FM10K_RX_DATABUF_ALIGN)
 			- (char *)mb->buf_addr);
+	mb->userdata = NULL;
 	mb->port = in_port;
 }
 
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 88b015d..177b823 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -804,6 +804,7 @@ i40e_rx_alloc_bufs(struct i40e_rx_queue *rxq)
 		rte_mbuf_refcnt_set(mb, 1);
 		mb->next = NULL;
 		mb->data_off = RTE_PKTMBUF_HEADROOM;
+		mb->userdata = NULL;
 		mb->nb_segs = 1;
 		mb->port = rxq->port_id;
 		dma_addr = rte_cpu_to_le_64(\
@@ -961,6 +962,7 @@ i40e_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 				I40E_RXD_QW1_LENGTH_PBUF_SHIFT) - rxq->crc_len;
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 		rte_prefetch0(RTE_PTR_ADD(rxm->buf_addr, RTE_PKTMBUF_HEADROOM));
 		rxm->nb_segs = 1;
 		rxm->next = NULL;
@@ -1069,6 +1071,7 @@ i40e_recv_scattered_pkts(void *rx_queue,
 					I40E_RXD_QW1_LENGTH_PBUF_SHIFT;
 		rxm->data_len = rx_packet_len;
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 
 		/**
 		 * If this is the first buffer of the received packet, set the
@@ -2436,6 +2439,7 @@ i40e_alloc_rx_queue_mbufs(struct i40e_rx_queue *rxq)
 		rte_mbuf_refcnt_set(mbuf, 1);
 		mbuf->next = NULL;
 		mbuf->data_off = RTE_PKTMBUF_HEADROOM;
+		mbuf->userdata = NULL;
 		mbuf->nb_segs = 1;
 		mbuf->port = rxq->port_id;
 
diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
index b1db57f..558c039 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx.c
@@ -1054,6 +1054,7 @@ ixgbe_rx_alloc_bufs(struct ixgbe_rx_queue *rxq, bool reset_mbuf)
 
 		rte_mbuf_refcnt_set(mb, 1);
 		mb->data_off = RTE_PKTMBUF_HEADROOM;
+		mb->userdata = NULL;
 
 		/* populate the descriptors */
 		dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb));
@@ -1321,6 +1322,7 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		pkt_len = (uint16_t) (rte_le_to_cpu_16(rxd.wb.upper.length) -
 				      rxq->crc_len);
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 		rte_packet_prefetch((char *)rxm->buf_addr + rxm->data_off);
 		rxm->nb_segs = 1;
 		rxm->next = NULL;
@@ -1596,6 +1598,7 @@ next_desc:
 			rxe->mbuf = nmb;
 
 			rxm->data_off = RTE_PKTMBUF_HEADROOM;
+			rxm->userdata = NULL;
 			rxdp->read.hdr_addr = dma;
 			rxdp->read.pkt_addr = dma;
 		} else
@@ -3462,6 +3465,7 @@ ixgbe_alloc_rx_queue_mbufs(struct ixgbe_rx_queue *rxq)
 		rte_mbuf_refcnt_set(mbuf, 1);
 		mbuf->next = NULL;
 		mbuf->data_off = RTE_PKTMBUF_HEADROOM;
+		mbuf->userdata = NULL;
 		mbuf->nb_segs = 1;
 		mbuf->port = rxq->port_id;
 
diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
index 912d3b4..d73d8dc 100644
--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
@@ -742,6 +742,7 @@ ixgbe_rxq_vec_setup(struct ixgbe_rx_queue *rxq)
 
 	mb_def.nb_segs = 1;
 	mb_def.data_off = RTE_PKTMBUF_HEADROOM;
+	mb_def.userdata = NULL;
 	mb_def.port = rxq->port_id;
 	rte_mbuf_refcnt_set(&mb_def, 1);
 
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 091c7fb..b6966cc 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -519,6 +519,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 
 		rxm->port = rxvq->port_id;
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 
 		rxm->nb_segs = 1;
 		rxm->next = NULL;
@@ -635,6 +636,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 			seg_num = 1;
 
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 		rxm->nb_segs = seg_num;
 		rxm->next = NULL;
 		rxm->pkt_len = (uint32_t)(len[0] - hdr_size);
@@ -673,6 +675,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 				rxm = rcv_pkts[extra_idx];
 
 				rxm->data_off = RTE_PKTMBUF_HEADROOM - hdr_size;
+				rxm->userdata = NULL;
 				rxm->next = NULL;
 				rxm->pkt_len = (uint32_t)(len[extra_idx]);
 				rxm->data_len = (uint16_t)(len[extra_idx]);
diff --git a/drivers/net/vmxnet3/vmxnet3_rxtx.c b/drivers/net/vmxnet3/vmxnet3_rxtx.c
index c0cbacb..fe6560b 100644
--- a/drivers/net/vmxnet3/vmxnet3_rxtx.c
+++ b/drivers/net/vmxnet3/vmxnet3_rxtx.c
@@ -638,6 +638,7 @@ vmxnet3_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		rxm->pkt_len = (uint16_t)rcd->len;
 		rxm->data_len = (uint16_t)rcd->len;
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 		rxm->ol_flags = 0;
 		rxm->vlan_tci = 0;
 
diff --git a/drivers/net/xenvirt/rte_eth_xenvirt.c b/drivers/net/xenvirt/rte_eth_xenvirt.c
index 73e8bce..4000a31 100644
--- a/drivers/net/xenvirt/rte_eth_xenvirt.c
+++ b/drivers/net/xenvirt/rte_eth_xenvirt.c
@@ -111,6 +111,7 @@ eth_xenvirt_rx(void *q, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 		PMD_RX_LOG(DEBUG, "packet len:%d\n", len[i]);
 		rxm->next = NULL;
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->userdata = NULL;
 		rxm->data_len = (uint16_t)(len[i] - sizeof(struct virtio_net_hdr));
 		rxm->nb_segs = 1;
 		rxm->port = pi->port_id;
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index f0a543b..302a9c1 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -794,6 +794,7 @@ static inline void rte_pktmbuf_reset(struct rte_mbuf *m)
 	m->vlan_tci_outer = 0;
 	m->nb_segs = 1;
 	m->port = 0xff;
+	m->userdata = NULL;
 
 	m->ol_flags = 0;
 	m->packet_type = 0;
-- 
2.1.4

Re: [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized

[Bruce Richardson]

On Thu, Jul 09, 2015 at 04:37:48PM -0700, Stephen Hemminger wrote:
> From: Stephen Hemminger <shemming@brocade.com>
> 
> For applications that use m->userdata the initialization can
> be a signficant (10%) performance penalty.
> 
> Rather than taking the cache penalty of initializing userdata
> in the receive handling, do it in the place where mbuf is
> already cache hot and being setup.

Should the management of the userdata field not be the responsibility of the
app itself, rather than having the PMD manage it? If the PMD does manage the
userdata field, I would suggest taking the approach of having the field cleared
by the mbuf library on free, rather than on RX.

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/cxgbe/sge.c               | 1 +
>  drivers/net/e1000/em_rxtx.c           | 2 ++
>  drivers/net/e1000/igb_rxtx.c          | 2 ++
>  drivers/net/enic/enic_main.c          | 2 ++
>  drivers/net/fm10k/fm10k.h             | 1 +
>  drivers/net/i40e/i40e_rxtx.c          | 4 ++++
>  drivers/net/ixgbe/ixgbe_rxtx.c        | 4 ++++
>  drivers/net/ixgbe/ixgbe_rxtx_vec.c    | 1 +
>  drivers/net/virtio/virtio_rxtx.c      | 3 +++
>  drivers/net/vmxnet3/vmxnet3_rxtx.c    | 1 +
>  drivers/net/xenvirt/rte_eth_xenvirt.c | 1 +
>  lib/librte_mbuf/rte_mbuf.h            | 1 +
>  12 files changed, 23 insertions(+)
> 
<snip>
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> index 912d3b4..d73d8dc 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> @@ -742,6 +742,7 @@ ixgbe_rxq_vec_setup(struct ixgbe_rx_queue *rxq)
>  
>  	mb_def.nb_segs = 1;
>  	mb_def.data_off = RTE_PKTMBUF_HEADROOM;
> +	mb_def.userdata = NULL;
>  	mb_def.port = rxq->port_id;
>  	rte_mbuf_refcnt_set(&mb_def, 1);
>  

This won't actually work for the vector PMD. The userdata field is not covered
by the "rearm_data" part of the mbuf.

/Bruce

Re: [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized

[Stephen Hemminger]

On Fri, 10 Jul 2015 10:32:17 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Jul 09, 2015 at 04:37:48PM -0700, Stephen Hemminger wrote:
> > From: Stephen Hemminger <shemming@brocade.com>
> > 
> > For applications that use m->userdata the initialization can
> > be a signficant (10%) performance penalty.
> > 
> > Rather than taking the cache penalty of initializing userdata
> > in the receive handling, do it in the place where mbuf is
> > already cache hot and being setup.  
> 
> Should the management of the userdata field not be the responsibility of the
> app itself, rather than having the PMD manage it? If the PMD does manage the
> userdata field, I would suggest taking the approach of having the field cleared
> by the mbuf library on free, rather than on RX.

The problem with that is m->userdata is that when the application
gets the mbuf, touching the userdata field causes false cache
sharing and we see a significant performance drop. Internally the
userdata field is only used for few special cases and 0/NULL
is used to indicate that no metadata is present.

Re: [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized

[Olivier MATZ]

On 07/10/2015 05:43 PM, Stephen Hemminger wrote:
> On Fri, 10 Jul 2015 10:32:17 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
>
>> On Thu, Jul 09, 2015 at 04:37:48PM -0700, Stephen Hemminger wrote:
>>> From: Stephen Hemminger <shemming@brocade.com>
>>>
>>> For applications that use m->userdata the initialization can
>>> be a signficant (10%) performance penalty.
>>>
>>> Rather than taking the cache penalty of initializing userdata
>>> in the receive handling, do it in the place where mbuf is
>>> already cache hot and being setup.
>>
>> Should the management of the userdata field not be the responsibility of the
>> app itself, rather than having the PMD manage it? If the PMD does manage the
>> userdata field, I would suggest taking the approach of having the field cleared
>> by the mbuf library on free, rather than on RX.
>
> The problem with that is m->userdata is that when the application
> gets the mbuf, touching the userdata field causes false cache
> sharing and we see a significant performance drop. Internally the
> userdata field is only used for few special cases and 0/NULL
> is used to indicate that no metadata is present.
>

I agree with Bruce: the userdata field is in the second cache line,
and we should avoid touching it in rx path. Resetting it in mbuf_free()
may not be the solution either: in tx path, the mbufs are freed when
we want to send a new mbuf on the hw ring entry, and the mbuf may
not be in the cache anymore.

I don't understand why doing it in the application induces a performance
drop compared to the PMD. What do you mean by false cache sharing?

Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Olivier MATZ]

On 07/10/2015 01:37 AM, Stephen Hemminger wrote:
> From: Stephen Hemminger <shemming@brocade.com>
>
> Added rte_pktmbuf_copy() function since copying multi-part
> segments is common issue and can be problematic.
>
> Signed-off-by: Mike Davison <mdavison@brocade.com>
> Reviewed-by: Stephen Hemminger <shemming@brocade.com>
> ---
>   lib/librte_mbuf/rte_mbuf.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 80419df..f0a543b 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -60,6 +60,7 @@
>   #include <rte_atomic.h>
>   #include <rte_prefetch.h>
>   #include <rte_branch_prediction.h>
> +#include <rte_memcpy.h>
>
>   #ifdef __cplusplus
>   extern "C" {
> @@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const struct rte_mbuf *m)
>   	return !!(m->nb_segs == 1);
>   }
>
> +/*
> + * Creates a copy 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.
> + *  - Copy segment to newly created mbuf.
> + * Then updates pkt_len and nb_segs of the new packet mbuf to match values
> + * from the original packet mbuf.
> + *
> + * @param md
> + *   The packet mbuf to be copied.
> + * @param mp
> + *   The mempool from which the mbufs are allocated.
> + * @return
> + *   - The pointer to the new mbuf on success.
> + *   - NULL if allocation fails.
> + */
> +static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
> +		struct rte_mempool *mp)
> +{
> +	struct rte_mbuf *mc = NULL;
> +	struct rte_mbuf **prev = &mc;
> +
> +	do {
> +		struct rte_mbuf *mi;
> +
> +		mi = rte_pktmbuf_alloc(mp);
> +		if (unlikely(mi == NULL)) {
> +			rte_pktmbuf_free(mc);
> +			return NULL;
> +		}
> +

I think we should check that the destination mbuf is large enough
to store the segment data. Then we have 2 options on failure:
- split the original segment into several destination segments
- return an error


> +		mi->data_off = md->data_off;
> +		mi->data_len = md->data_len;
> +		mi->port = md->port;
> +		mi->vlan_tci = md->vlan_tci;
> +		mi->tx_offload = md->tx_offload;
> +		mi->hash = md->hash;
> +
> +		mi->next = NULL;
> +		mi->pkt_len = md->pkt_len;
> +		mi->nb_segs = md->nb_segs;
> +		mi->ol_flags = md->ol_flags;
> +		mi->packet_type = md->packet_type;
> +
> +		rte_memcpy(rte_pktmbuf_mtod(mi, char *),
> +			   rte_pktmbuf_mtod(md, char *),
> +			   md->data_len);
> +
> +		*prev = mi;
> +		prev = &mi->next;
> +	} while ((md = md->next) != NULL);
> +
> +	*prev = NULL;
> +	__rte_mbuf_sanity_check(mc, 1);
> +	return mc;
> +}
> +
>   /**
>    * Dump an mbuf structure to the console.
>    *
>

Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Mrzyglod, DanielX T]

>-----Original Message-----
>From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
>Sent: Friday, July 10, 2015 1:38 AM
>To: dev@dpdk.org
>Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger
><shemming@brocade.com>
>Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
>
>From: Stephen Hemminger <shemming@brocade.com>
>
>Added rte_pktmbuf_copy() function since copying multi-part
>segments is common issue and can be problematic.
>
>Signed-off-by: Mike Davison <mdavison@brocade.com>
>Reviewed-by: Stephen Hemminger <shemming@brocade.com>
>---
> lib/librte_mbuf/rte_mbuf.h | 59
>++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
>
>diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>index 80419df..f0a543b 100644
>--- a/lib/librte_mbuf/rte_mbuf.h
>+++ b/lib/librte_mbuf/rte_mbuf.h
>@@ -60,6 +60,7 @@
> #include <rte_atomic.h>
> #include <rte_prefetch.h>
> #include <rte_branch_prediction.h>
>+#include <rte_memcpy.h>
>
> #ifdef __cplusplus
> extern "C" {
>@@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const
>struct rte_mbuf *m)
> 	return !!(m->nb_segs == 1);
> }
>
>+/*
>+ * Creates a copy 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.
>+ *  - Copy segment to newly created mbuf.
>+ * Then updates pkt_len and nb_segs of the new packet mbuf to match values
>+ * from the original packet mbuf.
>+ *
>+ * @param md
>+ *   The packet mbuf to be copied.
>+ * @param mp
>+ *   The mempool from which the mbufs are allocated.
>+ * @return
>+ *   - The pointer to the new mbuf on success.
>+ *   - NULL if allocation fails.
>+ */
>+static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
>+		struct rte_mempool *mp)
>+{
>+	struct rte_mbuf *mc = NULL;
>+	struct rte_mbuf **prev = &mc;
>+
>+	do {
>+		struct rte_mbuf *mi;
>+
>+		mi = rte_pktmbuf_alloc(mp);
>+		if (unlikely(mi == NULL)) {
>+			rte_pktmbuf_free(mc);
>+			return NULL;
>+		}
>+
>+		mi->data_off = md->data_off;
>+		mi->data_len = md->data_len;
>+		mi->port = md->port;
>+		mi->vlan_tci = md->vlan_tci;
>+		mi->tx_offload = md->tx_offload;
>+		mi->hash = md->hash;
>+
>+		mi->next = NULL;
>+		mi->pkt_len = md->pkt_len;
>+		mi->nb_segs = md->nb_segs;
>+		mi->ol_flags = md->ol_flags;
>+		mi->packet_type = md->packet_type;
>+
>+		rte_memcpy(rte_pktmbuf_mtod(mi, char *),
>+			   rte_pktmbuf_mtod(md, char *),
>+			   md->data_len);
>+
>+		*prev = mi;
>+		prev = &mi->next;
>+	} while ((md = md->next) != NULL);
>+
>+	*prev = NULL;
>+	__rte_mbuf_sanity_check(mc, 1);
>+	return mc;
>+}
>+
> /**
>  * Dump an mbuf structure to the console.
>  *
>--
>2.1.4

Hi Stephen :>
This patch look useful in case of backup buffs. 
There will be second approach ?

Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Stephen Hemminger]

On Fri, 22 Jan 2016 13:40:44 +0000
"Mrzyglod, DanielX T" <danielx.t.mrzyglod@intel.com> wrote:

> 
> 
> >-----Original Message-----
> >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> >Sent: Friday, July 10, 2015 1:38 AM
> >To: dev@dpdk.org
> >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger
> ><shemming@brocade.com>
> >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> >
> >From: Stephen Hemminger <shemming@brocade.com>
> >
> >Added rte_pktmbuf_copy() function since copying multi-part
> >segments is common issue and can be problematic.
> >
> >Signed-off-by: Mike Davison <mdavison@brocade.com>
> >Reviewed-by: Stephen Hemminger <shemming@brocade.com>
> >---
> > lib/librte_mbuf/rte_mbuf.h | 59
> >++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 59 insertions(+)
> >
> >diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> >index 80419df..f0a543b 100644
> >--- a/lib/librte_mbuf/rte_mbuf.h
> >+++ b/lib/librte_mbuf/rte_mbuf.h
> >@@ -60,6 +60,7 @@
> > #include <rte_atomic.h>
> > #include <rte_prefetch.h>
> > #include <rte_branch_prediction.h>
> >+#include <rte_memcpy.h>
> >
> > #ifdef __cplusplus
> > extern "C" {
> >@@ -1272,6 +1273,64 @@ static inline int rte_pktmbuf_is_contiguous(const
> >struct rte_mbuf *m)
> > 	return !!(m->nb_segs == 1);
> > }
> >
> >+/*
> >+ * Creates a copy 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.
> >+ *  - Copy segment to newly created mbuf.
> >+ * Then updates pkt_len and nb_segs of the new packet mbuf to match values
> >+ * from the original packet mbuf.
> >+ *
> >+ * @param md
> >+ *   The packet mbuf to be copied.
> >+ * @param mp
> >+ *   The mempool from which the mbufs are allocated.
> >+ * @return
> >+ *   - The pointer to the new mbuf on success.
> >+ *   - NULL if allocation fails.
> >+ */
> >+static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
> >+		struct rte_mempool *mp)
> >+{
> >+	struct rte_mbuf *mc = NULL;
> >+	struct rte_mbuf **prev = &mc;
> >+
> >+	do {
> >+		struct rte_mbuf *mi;
> >+
> >+		mi = rte_pktmbuf_alloc(mp);
> >+		if (unlikely(mi == NULL)) {
> >+			rte_pktmbuf_free(mc);
> >+			return NULL;
> >+		}
> >+
> >+		mi->data_off = md->data_off;
> >+		mi->data_len = md->data_len;
> >+		mi->port = md->port;
> >+		mi->vlan_tci = md->vlan_tci;
> >+		mi->tx_offload = md->tx_offload;
> >+		mi->hash = md->hash;
> >+
> >+		mi->next = NULL;
> >+		mi->pkt_len = md->pkt_len;
> >+		mi->nb_segs = md->nb_segs;
> >+		mi->ol_flags = md->ol_flags;
> >+		mi->packet_type = md->packet_type;
> >+
> >+		rte_memcpy(rte_pktmbuf_mtod(mi, char *),
> >+			   rte_pktmbuf_mtod(md, char *),
> >+			   md->data_len);
> >+
> >+		*prev = mi;
> >+		prev = &mi->next;
> >+	} while ((md = md->next) != NULL);
> >+
> >+	*prev = NULL;
> >+	__rte_mbuf_sanity_check(mc, 1);
> >+	return mc;
> >+}
> >+
> > /**
> >  * Dump an mbuf structure to the console.
> >  *
> >--
> >2.1.4
> 
> Hi Stephen :>
> This patch look useful in case of backup buffs. 
> There will be second approach ?

I did bother resubmitting it since unless there are users in current
code base it would likely rot.

Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Olivier MATZ]

Hi Stephen,

Please find some comments below.

On 01/22/2016 02:40 PM, Mrzyglod, DanielX T wrote:
>> +/*
>> + * Creates a copy 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.
>> + *  - Copy segment to newly created mbuf.
>> + * Then updates pkt_len and nb_segs of the new packet mbuf to match values
>> + * from the original packet mbuf.
>> + *
>> + * @param md
>> + *   The packet mbuf to be copied.
>> + * @param mp
>> + *   The mempool from which the mbufs are allocated.
>> + * @return
>> + *   - The pointer to the new mbuf on success.
>> + *   - NULL if allocation fails.
>> + */
>> +static inline struct rte_mbuf *rte_pktmbuf_copy(struct rte_mbuf *md,
>> +		struct rte_mempool *mp)

You are usually against inline functions. Any reason why it's better
to inline the code in this case?

Also, I think the mbuf argument should be const.

>> +{
>> +	struct rte_mbuf *mc = NULL;
>> +	struct rte_mbuf **prev = &mc;
>> +
>> +	do {
>> +		struct rte_mbuf *mi;
>> +
>> +		mi = rte_pktmbuf_alloc(mp);
>> +		if (unlikely(mi == NULL)) {
>> +			rte_pktmbuf_free(mc);
>> +			return NULL;
>> +		}
>> +
>> +		mi->data_off = md->data_off;

I'm not a big fan of 'mi' and 'md' (and 'mc'). In rte_pktmbuf_attach(),
'md' means direct and 'mi' means indirect. Here, all mbufs are direct
(i.e. they points to their own data buffer).

I think that using 'm' and 'm2' (or m_dup) is clearer. Also, 'seg' looks
clearer for a mbuf that points to a rte_mbuf inside the chain.

>> +		mi->data_len = md->data_len;
>> +		mi->port = md->port;

We don't need to update port for all the segments here.
Same for vlan_tci, tx_offload, hash, pkt_len, nb_segs, ol_flags,
packet_type.

>> +		mi->vlan_tci = md->vlan_tci;
>> +		mi->tx_offload = md->tx_offload;
>> +		mi->hash = md->hash;
>> +
>> +		mi->next = NULL;
>> +		mi->pkt_len = md->pkt_len;
>> +		mi->nb_segs = md->nb_segs;
>> +		mi->ol_flags = md->ol_flags;
>> +		mi->packet_type = md->packet_type;
>> +
>> +		rte_memcpy(rte_pktmbuf_mtod(mi, char *),
>> +			   rte_pktmbuf_mtod(md, char *),
>> +			   md->data_len);
>> +
>> +		*prev = mi;
>> +		prev = &mi->next;

Using a double mbuf pointer here looks a bit overkill to me.
I suggest to do something like (just an example, not even compiled):

struct rte_mbuf *rte_pktmbuf_copy(const struct rte_mbuf *m,
	struct rte_mempool *mp)
{
	struct rte_mbuf *m2, *seg, *seg2;

	m2 = rte_pktmbuf_alloc(mp);
	if (unlikely(m2 == NULL)) {
		rte_pktmbuf_free(m2);
		return NULL;
	}
	m2->data_off = m->data_off;
	m2->data_len = m->data_len;
	m2->pkt_len = m->pkt_len;
	m2->tx_offload = m->tx_offload;
	/* ... */

	for (seg = m->next; seg != NULL; seg = seg->next) {

		seg2 = rte_pktmbuf_alloc(mp);
		if (unlikely(seg2 == NULL)) {
			rte_pktmbuf_free(m2);
			return NULL;
		}

		seg2->data_off = seg->data_off;
		/* ... */

		seg = seg->next;
	}
	return m2;
}


>> +	} while ((md = md->next) != NULL);
>> +
>> +	*prev = NULL;

why this?

>> +	__rte_mbuf_sanity_check(mc, 1);
>> +	return mc;
>> +}
>> +

I agree this kind of function could be useful. But if there is no
user of this code inside the dpdk, I think we must at least have
a test function for it in app/test/test_mbuf.c


Regards,
Olivier

Re: [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized

[Olivier MATZ]

Hi,

On 07/10/2015 05:43 PM, Stephen Hemminger wrote:
> On Fri, 10 Jul 2015 10:32:17 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
>
>> On Thu, Jul 09, 2015 at 04:37:48PM -0700, Stephen Hemminger wrote:
>>> From: Stephen Hemminger <shemming@brocade.com>
>>>
>>> For applications that use m->userdata the initialization can
>>> be a signficant (10%) performance penalty.
>>>
>>> Rather than taking the cache penalty of initializing userdata
>>> in the receive handling, do it in the place where mbuf is
>>> already cache hot and being setup.
>>
>> Should the management of the userdata field not be the responsibility of the
>> app itself, rather than having the PMD manage it? If the PMD does manage the
>> userdata field, I would suggest taking the approach of having the field cleared
>> by the mbuf library on free, rather than on RX.
>
> The problem with that is m->userdata is that when the application
> gets the mbuf, touching the userdata field causes false cache
> sharing and we see a significant performance drop. Internally the
> userdata field is only used for few special cases and 0/NULL
> is used to indicate that no metadata is present.

Agree with Bruce. The management of this field is the responsibility
of the application.

Also, this field is located in the second cache line, and we should
avoid to touch it in the rx functions. We had the same discussion for
the next field in this thread:
http://dpdk.org/ml/archives/dev/2015-June/019182.html

A pure-app solution would be to set it to NULL in:
- your mempool object init function
- in your mbuf_free() function, before calling rte_pktmbuf_free()
- before passing the mbuf to the tx driver

But a better solution in dpdk is probably what Bruce suggests.

Regards,
Olivier

Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Stephen Hemminger]

On Wed, 3 Feb 2016 17:23:05 +0000
Olivier MATZ <olivier.matz@6wind.com> wrote:

>  +	} while ((md = md->next) != NULL);
> >> +
> >> +	*prev = NULL;  
> 
> why this?
This is null terminating the linked list of segments.

Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Pattan, Reshma]

Hi,

> > >-----Original Message-----
> > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > >Hemminger
> > >Sent: Friday, July 10, 2015 1:38 AM
> > >To: dev@dpdk.org
> > >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger
> > ><shemming@brocade.com>
> > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> > >
> > >From: Stephen Hemminger <shemming@brocade.com>
> > >
> > >Added rte_pktmbuf_copy() function since copying multi-part segments
> > >is common issue and can be problematic.
> > >
> > >Signed-off-by: Mike Davison <mdavison@brocade.com>
> > >Reviewed-by: Stephen Hemminger <shemming@brocade.com>
> > >---
> >
> > Hi Stephen :>
> > This patch look useful in case of backup buffs.
> > There will be second approach ?
> 
> I did bother resubmitting it since unless there are users in current code base it
> would likely rot.

I was writing similar mbuf copy functionality  which is required for tcpdump feature.
But, It was brought to my notice that this patch with similar functionality already  exists, so I would like to take this patch and work on further.
Also, if you have any further code changes on this patch, would you please send  the latest one. I will work further.

Thanks,
Reshma

Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy

[Stephen Hemminger]

On Fri, 18 Mar 2016 17:03:51 +0000
"Pattan, Reshma" <reshma.pattan@intel.com> wrote:

> Hi,
> 
> > > >-----Original Message-----
> > > >From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > > >Hemminger
> > > >Sent: Friday, July 10, 2015 1:38 AM
> > > >To: dev@dpdk.org
> > > >Cc: Mike Davison <mdavison@brocade.com>; Stephen Hemminger
> > > ><shemming@brocade.com>
> > > >Subject: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
> > > >
> > > >From: Stephen Hemminger <shemming@brocade.com>
> > > >
> > > >Added rte_pktmbuf_copy() function since copying multi-part segments
> > > >is common issue and can be problematic.
> > > >
> > > >Signed-off-by: Mike Davison <mdavison@brocade.com>
> > > >Reviewed-by: Stephen Hemminger <shemming@brocade.com>
> > > >---
> > >
> > > Hi Stephen :>
> > > This patch look useful in case of backup buffs.
> > > There will be second approach ?
> > 
> > I did bother resubmitting it since unless there are users in current code base it
> > would likely rot.
> 
> I was writing similar mbuf copy functionality  which is required for tcpdump feature.
> But, It was brought to my notice that this patch with similar functionality already  exists, so I would like to take this patch and work on further.
> Also, if you have any further code changes on this patch, would you please send  the latest one. I will work further.
> 
> Thanks,
> Reshma

We have a newer version that handles different size pools.