[dpdk-dev,1/6] testpmd: add io_retry forwarding

Message ID 1462488421-118990-2-git-send-email-zhihong.wang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Zhihong Wang May 5, 2016, 10:46 p.m. UTC
  This patch adds an io_retry-fwd in testpmd to prevent most packet
losses. It can be enabled by "set fwd io_retry".

io-fwd is the fastest possible forwarding engine, good for basic
performance test. Adding retry mechanism expands test case coverage
to support scenarios where packet loss affects test results.


Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
---
 app/test-pmd/Makefile      |   1 +
 app/test-pmd/iofwd-retry.c | 139 +++++++++++++++++++++++++++++++++++++++++++++
 app/test-pmd/testpmd.c     |   1 +
 app/test-pmd/testpmd.h     |   1 +
 4 files changed, 142 insertions(+)
 create mode 100644 app/test-pmd/iofwd-retry.c
  

Comments

Thomas Monjalon May 25, 2016, 9:32 a.m. UTC | #1
2016-05-05 18:46, Zhihong Wang:
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
>  extern struct fwd_engine io_fwd_engine;
> +extern struct fwd_engine io_retry_fwd_engine;
>  extern struct fwd_engine mac_fwd_engine;
>  extern struct fwd_engine mac_retry_fwd_engine;
>  extern struct fwd_engine mac_swap_engine;

We now have 2 engines with "retry" behaviour.
It is maybe the way to go, but I want to ask the question:
Would it be possible to have "retry" as an engine parameter?
  
Zhihong Wang May 26, 2016, 2:40 a.m. UTC | #2
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, May 25, 2016 5:32 PM
> To: Wang, Zhihong <zhihong.wang@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: Re: [PATCH 1/6] testpmd: add io_retry forwarding
> 
> 2016-05-05 18:46, Zhihong Wang:
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> >  extern struct fwd_engine io_fwd_engine;
> > +extern struct fwd_engine io_retry_fwd_engine;
> >  extern struct fwd_engine mac_fwd_engine;
> >  extern struct fwd_engine mac_retry_fwd_engine;
> >  extern struct fwd_engine mac_swap_engine;
> 
> We now have 2 engines with "retry" behaviour.
> It is maybe the way to go, but I want to ask the question:
> Would it be possible to have "retry" as an engine parameter?
> 

If it's just about the way to write commands there isn't much difference,
like "set fwd io_rety" and "set fwd io retry".

Do you mean to add the "retry" for all engines, and also implement this
as a parameter in each original engine? So for example, no iofwd-retry.c,
just add this feature inside iofwd.c?
  
Thomas Monjalon May 26, 2016, 6:27 a.m. UTC | #3
2016-05-26 02:40, Wang, Zhihong:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-05-05 18:46, Zhihong Wang:
> > > --- a/app/test-pmd/testpmd.h
> > > +++ b/app/test-pmd/testpmd.h
> > >  extern struct fwd_engine io_fwd_engine;
> > > +extern struct fwd_engine io_retry_fwd_engine;
> > >  extern struct fwd_engine mac_fwd_engine;
> > >  extern struct fwd_engine mac_retry_fwd_engine;
> > >  extern struct fwd_engine mac_swap_engine;
> > 
> > We now have 2 engines with "retry" behaviour.
> > It is maybe the way to go, but I want to ask the question:
> > Would it be possible to have "retry" as an engine parameter?
> > 
> 
> If it's just about the way to write commands there isn't much difference,
> like "set fwd io_rety" and "set fwd io retry".
> 
> Do you mean to add the "retry" for all engines, and also implement this
> as a parameter in each original engine? So for example, no iofwd-retry.c,
> just add this feature inside iofwd.c?

Yes, if it makes sense. For engines other than io_fwd and mac_fwd, the retry
option can be unsupported (return an error) as a first step.
  
Zhihong Wang May 26, 2016, 9:24 a.m. UTC | #4
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, May 26, 2016 2:27 PM
> To: Wang, Zhihong <zhihong.wang@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> Richardson, Bruce <bruce.richardson@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Subject: Re: [PATCH 1/6] testpmd: add io_retry forwarding
> 
> 2016-05-26 02:40, Wang, Zhihong:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-05-05 18:46, Zhihong Wang:
> > > > --- a/app/test-pmd/testpmd.h
> > > > +++ b/app/test-pmd/testpmd.h
> > > >  extern struct fwd_engine io_fwd_engine;
> > > > +extern struct fwd_engine io_retry_fwd_engine;
> > > >  extern struct fwd_engine mac_fwd_engine;
> > > >  extern struct fwd_engine mac_retry_fwd_engine;
> > > >  extern struct fwd_engine mac_swap_engine;
> > >
> > > We now have 2 engines with "retry" behaviour.
> > > It is maybe the way to go, but I want to ask the question:
> > > Would it be possible to have "retry" as an engine parameter?
> > >
> >
> > If it's just about the way to write commands there isn't much difference,
> > like "set fwd io_rety" and "set fwd io retry".
> >
> > Do you mean to add the "retry" for all engines, and also implement this
> > as a parameter in each original engine? So for example, no iofwd-retry.c,
> > just add this feature inside iofwd.c?
> 
> Yes, if it makes sense. For engines other than io_fwd and mac_fwd, the retry
> option can be unsupported (return an error) as a first step.

I think it makes sense in terms of making code more clear and manageable.
  

Patch

diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
index 72426f3..a6735cf 100644
--- a/app/test-pmd/Makefile
+++ b/app/test-pmd/Makefile
@@ -49,6 +49,7 @@  SRCS-y += parameters.c
 SRCS-$(CONFIG_RTE_LIBRTE_CMDLINE) += cmdline.c
 SRCS-y += config.c
 SRCS-y += iofwd.c
+SRCS-y += iofwd-retry.c
 SRCS-y += macfwd.c
 SRCS-y += macfwd-retry.c
 SRCS-y += macswap.c
diff --git a/app/test-pmd/iofwd-retry.c b/app/test-pmd/iofwd-retry.c
new file mode 100644
index 0000000..14c5660
--- /dev/null
+++ b/app/test-pmd/iofwd-retry.c
@@ -0,0 +1,139 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdarg.h>
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+#include <stdint.h>
+#include <unistd.h>
+#include <inttypes.h>
+
+#include <sys/queue.h>
+#include <sys/stat.h>
+
+#include <rte_common.h>
+#include <rte_byteorder.h>
+#include <rte_log.h>
+#include <rte_debug.h>
+#include <rte_cycles.h>
+#include <rte_memory.h>
+#include <rte_memzone.h>
+#include <rte_launch.h>
+#include <rte_eal.h>
+#include <rte_per_lcore.h>
+#include <rte_lcore.h>
+#include <rte_atomic.h>
+#include <rte_branch_prediction.h>
+#include <rte_ring.h>
+#include <rte_memory.h>
+#include <rte_memcpy.h>
+#include <rte_mempool.h>
+#include <rte_mbuf.h>
+#include <rte_interrupts.h>
+#include <rte_pci.h>
+#include <rte_ether.h>
+#include <rte_ethdev.h>
+#include <rte_string_fns.h>
+
+#include "testpmd.h"
+
+#define TX_RETRY_TIMES 128
+#define TX_RETRY_WAIT_US 1
+
+/*
+ * I/O retry forwarding mode.
+ * Forward packets "as-is" without access to packet data.
+ */
+static void
+pkt_burst_io_retry_forward(struct fwd_stream *fs)
+{
+	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
+	uint16_t nb_rx;
+	uint16_t nb_tx;
+	uint32_t retry;
+
+#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
+	uint64_t start_tsc;
+	uint64_t end_tsc;
+	uint64_t core_cycles;
+#endif
+
+#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
+	start_tsc = rte_rdtsc();
+#endif
+
+	nb_rx = rte_eth_rx_burst(fs->rx_port, fs->rx_queue,
+			pkts_burst, nb_pkt_per_burst);
+	if (unlikely(nb_rx == 0))
+		return;
+	fs->rx_packets += nb_rx;
+
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+	fs->rx_burst_stats.pkt_burst_spread[nb_rx]++;
+#endif
+
+	retry = 0;
+	nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue,
+			pkts_burst, nb_rx);
+	while (unlikely(nb_tx < nb_rx) && (retry++ < TX_RETRY_TIMES)) {
+		rte_delay_us(TX_RETRY_WAIT_US);
+		nb_tx += rte_eth_tx_burst(fs->tx_port, fs->tx_queue,
+				&pkts_burst[nb_tx], nb_rx - nb_tx);
+	}
+	fs->tx_packets += nb_tx;
+
+#ifdef RTE_TEST_PMD_RECORD_BURST_STATS
+	fs->tx_burst_stats.pkt_burst_spread[nb_tx]++;
+#endif
+
+	if (unlikely(nb_tx < nb_rx)) {
+		fs->fwd_dropped += (nb_rx - nb_tx);
+		do {
+			rte_pktmbuf_free(pkts_burst[nb_tx]);
+		} while (++nb_tx < nb_rx);
+	}
+
+#ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES
+	end_tsc = rte_rdtsc();
+	core_cycles = (end_tsc - start_tsc);
+	fs->core_cycles = (uint64_t) (fs->core_cycles + core_cycles);
+#endif
+}
+
+struct fwd_engine io_retry_fwd_engine = {
+	.fwd_mode_name  = "io_retry",
+	.port_fwd_begin = NULL,
+	.port_fwd_end   = NULL,
+	.packet_fwd     = pkt_burst_io_retry_forward,
+};
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 26a174c..61abcf8 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -143,6 +143,7 @@  streamid_t nb_fwd_streams;       /**< Is equal to (nb_ports * nb_rxq). */
  */
 struct fwd_engine * fwd_engines[] = {
 	&io_fwd_engine,
+	&io_retry_fwd_engine,
 	&mac_fwd_engine,
 	&mac_retry_fwd_engine,
 	&mac_swap_engine,
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 0f72ca1..fbc18b0 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -221,6 +221,7 @@  struct fwd_engine {
 };
 
 extern struct fwd_engine io_fwd_engine;
+extern struct fwd_engine io_retry_fwd_engine;
 extern struct fwd_engine mac_fwd_engine;
 extern struct fwd_engine mac_retry_fwd_engine;
 extern struct fwd_engine mac_swap_engine;