From: "Richardson, Bruce" <bruce.richardson@intel.com> To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Neil Horman <nhorman@tuxdriver.com>, "Liang, Cunming" <cunming.liang@intel.com> Cc: "dev@dpdk.org" <dev@dpdk.org> Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx cycles/packet Date: Wed, 22 Oct 2014 15:09:36 +0000 Message-ID: <59AF69C657FD0841A61C55336867B5B034421C3A@IRSMSX103.ger.corp.intel.com> (raw) In-Reply-To: <2601191342CEEE43887BDE71AB977258213955A6@IRSMSX105.ger.corp.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
next prev parent reply other threads:[~2014-10-22 15:02 UTC|newest] Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-08-25 6:12 [dpdk-dev] [PATCH 0/5] app/test: unit test to measure cycles per packet Cunming Liang 2014-08-25 6:12 ` [dpdk-dev] [PATCH 1/5] app/test: unit test for rx and tx cycles/packet Cunming Liang 2014-08-25 6:12 ` [dpdk-dev] [PATCH 2/5] app/test: measure standalone rx or " Cunming Liang 2014-08-25 6:12 ` [dpdk-dev] [PATCH 3/5] ixgbe/vpmd: add fix to store unaligned mbuf point array Cunming Liang 2014-08-25 6:12 ` [dpdk-dev] [PATCH 4/5] app/test: add unit test to measure RX burst cycles Cunming Liang 2014-08-25 6:12 ` [dpdk-dev] [PATCH 5/5] app/test: allow to create packets in different sizes Cunming Liang 2014-09-03 1:46 ` [dpdk-dev] [PATCH 0/5] app/test: unit test to measure cycles per packet Zhan, Zhaochen 2014-10-10 12:29 ` [dpdk-dev] [PATCH v2 0/4] " Cunming Liang 2014-10-10 12:29 ` [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx cycles/packet Cunming Liang 2014-10-10 17:52 ` Neil Horman 2014-10-12 11:10 ` Liang, Cunming [not found] ` <E115CCD9D858EF4F90C690B0DCB4D8972262B720@IRSMSX108.ger.corp.intel.com> 2014-10-14 0:54 ` Liang, Cunming 2014-10-21 10:33 ` Neil Horman 2014-10-21 10:43 ` Richardson, Bruce 2014-10-21 13:37 ` Neil Horman 2014-10-21 22:43 ` Ananyev, Konstantin 2014-10-21 13:17 ` Liang, Cunming 2014-10-22 14:03 ` Neil Horman 2014-10-22 14:48 ` Liang, Cunming 2014-10-22 14:53 ` Ananyev, Konstantin 2014-10-22 15:09 ` Richardson, Bruce [this message] 2014-10-24 3:06 ` Liang, Cunming 2014-10-10 12:29 ` [dpdk-dev] [PATCH v2 2/4] app/test: measure standalone rx or " Cunming Liang 2014-10-10 12:30 ` [dpdk-dev] [PATCH v2 3/4] app/test: add unit test to measure RX burst cycles Cunming Liang 2014-10-10 12:30 ` [dpdk-dev] [PATCH v2 4/4] app/test: allow to create packets in different sizes Cunming Liang 2014-10-20 8:13 ` [dpdk-dev] [PATCH v3 0/2] app/test: unit test to measure cycles per packet Cunming Liang 2014-10-20 8:13 ` [dpdk-dev] [PATCH v3 1/2] app/test: allow to create packets in different sizes Cunming Liang 2014-10-20 8:13 ` [dpdk-dev] [PATCH v3 2/2] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang 2014-10-21 2:40 ` [dpdk-dev] [PATCH v3 0/2] app/test: unit test to measure cycles per packet Liu, Yong 2014-10-24 5:39 ` [dpdk-dev] [PATCH v4 0/3] " Cunming Liang 2014-10-24 5:39 ` [dpdk-dev] [PATCH v4 1/3] app/test: allow to create packets in different sizes Cunming Liang 2014-10-24 5:39 ` [dpdk-dev] [PATCH v4 2/3] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang 2014-10-24 5:39 ` [dpdk-dev] [PATCH v4 3/3] ethdev: fix wrong error return refer to API definition Cunming Liang 2014-10-24 5:57 ` [dpdk-dev] [PATCH v5 0/3] app/test: unit test to measure cycles per packet Cunming Liang 2014-10-24 5:58 ` [dpdk-dev] [PATCH v5 1/3] app/test: allow to create packets in different sizes Cunming Liang 2014-10-24 5:58 ` [dpdk-dev] [PATCH v5 2/3] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang 2014-10-24 5:58 ` [dpdk-dev] [PATCH v5 3/3] ethdev: fix wrong error return refere to API definition Cunming Liang 2014-10-27 1:20 ` [dpdk-dev] [PATCH v6 0/3] app/test: unit test to measure cycles per packet Cunming Liang 2014-10-27 1:20 ` [dpdk-dev] [PATCH v6 1/3] app/test: allow to create packets in different sizes Cunming Liang 2014-10-27 1:20 ` [dpdk-dev] [PATCH v6 2/3] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang 2014-11-11 23:28 ` Thomas Monjalon 2014-11-12 6:32 ` Liang, Cunming 2014-10-27 1:20 ` [dpdk-dev] [PATCH v6 3/3] ethdev: fix wrong error return refere to API definition Cunming Liang 2014-10-27 16:03 ` Ananyev, Konstantin 2014-10-27 1:45 ` [dpdk-dev] [PATCH v6 0/3] app/test: unit test to measure cycles per packet Liu, Yong 2014-10-29 5:06 ` Liang, Cunming 2014-11-12 6:24 ` [dpdk-dev] [PATCH v7 0/7] " Cunming Liang 2014-11-12 6:24 ` [dpdk-dev] [PATCH v7 1/7] app/test: allow to create packets in different sizes Cunming Liang 2014-11-12 6:24 ` [dpdk-dev] [PATCH v7 2/7] ixgbe:clean scattered_rx configure in dev_stop Cunming Liang 2014-11-12 7:53 ` Thomas Monjalon 2014-11-12 8:21 ` Liang, Cunming 2014-11-12 9:24 ` Thomas Monjalon 2014-11-12 10:29 ` Liang, Cunming 2014-11-12 10:32 ` Thomas Monjalon 2014-11-12 10:42 ` Liang, Cunming 2014-11-12 6:24 ` [dpdk-dev] [PATCH v7 3/7] ether: new API to format eth_addr in string Cunming Liang 2014-11-12 6:24 ` [dpdk-dev] [PATCH v7 4/7] app/testpmd: cleanup eth_addr print Cunming Liang 2014-11-12 6:24 ` [dpdk-dev] [PATCH v7 5/7] examples: " Cunming Liang 2014-11-12 6:24 ` [dpdk-dev] [PATCH v7 6/7] app/test: measure the cost of rx/tx routines by cycle number Cunming Liang 2014-11-12 6:24 ` [dpdk-dev] [PATCH v7 7/7] ethdev: fix wrong error return refere to API definition Cunming Liang 2014-11-12 23:50 ` [dpdk-dev] [PATCH v7 0/7] app/test: unit test to measure cycles per packet Thomas Monjalon 2014-10-24 5:59 ` [dpdk-dev] [PATCH v4 0/3] " Liang, Cunming [not found] ` <1414130090-17910-1-git-send-email-y> [not found] ` <1414130090-17910-4-git-send-email-y> 2014-10-24 11:04 ` [dpdk-dev] [PATCH v5 3/3] ethdev: fix wrong error return refere to API definition Ananyev, Konstantin 2014-10-27 0:58 ` Liang, Cunming 2014-10-28 12:21 ` [dpdk-dev] [PATCH v2 0/4] app/test: unit test to measure cycles per packet Neil Horman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=59AF69C657FD0841A61C55336867B5B034421C3A@IRSMSX103.ger.corp.intel.com \ --to=bruce.richardson@intel.com \ --cc=cunming.liang@intel.com \ --cc=dev@dpdk.org \ --cc=konstantin.ananyev@intel.com \ --cc=nhorman@tuxdriver.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git