It's reasonable to me. I'll make a patch for rte_ethdev.c.
> -----Original Message----- > From: Richardson, Bruce > Sent: Wednesday, October 22, 2014 11:10 PM > To: Ananyev, Konstantin; Neil Horman; Liang, Cunming > Cc: dev at 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 at dpdk.org] On Behalf Of Ananyev, Konstantin > > Sent: Wednesday, October 22, 2014 3:53 PM > > To: Neil Horman; Liang, Cunming > > Cc: dev at 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 at dpdk.org] On Behalf Of Neil Horman > > > Sent: Wednesday, October 22, 2014 3:03 PM > > > To: Liang, Cunming > > > Cc: dev at 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 at tuxdriver.com] > > > > > Sent: Tuesday, October 21, 2014 6:33 PM > > > > > To: Liang, Cunming > > > > > Cc: dev at 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 at tuxdriver.com] > > > > > > > Sent: Saturday, October 11, 2014 1:52 AM > > > > > > > To: Liang, Cunming > > > > > > > Cc: dev at 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