> -----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