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: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Wednesday, October 22, 2014 10:03 PM
> 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 Tue, Oct 21, 2014 at 01:17:01PM +0000, Liang, Cunming wrote:
> >
> >
> > > -----Original Message-----
> > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > Sent: Tuesday, October 21, 2014 6:33 PM
> > > 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 Sun, Oct 12, 2014 at 11:10:39AM +0000, Liang, Cunming wrote:
> > > > 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.
> > > > >
> > > That presumes a non-memory constrained system, doesn't it?  I suppose in
> the
> > > end
> > > as long as you have consistency, its not overly relevant, but it seems like
> > > you'll want to add data sizing as a feature to this eventually (i.e. the ability
> > > to test performance for larger frames sizes), at which point you'll need to
> make
> > > this non-static anyway.
> > [Liang, Cunming] For a normal Ethernet packet(w/o jumbo frame), packet size is
> 1518B.
> > As in really network, there won't have huge number of jumbo frames.
> > The mbuf size 2048 is a reasonable value to cover most of the packet size.
> > It's also be chosen by lots of NIC as the default receiving buffer size in DMA
> register.
> > In case larger than the size, it need do scatter and gather but lose some
> performance.
> > The unit test won't measure size from 64 to 9600, won't plan to measure
> scatter-gather rx/tx.
> > It focus on 64B packet size and taking the mbuf size being used the most often.
> Fine.
> 
> > >
> > > > > > +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.
> > > > >
> > > Sounds good.
> > >
> > > > >
> > > > > > +}
> > > > > > +
> > > > > > +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.
> > > >
> > > Hmm, ok, that sounds reasonable.
> > >
> > > > >
> > > > > > +		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.
> > > No, look at FUNC_PTR_OR_ERR_RET.  If the passed in function pointer is null,
> > > -ENOTSUPP will be returned to the application, you need to handle the error
> > > condition.
> > [Liang, Cunming] The runtime rte_eth_rx_burst is a inline function in
> rte_ethdev.h.
> > The one you're talking about is the one defined in rte_ethdev.c which is a
> extern non-inline function.
> > It works only when RTE_LIBRTE_ETHDEV_DEBUG turns on.
> > If we always turns such library internal checking on, it lose performance.
> > So I insist it's a library internal error checking, doesn't need to take care in
> application level.
> I'm really flored that you would respond this way.
> 
> What you have is two versions of a function, one returns errors and one doesn't,
> and the version used is based on compile time configuration.  You've written
> your application such that it can only gracefully handle one of the two
> versions, and you're happy to allow the other version, the one thats supposed to
> be in use when you are debugging applications, to create silent accounting
> failures in your application.  And it completely ignores the fact that the
> non-debug version might one day return errors as well (even if it doesn't
> currently).  Your assertion above that we can ignore it because its debug code
> is the most short sighted thing I've read in a long time.
> 
[Liang, Cunming] If as you said, two version of function, one returns errors and one doesn't.
I'll agree with your point. But in fact in either one, will cause error.
Non-debug version will just segment fault as calling NULL function pointer.
Debug version will print the error root cause.
If you searching the rte_eth_rx_burst calling in DPDK, it's used in every sample, no one checks it.
This function call is in fast path, I don't think it's a good idea pay additional cycles on such branch condition.
> > >
> > > > In such library exceptional case, I prefer not expecting sample/application to
> > > condition check library functional error.
> > > But you're assertion about the library handling the exception is wrong.
> > >
> > > > >
> > > > > > +			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.
> > > Ok.
> > >
> > > > >
> > > > > > +				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).
> > > I'm not sure what you're asserting here.  Are you suggesting that you want
> to
> > > include the time it takes to free memory buffers in your testing?  That seems
> > > dubious at best to me.  If you want to measure I/O, thats great, but memory
> > > management of packet buffers is a separate operation from that I/O
> > [Liang, Cunming] Agree with you. I means it doesn't need to take care of the
> mbuf free cost.
> > As I said in previous reply, it rarely happens in line rate.
> > The cycle measurement doesn't make much sense if happens.
> > On that time the unit test just notify it happens, and keep safe free all the
> unsent mbuf.
> >
> > >
> > > > >
> > > > > > +				} 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.
> > > ok
> > >
> > > > >
> > > > > > +	}
> > > > > > +
> > > > > > +	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.
> > > >
> > > Ok
> > >
> > > > >
> > > > > > +	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.
> > > But theres no guarantee that the tsc starts at zero when you begin your test.
> > > The system may have been up for a long time and near wrapping already.
> > > Regardless, you need to account for the possibility that cur_tsc is smaller
> > > than prev_tsc, or this breaks.
> > [Liang, Cunming] In case prev_tsc near wrapping, and cur_tsc get wrapped.
> > As they are unsigned, it still ok.
> > e.g. cur_tsc=0x2, prev_tsc=0xFFFFFFFFFFFFFFFC
> > Delta=cur_tsc-prev_tsc is 6, which is still correct.
> Ah, you're right, my fault.
> > And for uint64_t, we need to start computer for hundreds of years.
> That only assumes that the tsc has been kept at its initial state from boot, its
> entirely possible that the tsc is writen to a different value.  IIRC, in several
> debug configurations, the linux kernel inits the tsc to a large value so that it
> rolls over shortly after boot to catch wrapping errors.  Though as you note,
> your used of uint64 makes it irrelevant.
[Liang, Cunming] Ok, you're right. 
I think we've already got agreement on delta of tsc.
So we can close this one.
> 
> > >
> > > > >
> > > > > > +	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.
> > > Its a janitorial issue.  Before the program exits, you need to free any
> > > resources that you've allocated.
> > [Liang, Cunming] Yes, I do but only not for the mbuf pool.
> > Firstly the case exit is not equal to program exit. I expect the re-enter will use
> the same mempool, so that can do cache warm up.
> > Secondly, DPDK doesn't supply really mempool release. The program exit, the
> memzone then free.
> I continue to find the fact that freeing mempools isn't supported is rediculous,
> but I suppose thats neither here nor there, you need to be able to clean up
> resources that you allocate, weather you are exiting a test case, a program, or
> simply a function, its just not sane to not be able to do that.
[Liang, Cunming] Got it, agree with you on the principle.
> 
> Given the return code check above, all thats left to say I think is NAK to this
> code.  If Thomas wants to ignore me, I suppose thats his perogative, but I can't
> see how you can ignore that case.
> 
> Neil