DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/2] mbuf: improvements
@ 2015-07-09 23:37 Stephen Hemminger
  2015-07-09 23:37 ` [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy Stephen Hemminger
  2015-07-09 23:37 ` [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized Stephen Hemminger
  0 siblings, 2 replies; 14+ messages in thread
From: Stephen Hemminger @ 2015-07-09 23:37 UTC (permalink / raw)
  To: dev

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
  2015-07-09 23:37 [dpdk-dev] [PATCH 0/2] mbuf: improvements Stephen Hemminger
@ 2015-07-09 23:37 ` Stephen Hemminger
  2015-07-15 10:14   ` Olivier MATZ
  2016-01-22 13:40   ` Mrzyglod, DanielX T
  2015-07-09 23:37 ` [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized Stephen Hemminger
  1 sibling, 2 replies; 14+ messages in thread
From: Stephen Hemminger @ 2015-07-09 23:37 UTC (permalink / raw)
  To: dev; +Cc: Mike Davison, 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized
  2015-07-09 23:37 [dpdk-dev] [PATCH 0/2] mbuf: improvements Stephen Hemminger
  2015-07-09 23:37 ` [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy Stephen Hemminger
@ 2015-07-09 23:37 ` Stephen Hemminger
  2015-07-10  9:32   ` Bruce Richardson
  1 sibling, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2015-07-09 23:37 UTC (permalink / raw)
  To: dev; +Cc: 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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized
  2015-07-09 23:37 ` [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized Stephen Hemminger
@ 2015-07-10  9:32   ` Bruce Richardson
  2015-07-10 15:43     ` Stephen Hemminger
  0 siblings, 1 reply; 14+ messages in thread
From: Bruce Richardson @ 2015-07-10  9:32 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev, Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized
  2015-07-10  9:32   ` Bruce Richardson
@ 2015-07-10 15:43     ` Stephen Hemminger
  2015-07-15 10:10       ` Olivier MATZ
  2016-02-03 17:23       ` Olivier MATZ
  0 siblings, 2 replies; 14+ messages in thread
From: Stephen Hemminger @ 2015-07-10 15:43 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized
  2015-07-10 15:43     ` Stephen Hemminger
@ 2015-07-15 10:10       ` Olivier MATZ
  2016-02-03 17:23       ` Olivier MATZ
  1 sibling, 0 replies; 14+ messages in thread
From: Olivier MATZ @ 2015-07-15 10:10 UTC (permalink / raw)
  To: Stephen Hemminger, Bruce Richardson; +Cc: dev, Stephen Hemminger

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?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
  2015-07-09 23:37 ` [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy Stephen Hemminger
@ 2015-07-15 10:14   ` Olivier MATZ
  2016-01-22 13:40   ` Mrzyglod, DanielX T
  1 sibling, 0 replies; 14+ messages in thread
From: Olivier MATZ @ 2015-07-15 10:14 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Mike Davison, Stephen Hemminger

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.
>    *
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
  2015-07-09 23:37 ` [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy Stephen Hemminger
  2015-07-15 10:14   ` Olivier MATZ
@ 2016-01-22 13:40   ` Mrzyglod, DanielX T
  2016-01-22 17:33     ` Stephen Hemminger
                       ` (2 more replies)
  1 sibling, 3 replies; 14+ messages in thread
From: Mrzyglod, DanielX T @ 2016-01-22 13:40 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Mike Davison, Stephen Hemminger



>-----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 ?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
  2016-01-22 13:40   ` Mrzyglod, DanielX T
@ 2016-01-22 17:33     ` Stephen Hemminger
  2016-03-18 17:03       ` Pattan, Reshma
  2016-02-03 17:23     ` Olivier MATZ
       [not found]     ` <ccbdb829556f4423b009aff93f27c93b@BRMWP-EXMB11.corp.brocade.com>
  2 siblings, 1 reply; 14+ messages in thread
From: Stephen Hemminger @ 2016-01-22 17:33 UTC (permalink / raw)
  To: Mrzyglod, DanielX T; +Cc: dev, Mike Davison, 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
  2016-01-22 13:40   ` Mrzyglod, DanielX T
  2016-01-22 17:33     ` Stephen Hemminger
@ 2016-02-03 17:23     ` Olivier MATZ
       [not found]     ` <ccbdb829556f4423b009aff93f27c93b@BRMWP-EXMB11.corp.brocade.com>
  2 siblings, 0 replies; 14+ messages in thread
From: Olivier MATZ @ 2016-02-03 17:23 UTC (permalink / raw)
  To: Mrzyglod, DanielX T, Stephen Hemminger, dev
  Cc: Mike Davison, Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized
  2015-07-10 15:43     ` Stephen Hemminger
  2015-07-15 10:10       ` Olivier MATZ
@ 2016-02-03 17:23       ` Olivier MATZ
  1 sibling, 0 replies; 14+ messages in thread
From: Olivier MATZ @ 2016-02-03 17:23 UTC (permalink / raw)
  To: Stephen Hemminger, Bruce Richardson; +Cc: dev, Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
       [not found]     ` <ccbdb829556f4423b009aff93f27c93b@BRMWP-EXMB11.corp.brocade.com>
@ 2016-02-04  0:49       ` Stephen Hemminger
  0 siblings, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2016-02-04  0:49 UTC (permalink / raw)
  To: Olivier MATZ; +Cc: dev, Mike Davison, 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
  2016-01-22 17:33     ` Stephen Hemminger
@ 2016-03-18 17:03       ` Pattan, Reshma
  2016-03-18 17:40         ` Stephen Hemminger
  0 siblings, 1 reply; 14+ messages in thread
From: Pattan, Reshma @ 2016-03-18 17:03 UTC (permalink / raw)
  To: Stephen Hemminger, Mrzyglod, DanielX T
  Cc: dev, Mike Davison, Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy
  2016-03-18 17:03       ` Pattan, Reshma
@ 2016-03-18 17:40         ` Stephen Hemminger
  0 siblings, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2016-03-18 17:40 UTC (permalink / raw)
  To: Pattan, Reshma; +Cc: Mrzyglod, DanielX T, dev, Mike Davison, 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-03-18 17:40 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-09 23:37 [dpdk-dev] [PATCH 0/2] mbuf: improvements Stephen Hemminger
2015-07-09 23:37 ` [dpdk-dev] [PATCH 1/2] mbuf: Add rte_pktmbuf_copy Stephen Hemminger
2015-07-15 10:14   ` Olivier MATZ
2016-01-22 13:40   ` Mrzyglod, DanielX T
2016-01-22 17:33     ` Stephen Hemminger
2016-03-18 17:03       ` Pattan, Reshma
2016-03-18 17:40         ` Stephen Hemminger
2016-02-03 17:23     ` Olivier MATZ
     [not found]     ` <ccbdb829556f4423b009aff93f27c93b@BRMWP-EXMB11.corp.brocade.com>
2016-02-04  0:49       ` Stephen Hemminger
2015-07-09 23:37 ` [dpdk-dev] [PATCH 2/2] mbuf: make sure userdata is initialized Stephen Hemminger
2015-07-10  9:32   ` Bruce Richardson
2015-07-10 15:43     ` Stephen Hemminger
2015-07-15 10:10       ` Olivier MATZ
2016-02-03 17:23       ` Olivier MATZ

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).