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

["Richardson, Bruce" <bruce.richardson@intel.com>]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Wednesday, October 22, 2014 3:53 PM
> To: Neil Horman; Liang, Cunming
> 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 Neil Horman
> > Sent: Wednesday, October 22, 2014 3: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
> > > > > >
<...snip...>
> > > > > >
> > > > > > > +		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.
> 
> Actually looking at rte_eth_rx_burst(): it returns uint16_t.
> Which is sort of a strange if it expects to return a negative value as error code.
> Also reading though 'formal' comments of rte_eth_rx_burst() - it seems that it
> not supposed to return negative values:
> "...
> The rte_eth_rx_burst() function does not provide any error
>  notification to avoid the corresponding overhead. As a hint, the
>  upper-level application might check the status of the device link once
>  being systematically returned a 0 value for a given number of tries.
> ...
> @return
>  *   The number of packets actually retrieved, which is the number
>  *   of pointers to *rte_mbuf* structures effectively supplied to the
>  *   *rx_pkts* array."
> 
> Again, if you looks at rte_eth_rx_burst() implementation, when
> RTE_LIBRTE_ETHDEV_DEBUG is on -
> For some error cases it returns '0', foor other's: -ENOTSUP:
> 
> FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, -ENOTSUP);
> if (queue_id >= dev->data->nb_rx_queues) {
>                 PMD_DEBUG_TRACE("Invalid RX queue_id=%d\n", queue_id);
>                 return 0;
> }
> 
> Which makes me thing that we just have errnoneous implementation of
> rte_eth_rx_burst()
> when RTE_LIBRTE_ETHDEV_DEBUG is on.
> Probably just a mechanical mistake, when FUNC_PTR_OR_ERR_RET() was
> added.
>  I'd say we change rte_eth_rx_burst() to always return 0 (as was initially
> planned).
> 
> Konstantin

Agreed. For any errors other than no-packets available yet, we can also consider setting rte_errno when returning 0. 

/Bruce