From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id D50337E7B for ; Wed, 22 Oct 2014 17:02:23 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP; 22 Oct 2014 08:10:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,769,1406617200"; d="scan'208";a="609350875" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by fmsmga001.fm.intel.com with ESMTP; 22 Oct 2014 08:10:02 -0700 Received: from irsmsx108.ger.corp.intel.com (163.33.3.3) by IRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP Server (TLS) id 14.3.195.1; Wed, 22 Oct 2014 16:09:36 +0100 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.175]) by IRSMSX108.ger.corp.intel.com ([169.254.11.21]) with mapi id 14.03.0195.001; Wed, 22 Oct 2014 16:09:36 +0100 From: "Richardson, Bruce" To: "Ananyev, Konstantin" , Neil Horman , "Liang, Cunming" Thread-Topic: [dpdk-dev] [PATCH v2 1/4] app/test: unit test for rx and tx cycles/packet Thread-Index: AQHP5LMIsQNGJDdAuUyOgB+c6Fi2fpwsQCWAgA4aiICAAC3CgIABn0EAgAAN5QCAABThoA== Date: Wed, 22 Oct 2014 15:09:36 +0000 Message-ID: <59AF69C657FD0841A61C55336867B5B034421C3A@IRSMSX103.ger.corp.intel.com> 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> In-Reply-To: <2601191342CEEE43887BDE71AB977258213955A6@IRSMSX105.ger.corp.intel.com> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] 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: Wed, 22 Oct 2014 15:02:24 -0000 > -----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 >=20 >=20 >=20 > > -----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 t= x > 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 a= nd 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 =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", idx)= ; > > > > > > > + > > > > > > > + 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_b= urst 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 t= he > 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 perform= ance. > > > So I insist it's a library internal error checking, doesn't need to t= ake 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 wr= itten > > 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 su= pposed > to > > be in use when you are debugging applications, to create silent account= ing > > 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 deb= ug code > > is the most short sighted thing I've read in a long time. >=20 > 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 er= ror code. > Also reading though 'formal' comments of rte_eth_rx_burst() - it seems th= at 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." >=20 > 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: >=20 > 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; > } >=20 > 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 initiall= y > planned). >=20 > Konstantin Agreed. For any errors other than no-packets available yet, we can also con= sider setting rte_errno when returning 0.=20 /Bruce