From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 1DF335955 for ; Fri, 24 Oct 2014 04:59:52 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 23 Oct 2014 20:08:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,778,1406617200"; d="scan'208";a="619579346" Received: from pgsmsx103.gar.corp.intel.com ([10.221.44.82]) by fmsmga002.fm.intel.com with ESMTP; 23 Oct 2014 20:08:17 -0700 Received: from pgsmsx104.gar.corp.intel.com (10.221.44.91) by PGSMSX103.gar.corp.intel.com (10.221.44.82) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 24 Oct 2014 11:06:38 +0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by PGSMSX104.gar.corp.intel.com (10.221.44.91) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 24 Oct 2014 11:06:38 +0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.156]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.207]) with mapi id 14.03.0195.001; Fri, 24 Oct 2014 11:06:37 +0800 From: "Liang, Cunming" To: "Richardson, Bruce" , "Ananyev, Konstantin" , Neil Horman Thread-Topic: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx cycles/packet Thread-Index: AQHP5IYD5aCkJuEDxEurfcum8uYUPpwpFr4AgALSmBCADfxZgIAApRJAgAEn8ACAAA3lAIAABKQAgALfhMA= Date: Fri, 24 Oct 2014 03:06:36 +0000 Message-ID: References: <1408947174-11323-1-git-send-email-cunming.liang@intel.com> <1412944201-30703-1-git-send-email-cunming.liang@intel.com> <1412944201-30703-2-git-send-email-cunming.liang@intel.com> <20141010175226.GG19499@hmsreliant.think-freely.org> <20141021103315.GB12795@hmsreliant.think-freely.org> <20141022140316.GA13913@localhost.localdomain> <2601191342CEEE43887BDE71AB977258213955A6@IRSMSX105.ger.corp.intel.com> <59AF69C657FD0841A61C55336867B5B034421C3A@IRSMSX103.ger.corp.intel.com> In-Reply-To: <59AF69C657FD0841A61C55336867B5B034421C3A@IRSMSX103.ger.corp.intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx cycles/packet X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Oct 2014 02:59:56 -0000 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@dpdk.org > Subject: RE: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx > cycles/packet >=20 >=20 >=20 > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstanti= n > > 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 t= x > > 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 fo= r rx and tx > > > > > cycles/packet > > > > > > > > <...snip...> > > > > > > > > > > > > > > > + printf("Force Stop!\n"); > > > > > > > > + stop =3D 1; > > > > > > > > + } > > > > > > > > + if (signum =3D=3D 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 =3D 0, nb_tx =3D 0; > > > > > > > > + struct lcore_conf *conf; > > > > > > > > + uint64_t prev_tsc, cur_tsc; > > > > > > > > + int pkt_per_port; > > > > > > > > + uint64_t packets_per_second, total_packets; > > > > > > > > + > > > > > > > > + lcore_id =3D rte_lcore_id(); > > > > > > > > + conf =3D &lcore_conf[lcore_id]; > > > > > > > > + if (conf->status !=3D LCORE_USED) > > > > > > > > + return 0; > > > > > > > > + > > > > > > > > + pkt_per_port =3D MAX_TRAFIC_BURST / conf->nb_ports; > > > > > > > > + > > > > > > > > + int idx =3D 0; > > > > > > > > + for (i =3D 0; i < conf->nb_ports; i++) { > > > > > > > > + int num =3D pkt_per_port; > > > > > > > > + portid =3D conf->portlist[i]; > > > > > > > > + printf("inject %d packet to port %d\n", num, portid); > > > > > > > > + while (num) { > > > > > > > > + nb_tx =3D RTE_MIN(MAX_PKT_BURST, num); > > > > > > > > + nb_tx =3D rte_eth_tx_burst(portid, 0, > > > > > > > > + &tx_burst[idx], nb_tx); > > > > > > > > + num -=3D nb_tx; > > > > > > > > + idx +=3D nb_tx; > > > > > > > > + } > > > > > > > > + } > > > > > > > > + printf("Total packets inject to prime ports =3D %u\n", id= x); > > > > > > > > + > > > > > > > > + packets_per_second =3D (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 =3D RTE_TEST_DURATION * conf->nb_ports * > > > > > > > packets_per_second; > > > > > > > > + printf("Test will stop after at least %"PRIu64" packets > > received\n", > > > > > > > > + + total_packets); > > > > > > > > + > > > > > > > > + prev_tsc =3D rte_rdtsc(); > > > > > > > > + > > > > > > > > + while (likely(!stop)) { > > > > > > > > + for (i =3D 0; i < conf->nb_ports; i++) { > > > > > > > > + portid =3D conf->portlist[i]; > > > > > > > > + nb_rx =3D rte_eth_rx_burst((uint8_t) portid, 0, > > > > > > > > + pkts_burst, > > MAX_PKT_BURST); > > > > > > > > + if (unlikely(nb_rx =3D=3D 0)) { > > > > > > > > + idle++; > > > > > > > > + continue; > > > > > > > > + } > > > > > > > > + > > > > > > > > + count +=3D 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_DEB= UG > > > > > CONFIG. > > > > > > The error is used to avoid no function call register. > > > > > > When ETHDEV_DEBUG turn off, the NULL function call cause segfau= lt > > directly. > > > > > > So I think it's a library internal error. > > > > > No, look at FUNC_PTR_OR_ERR_RET. If the passed in function point= er 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 whi= ch 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 perfo= rmance. > > > > 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 o= ne > > 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 t= wo > > > 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 accou= nting > > > failures in your application. And it completely ignores the fact tha= t the > > > non-debug version might one day return errors as well (even if it doe= sn't > > > currently). Your assertion above that we can ignore it because its d= ebug 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 >=3D dev->data->nb_rx_queues) { > > PMD_DEBUG_TRACE("Invalid RX queue_id=3D%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 initia= lly > > planned). > > > > Konstantin >=20 > Agreed. For any errors other than no-packets available yet, we can also c= onsider > setting rte_errno when returning 0. >=20 > /Bruce