[dpdk-dev,v2,1/3] port: added WRITER_APPROACH == 1 implementation to ring port

Message ID 1430395084-6172-2-git-send-email-michalx.k.jastrzebski@intel.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Michal Jastrzebski April 30, 2015, 11:58 a.m. UTC
  From: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>

Added better optimized implementation of tx_bulk for ring writer port
based on
similar solution in ethdev_writer port. New implementation sends burst
without
copying data to internal buffer if it is possible.

Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
---
 lib/librte_port/rte_port_ring.c |   59 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)
  

Comments

Thomas Monjalon May 17, 2015, 9:44 p.m. UTC | #1
2015-04-30 13:58, Michal Jastrzebski:
> From: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> 
> Added better optimized implementation of tx_bulk for ring writer port
> based on
> similar solution in ethdev_writer port. New implementation sends burst
> without
> copying data to internal buffer if it is possible.
> 
> Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
[...]
> +#if RTE_PORT_RING_WRITER_APPROACH == 0

Nack
Maybe you missed the previous comment:
	http://dpdk.org/ml/archives/dev/2015-March/015999.html
  
Cristian Dumitrescu May 19, 2015, 10:49 p.m. UTC | #2
HI Thomas and David,

We can remove one of the code branches if you guys feel strongly about it.

The reason we recommended to keep both is because both of them are working, and we wanted to keep both of them for a while to get some feedback from other people about which one they prefer. It might be that some apps would prefer one over the other for performance reasons. But again, we can resend this patch with only one code path present.

Regards,
Cristian

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Sunday, May 17, 2015 10:45 PM
> To: Jastrzebski, MichalX K; Gajdzica, MaciejX T
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/3] port: added WRITER_APPROACH == 1
> implementation to ring port
> 
> 2015-04-30 13:58, Michal Jastrzebski:
> > From: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> >
> > Added better optimized implementation of tx_bulk for ring writer port
> > based on
> > similar solution in ethdev_writer port. New implementation sends burst
> > without
> > copying data to internal buffer if it is possible.
> >
> > Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> [...]
> > +#if RTE_PORT_RING_WRITER_APPROACH == 0
> 
> Nack
> Maybe you missed the previous comment:
> 	http://dpdk.org/ml/archives/dev/2015-March/015999.html
  
Thomas Monjalon May 19, 2015, 11:19 p.m. UTC | #3
Hi Cristian,

2015-05-19 22:49, Dumitrescu, Cristian:
> HI Thomas and David,
> 
> We can remove one of the code branches if you guys feel strongly about it.

We don't feel strongly as we are not authors nor testers of this new code.
But we want to avoid the maintenance nightmare of #ifdef.

> The reason we recommended to keep both is because both of them are working,
> and we wanted to keep both of them for a while to get some feedback from
> other people about which one they prefer. It might be that some apps would
> prefer one over the other for performance reasons. But again, we can resend
> this patch with only one code path present.

In general, RFC patches are used to request feedbacks.
I think it's better to have only one implementation at a time.
Comments are welcome.

> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Sunday, May 17, 2015 10:45 PM
> > To: Jastrzebski, MichalX K; Gajdzica, MaciejX T
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/3] port: added WRITER_APPROACH == 1
> > implementation to ring port
> > 
> > 2015-04-30 13:58, Michal Jastrzebski:
> > > From: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> > >
> > > Added better optimized implementation of tx_bulk for ring writer port
> > > based on
> > > similar solution in ethdev_writer port. New implementation sends burst
> > > without
> > > copying data to internal buffer if it is possible.
> > >
> > > Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> > [...]
> > > +#if RTE_PORT_RING_WRITER_APPROACH == 0
> > 
> > Nack
> > Maybe you missed the previous comment:
> > 	http://dpdk.org/ml/archives/dev/2015-March/015999.html
  

Patch

diff --git a/lib/librte_port/rte_port_ring.c b/lib/librte_port/rte_port_ring.c
index fa3d77b..ba2eeb3 100644
--- a/lib/librte_port/rte_port_ring.c
+++ b/lib/librte_port/rte_port_ring.c
@@ -96,11 +96,14 @@  rte_port_ring_reader_free(void *port)
 /*
  * Port RING Writer
  */
+#define RTE_PORT_RING_WRITER_APPROACH                  1
+
 struct rte_port_ring_writer {
 	struct rte_mbuf *tx_buf[RTE_PORT_IN_BURST_SIZE_MAX];
 	struct rte_ring *ring;
 	uint32_t tx_burst_sz;
 	uint32_t tx_buf_count;
+	uint64_t bsz_mask;
 };
 
 static void *
@@ -130,6 +133,7 @@  rte_port_ring_writer_create(void *params, int socket_id)
 	port->ring = conf->ring;
 	port->tx_burst_sz = conf->tx_burst_sz;
 	port->tx_buf_count = 0;
+	port->bsz_mask = 1LLU << (conf->tx_burst_sz - 1);
 
 	return port;
 }
@@ -160,6 +164,8 @@  rte_port_ring_writer_tx(void *port, struct rte_mbuf *pkt)
 	return 0;
 }
 
+#if RTE_PORT_RING_WRITER_APPROACH == 0
+
 static int
 rte_port_ring_writer_tx_bulk(void *port,
 		struct rte_mbuf **pkts,
@@ -194,6 +200,59 @@  rte_port_ring_writer_tx_bulk(void *port,
 	return 0;
 }
 
+#elif RTE_PORT_RING_WRITER_APPROACH == 1
+
+static int
+rte_port_ring_writer_tx_bulk(void *port,
+		struct rte_mbuf **pkts,
+		uint64_t pkts_mask)
+{
+	struct rte_port_ring_writer *p =
+		(struct rte_port_ring_writer *) port;
+
+	uint32_t bsz_mask = p->bsz_mask;
+	uint32_t tx_buf_count = p->tx_buf_count;
+	uint64_t expr = (pkts_mask & (pkts_mask + 1)) |
+			((pkts_mask & bsz_mask) ^ bsz_mask);
+
+	if (expr == 0) {
+		uint64_t n_pkts = __builtin_popcountll(pkts_mask);
+		uint32_t n_pkts_ok;
+
+		if (tx_buf_count)
+			send_burst(p);
+
+		n_pkts_ok = rte_ring_sp_enqueue_burst(p->ring, (void **)pkts, n_pkts);
+
+		for ( ; n_pkts_ok < n_pkts; n_pkts_ok++) {
+			struct rte_mbuf *pkt = pkts[n_pkts_ok];
+
+			rte_pktmbuf_free(pkt);
+		}
+	} else {
+		for ( ; pkts_mask; ) {
+			uint32_t pkt_index = __builtin_ctzll(pkts_mask);
+			uint64_t pkt_mask = 1LLU << pkt_index;
+			struct rte_mbuf *pkt = pkts[pkt_index];
+
+			p->tx_buf[tx_buf_count++] = pkt;
+			pkts_mask &= ~pkt_mask;
+		}
+
+		p->tx_buf_count = tx_buf_count;
+		if (tx_buf_count >= p->tx_burst_sz)
+			send_burst(p);
+	}
+
+	return 0;
+}
+
+#else
+
+#error Invalid value for RTE_PORT_RING_WRITER_APPROACH
+
+#endif
+
 static int
 rte_port_ring_writer_flush(void *port)
 {