Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx cycles/packet

["Liang, Cunming" <cunming.liang@intel.com>]

> -----Original Message-----
> From: De Lara Guarch, Pablo
> Sent: Monday, October 13, 2014 8:56 PM
> To: Liang, Cunming; Neil Horman
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx
> cycles/packet
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Liang, Cunming
> > Sent: Sunday, October 12, 2014 12:11 PM
> > To: Neil Horman
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx
> > cycles/packet
> >
> > Hi Neil,
> >
> > Very appreciate your comments.
> > I add inline reply, will send v3 asap when we get alignment.
> >
> > BRs,
> > Liang Cunming
> >
> > > -----Original Message-----
> > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > Sent: Saturday, October 11, 2014 1:52 AM
> > > To: Liang, Cunming
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx
> > cycles/packet
> > >
> > > On Fri, Oct 10, 2014 at 08:29:58PM +0800, Cunming Liang wrote:
> > > > It provides unit test to measure cycles/packet in NIC loopback mode.
> > > > It simply gives the average cycles of IO used per packet without test
> > equipment.
> > > > When doing the test, make sure the link is UP.
> > > >
> > > > Usage Example:
> > > > 1. Run unit test app in interactive mode
> > > >     app/test -c f -n 4 -- -i
> > > > 2. Run and wait for the result
> > > >     pmd_perf_autotest
> > > >
> > > > There's option to choose rx/tx pair, default is vector.
> > > >     set_rxtx_mode [vector|scalar|full|hybrid]
> > > > Note: To get acurate scalar fast, please choose 'vector' or 'hybrid' without
> > > INC_VEC=y in config
> > > >
> > > > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > >
> > > Notes inline
> > >
> > > > ---
> > > >  app/test/Makefile                   |    1 +
> > > >  app/test/commands.c                 |   38 +++
> > > >  app/test/packet_burst_generator.c   |    4 +-
> > > >  app/test/test.h                     |    4 +
> > > >  app/test/test_pmd_perf.c            |  626
> > > +++++++++++++++++++++++++++++++++++
> > > >  lib/librte_pmd_ixgbe/ixgbe_ethdev.c |    6 +
> > > >  6 files changed, 677 insertions(+), 2 deletions(-)
> > > >  create mode 100644 app/test/test_pmd_perf.c
> > > >
> > > > diff --git a/app/test/Makefile b/app/test/Makefile
> > > > index 6af6d76..ebfa0ba 100644
> > > > --- a/app/test/Makefile
> > > > +++ b/app/test/Makefile
> > > > @@ -56,6 +56,7 @@ SRCS-y += test_memzone.c
> > > >
> > > >  SRCS-y += test_ring.c
> > > >  SRCS-y += test_ring_perf.c
> > > > +SRCS-y += test_pmd_perf.c
> > > >
> > > >  ifeq ($(CONFIG_RTE_LIBRTE_TABLE),y)
> > > >  SRCS-y += test_table.c
> > > > diff --git a/app/test/commands.c b/app/test/commands.c
> > > > index a9e36b1..f1e746e 100644
> > > > --- a/app/test/commands.c
> > > > +++ b/app/test/commands.c
> > > > @@ -310,12 +310,50 @@ cmdline_parse_inst_t cmd_quit = {
> > > >
> > > > +#define NB_ETHPORTS_USED                (1)
> > > > +#define NB_SOCKETS                      (2)
> > > > +#define MEMPOOL_CACHE_SIZE 250
> > > > +#define MBUF_SIZE (2048 + sizeof(struct rte_mbuf) +
> > > RTE_PKTMBUF_HEADROOM)
> > > Don't you want to size this in accordance with the amount of data your
> > sending
> > > (64 Bytes as noted above)?
> > [Liang, Cunming] The case is designed to measure small packet IO cost with
> > normal mbuf size.
> > Even if decreasing the size, it won't gain significant cycles.
> > >
> > > > +static void
> > > > +print_ethaddr(const char *name, const struct ether_addr *eth_addr)
> > > > +{
> > > > +	printf("%s%02X:%02X:%02X:%02X:%02X:%02X", name,
> > > > +		eth_addr->addr_bytes[0],
> > > > +		eth_addr->addr_bytes[1],
> > > > +		eth_addr->addr_bytes[2],
> > > > +		eth_addr->addr_bytes[3],
> > > > +		eth_addr->addr_bytes[4],
> > > > +		eth_addr->addr_bytes[5]);
> > > > +}
> > > > +
> > > This was copieed from print_ethaddr.  Seems like a good candidate for a
> > common
> > > function in rte_ether.h
> > [Liang, Cunming] Agree with you, some of samples now use it with the same
> > copy.
> > I'll rework it. Adding 'ether_format_addr' in rte_ether.h only for format the
> > 48bits address output.
> > And leaving other prints for application customization.
> > >
> > >
> > > > +}
> > > > +
> > > > +static void
> > > > +signal_handler(int signum)
> > > > +{
> > > > +	/* When we receive a USR1 signal, print stats */
> > > I think you mean SIGUSR2, below, SIGUSR1 tears the test down and exits
> > the
> > > program
> > [Liang, Cunming] Thanks, it's a typo.
> > >
> > > > +	if (signum == SIGUSR1) {
> > > SIGINT instead.  Thats the common practice.
> > [Liang, Cunming] I understood your opinion.
> > The considerations I'm not using SIGINT instead are:
> > 1. We unset ISIG in c_lflag of term. CRTL+C won't trigger SIGINT in command
> > interactive.
> >   It always has to explicitly send signal. No matter SIGUSR1 or SIGINT.
> > 2. By SIGINT semantic, expect to terminate the process.
> >   Here I expect to force stop this case, but still alive in command line.
> >   After it stopped, it can run again or start to run other test cases.
> >   So I keep SIGINT, SIGUSR1 in different behavior.
> > 3. It should be rarely used.
> >   Only when exception timeout, I leave this backdoor for automation test
> > control.
> >   For manual test, we can easily force kill the process.
> >
> > >
> > > > +		printf("Force Stop!\n");
> > > > +		stop = 1;
> > > > +	}
> > > > +	if (signum == SIGUSR2)
> > > > +		stats_display(0);
> > > > +}
> > > > +/* main processing loop */
> > > > +static int
> > > > +main_loop(__rte_unused void *args)
> > > > +{
> > > > +#define PACKET_SIZE 64
> > > > +#define FRAME_GAP 12
> > > > +#define MAC_PREAMBLE 8
> > > > +	struct rte_mbuf *pkts_burst[MAX_PKT_BURST];
> > > > +	unsigned lcore_id;
> > > > +	unsigned i, portid, nb_rx = 0, nb_tx = 0;
> > > > +	struct lcore_conf *conf;
> > > > +	uint64_t prev_tsc, cur_tsc;
> > > > +	int pkt_per_port;
> > > > +	uint64_t packets_per_second, total_packets;
> > > > +
> > > > +	lcore_id = rte_lcore_id();
> > > > +	conf = &lcore_conf[lcore_id];
> > > > +	if (conf->status != LCORE_USED)
> > > > +		return 0;
> > > > +
> > > > +	pkt_per_port =  MAX_TRAFIC_BURST / conf->nb_ports;
> > > > +
> > > > +	int idx = 0;
> > > > +	for (i = 0; i < conf->nb_ports; i++) {
> > > > +		int num = pkt_per_port;
> > > > +		portid = conf->portlist[i];
> > > > +		printf("inject %d packet to port %d\n", num, portid);
> > > > +		while (num) {
> > > > +			nb_tx = RTE_MIN(MAX_PKT_BURST, num);
> > > > +			nb_tx = rte_eth_tx_burst(portid, 0,
> > > > +						&tx_burst[idx], nb_tx);
> > > > +			num -= nb_tx;
> > > > +			idx += nb_tx;
> > > > +		}
> > > > +	}
> > > > +	printf("Total packets inject to prime ports = %u\n", idx);
> > > > +
> > > > +	packets_per_second = (link_mbps * 1000 * 1000) /
> > > > +		+((PACKET_SIZE + FRAME_GAP + MAC_PREAMBLE) *
> > CHAR_BIT);
> > > > +	printf("Each port will do %"PRIu64" packets per second\n",
> > > > +		+packets_per_second);
> > > > +
> > > > +	total_packets = RTE_TEST_DURATION * conf->nb_ports *
> > > packets_per_second;
> > > > +	printf("Test will stop after at least %"PRIu64" packets received\n",
> > > > +		+ total_packets);
> > > > +
> > > > +	prev_tsc = rte_rdtsc();
> > > > +
> > > > +	while (likely(!stop)) {
> > > > +		for (i = 0; i < conf->nb_ports; i++) {
> > > > +			portid = conf->portlist[i];
> > > > +			nb_rx = rte_eth_rx_burst((uint8_t) portid, 0,
> > > > +						 pkts_burst,
> > MAX_PKT_BURST);
> > > > +			if (unlikely(nb_rx == 0)) {
> > > > +				idle++;
> > > > +				continue;
> > > > +			}
> > > > +
> > > > +			count += nb_rx;
> > > Doesn't take into consideration error conditions.  rte_eth_rx_burst can
> > return
> > > -ENOTSUP
> > [Liang, Cunming] It returns -ENOTSUP when turning on ETHDEV_DEBUG
> > CONFIG.
> > The error is used to avoid no function call register.
> > When ETHDEV_DEBUG turn off, the NULL function call cause segfault
> > directly.
> > So I think it's a library internal error.
> > In such library exceptional case, I prefer not expecting sample/application to
> > condition check library functional error.
> > >
> > > > +			nb_tx = rte_eth_tx_burst(portid, 0, pkts_burst,
> > nb_rx);
> > > Ditto with -ENOTSUP
> > >
> > > > +			if (unlikely(nb_tx < nb_rx)) {
> > > What makes this unlikely?  Seems like a perfectly reasonable condition to
> > happen
> > > to me.  If the network is busy, its completely likely that you will receive
> > more
> > > frames than you send, if you elect to receive all frames.
> > [Liang, Cunming] For this case, NIC works in MAC loopback mode.
> > It firstly injects numbers of packets to NIC. Then NIC will loopback all packets
> > to ingress side.
> > The code here will receive the packets, and send them back to NIC
> > Packets looping inside all come from initial injection.
> > As the total number of injected packets is much less than in-chip queue size,
> > the tx egress queue shouldn't block desc. ring update.
> > So when receiving packets in line rate and nb_tx < nb_rx, it means tx cannot
> > archive line rate.
> > When it happens, the cycles/packets result make no sense, as the bottle
> > neck is NIC.
> > The drop counter can record it.
> > >
> > > > +				drop += (nb_rx - nb_tx);
> > > > +				do {
> > > > +
> > 	rte_pktmbuf_free(pkts_burst[nb_tx]);
> > > Defer this, it skews your timing
> > [Liang, Cunming] Agree with you, I ever thought about it.
> > This test cases is designed to measure pure IO RX/TX routine.
> > When doing tx burst with result nb_tx < nb_rx, we either repeat re-send or
> > drop it.
> > Each way introduces noise(adding additional control code), resending much
> > times even cost more than free it.
> > The cycles/packets is useful when there's no packet drop, otherwise it gives
> > the hint where the problem comes from (by idle or drop).
> > >
> > > > +				} while (++nb_tx < nb_rx);
> > > > +			}
> > > > +		}
> > > > +		if (unlikely(count >= total_packets))
> > > > +			break;
> > > Whats the reasoning here?  Do you only ever expect to receive frames that
> > you
> > > send?  If so, seems like this should call for a big warning to get printed.
> > [Liang, Cunming] The loop exits when the pre-calculated total_packets are
> > received.
> > As the nb_rx is unpredictable, the packet counter may large equal than
> > total_packets the last time.
> > The reason unlikely used here is because the condition becomes true only
> > the last time.
> > >
> > > > +	}
> > > > +
> > > > +	cur_tsc = rte_rdtsc();
> > > > +
> > > > +	for (i = 0; i < conf->nb_ports; i++) {
> > > > +		portid = conf->portlist[i];
> > > > +		int nb_free = pkt_per_port;
> > > > +		do { /* dry out */
> > > > +			nb_rx = rte_eth_rx_burst((uint8_t) portid, 0,
> > > > +						 pkts_burst,
> > MAX_PKT_BURST);
> > > > +			nb_tx = 0;
> > > > +			while (nb_tx < nb_rx)
> > > > +				rte_pktmbuf_free(pkts_burst[nb_tx++]);
> > > > +			nb_free -= nb_rx;
> > > > +		} while (nb_free != 0);
> > > > +		printf("free %d mbuf left in port %u\n", pkt_per_port,
> > portid);
> > > > +	}
> > > > +
> > > Whats the purpose of this?  Are you trying to flush the device?  Wouldn't it
> > be
> > > enough just to stop the interface?
> > [Liang, Cunming] If we only run the cases once and exit, it doesn't matter.
> > But it's designed to run multi-times without exit, for the purpose of warming
> > up or for getting average number.
> > So stopping device is not enough, we have to release the flying packets.
> >
> > >
> > > > +	if (count == 0)
> > > > +		return -1;
> > > > +
> > > > +	printf("%lu packet, %lu drop, %lu idle\n", count, drop, idle);
> > > > +	printf("Result: %ld cycles per packet\n", (cur_tsc - prev_tsc) / count);
> > > > +
> > > Bad math here.  Theres no guarantee that the tsc hasn't wrapped
> > (potentially
> > > more than once) depending on your test length.  you need to check the tsc
> > before
> > > and after each burst and record an average of deltas instead, accounting in
> > each
> > > instance for the possibility of wrap.
> > [Liang, Cunming] I'm not sure catch your point correctly.
> > I think both cur_tsc and prev_tsc are 64 bits width.
> > For 3GHz, I think it won't wrapped so quick.
> > As it's uint64_t, so even get wrapped, the delta should still be correct.
> 
> You need to change those %lu to %PRIu64, or you will get a compilation error
> when
> using 32-bit targets, since those variables are uint64_t. There are other parts of
> the
> code with same problem, as well.
[Liang, Cunming] Make sense to me. Thanks.
> 
> 
> > >
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +test_pmd_perf(void)
> > > > +{
> > > > +	uint16_t nb_ports, num, nb_lcores, slave_id = (uint16_t)-1;
> > > > +	uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT;
> > > > +	uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT;
> > > > +	uint16_t portid;
> > > > +	uint16_t nb_rx_queue = 1, nb_tx_queue = 1;
> > > > +	int socketid = -1;
> > > > +	int ret;
> > > > +
> > > > +	printf("Start PMD RXTX cycles cost test.\n");
> > > > +
> > > > +	signal(SIGUSR1, signal_handler);
> > > Again SIGINT here.
> > >
> > > > +	signal(SIGUSR2, signal_handler);
> > > > +
> > > > +	nb_ports = rte_eth_dev_count();
> > > > +	if (nb_ports < NB_ETHPORTS_USED) {
> > > > +		printf("At least %u port(s) used for perf. test\n",
> > > > +		       NB_ETHPORTS_USED);
> > > > +		return -1;
> > > > +	}
> > > > +
> > > > +	if (nb_ports > RTE_MAX_ETHPORTS)
> > > > +		nb_ports = RTE_MAX_ETHPORTS;
> > > > +
> > > > +	nb_lcores = rte_lcore_count();
> > > > +
> > > > +	memset(lcore_conf, 0, sizeof(lcore_conf));
> > > > +	init_lcores();
> > > > +
> > > > +	init_mbufpool(NB_MBUF);
> > > > +
> > > > +	reset_count();
> > > > +	num = 0;
> > > > +	for (portid = 0; portid < nb_ports; portid++) {
> > > > +		if (socketid == -1) {
> > > > +			socketid = rte_eth_dev_socket_id(portid);
> > > > +			slave_id = alloc_lcore(socketid);
> > > > +			if (slave_id == (uint16_t)-1) {
> > > > +				printf("No avail lcore to run test\n");
> > > > +				return -1;
> > > > +			}
> > > > +			printf("Performance test runs on lcore %u socket
> > %u\n",
> > > > +			       slave_id, socketid);
> > > > +		}
> > > > +
> > > > +		if (socketid != rte_eth_dev_socket_id(portid)) {
> > > > +			printf("Skip port %d\n", portid);
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		/* port configure */
> > > > +		ret = rte_eth_dev_configure(portid, nb_rx_queue,
> > > > +					    nb_tx_queue, &port_conf);
> > > > +		if (ret < 0)
> > > > +			rte_exit(EXIT_FAILURE,
> > > > +				"Cannot configure device: err=%d,
> > port=%d\n",
> > > > +				 ret, portid);
> > > > +
> > > > +		rte_eth_macaddr_get(portid, &ports_eth_addr[portid]);
> > > > +		printf("Port %u ", portid);
> > > > +		print_ethaddr("Address:", &ports_eth_addr[portid]);
> > > > +		printf("\n");
> > > > +
> > > > +		/* tx queue setup */
> > > > +		ret = rte_eth_tx_queue_setup(portid, 0, nb_txd,
> > > > +					     socketid, &tx_conf);
> > > > +		if (ret < 0)
> > > > +			rte_exit(EXIT_FAILURE,
> > > > +				"rte_eth_tx_queue_setup: err=%d, "
> > > > +				"port=%d\n", ret, portid);
> > > > +
> > > > +		/* rx queue steup */
> > > > +		ret = rte_eth_rx_queue_setup(portid, 0, nb_rxd,
> > > > +						socketid, &rx_conf,
> > > > +						mbufpool[socketid]);
> > > > +		if (ret < 0)
> > > > +			rte_exit(EXIT_FAILURE, "rte_eth_rx_queue_setup:
> > err=%d,"
> > > > +				 "port=%d\n", ret, portid);
> > > > +
> > > > +		/* Start device */
> > > > +		stop = 0;
> > > > +		ret = rte_eth_dev_start(portid);
> > > > +		if (ret < 0)
> > > > +			rte_exit(EXIT_FAILURE,
> > > > +				"rte_eth_dev_start: err=%d, port=%d\n",
> > > > +				ret, portid);
> > > > +
> > > > +		/* always eanble promiscuous */
> > > > +		rte_eth_promiscuous_enable(portid);
> > > > +
> > > > +		lcore_conf[slave_id].portlist[num++] = portid;
> > > > +		lcore_conf[slave_id].nb_ports++;
> > > > +	}
> > > > +	check_all_ports_link_status(nb_ports, RTE_PORT_ALL);
> > > > +
> > > > +	init_traffic(mbufpool[socketid], tx_burst, MAX_TRAFIC_BURST);
> > > > +
> > > > +	rte_eal_remote_launch(main_loop, NULL, slave_id);
> > > > +	if (rte_eal_wait_lcore(slave_id) < 0)
> > > > +		return -1;
> > > > +
> > > > +	/* port tear down */
> > > > +	for (portid = 0; portid < nb_ports; portid++) {
> > > > +		if (socketid != rte_eth_dev_socket_id(portid))
> > > > +			continue;
> > > > +
> > > > +		rte_eth_dev_stop(portid);
> > > > +	}
> > > > +
> > > Clean up your allocated memory/lcores/etc?
> > [Liang, Cunming] It do cleanup on the beginning of case.
> > "Init_lcores","init_mbufpool","reset_count" guarantee the data clean before
> > each testing.
> > And mbufpool only allocated once even if we run multiple times.
> > >
> > > Neil