From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id ED5F82952 for ; Tue, 12 Apr 2016 17:40:20 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 12 Apr 2016 08:40:19 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,474,1455004800"; d="scan'208";a="957073242" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by fmsmga002.fm.intel.com with ESMTP; 12 Apr 2016 08:40:18 -0700 Received: from irsmsx111.ger.corp.intel.com (10.108.20.4) by IRSMSX102.ger.corp.intel.com (163.33.3.155) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 12 Apr 2016 16:40:18 +0100 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.238]) by irsmsx111.ger.corp.intel.com ([169.254.2.16]) with mapi id 14.03.0248.002; Tue, 12 Apr 2016 16:40:17 +0100 From: "Dumitrescu, Cristian" To: "Sanford, Robert" , "dev@dpdk.org" CC: "Liang, Cunming" , "Venkatesan, Venky" , "Richardson, Bruce" Thread-Topic: [dpdk-dev] [PATCH 4/4] port: fix ethdev writer burst too big Thread-Index: AQHRjDKoyc2GDY2FaUGj9fRrdME9ip+FL/cQgABNsgCAAQjg4A== Date: Tue, 12 Apr 2016 15:40:17 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D89126479A0F17@IRSMSX108.ger.corp.intel.com> References: <3EB4FA525960D640B5BDFFD6A3D89126479A0A65@IRSMSX108.ger.corp.intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZmU0MjM1MmMtYjA4MC00YTg3LWI5NDAtYTMxMTgwZDg4M2FkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InRcL29BSTFTNHNrSytMQ2U1U2lRdnhUc3BSbjVjRndQK0lZYzlaQUljK3hNPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 4/4] port: fix ethdev writer burst too big 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: Tue, 12 Apr 2016 15:40:21 -0000 > -----Original Message----- > From: Sanford, Robert [mailto:rsanford@akamai.com] > Sent: Monday, April 11, 2016 9:37 PM > To: Dumitrescu, Cristian ; dev@dpdk.org > Cc: Liang, Cunming ; Venkatesan, Venky > ; Richardson, Bruce > > Subject: Re: [dpdk-dev] [PATCH 4/4] port: fix ethdev writer burst too big >=20 > Hi Cristian, >=20 > Yes, I mostly agree with your suggestions: > 1. We should fix the two obvious bugs (1a and 1b) right away. Jasvinder's > patches look fine. > 2. We should take no immediate action on the issue I raised about PMDs > (vector IXGBE) not enqueuing more than 32 packets. We can discuss and > debate; no patch for 16.04, perhaps something in 16.07. >=20 >=20 > Let's start the discussion now, regarding vector IXGBE. You state > "Internally it handles packets in batches [of] 32 (as your code snippets > suggest), but there is no drop of excess packets taking place." I guess i= t > depends on your definition of "drop". If I pass 33 packets to > ixgbe_xmit_pkts_vec(), it will enqueue 32 packets, and return a value of > 32. Can we agree on that? >=20 Yes, Steve Liang and I looked at the latest IXGBE vector code and it looks = like you are right. The number of packets that get accepted is the minimum = between number of packets provided by the user (33 in our case) and two thr= esholds, txq->tx_rs_thresh and txq->nb_tx_free, which are by default set to= 32, which is the value that yields the best performance, hence only 32 pac= kets get accepted. It also looks virtually impossible to change this behaviour of IXGBE vector= driver. As an example, let's say 33 packets are presented by the user, IXG= BE picks the first 32 and tries to send them, but only 17 make it, so the o= ther 15 have to be returned back to the user; then there is the 33rd packet= that is picked, and this packet makes it. Since return value is a number (= not a mask), how do you tell the user that packets 0 .. 16 and 32 made it, = while the packets 17 .. 31 in the middle of the burst did not make it? So the only real place to improve this is the port_ethdev_writer. I wonder = whether there is nice way to combine existing behavior (burst treated as mi= nimal requirement) with your proposal (burst treated as constant requiremen= t) under the same code, and then pick between the two behaviors based on an= input parameter provided when port is created? > -- > Regards, > Robert >=20 >=20 > On 4/11/16 3:21 PM, "Dumitrescu, Cristian" > wrote: >=20 > >Hi Robert, > > > >I am doing a quick summary below on the changes proposed by these > patches: > > > >1. [PRIORITY 1] Bug fixing: > >a) Fix buffer overflow issue in rte_port_ring.c (writer, writer_nodrop): > >double the tx_buf buffer size (applicable for current code approach) > >b) Fix issue with handling burst sizes bigger than 32: replace all > >declarations of local variable bsz_size from uint32_t to uint64_t > > > >2. [PRIORITY 2] Treat burst size as a fixed/exact value for the TX burst > >(Approach 2) instead of minimal value (current code, Approach 1) for > >ethdev ports. Rationale is that some PMDs (like vector IXGBE) _might_ > >drop the excess packets in the burst. > > > >Additional input: > >1. Bruce and I looked together at the code, it looks that vector IXGBE i= s > >not doing this (anymore). Internally it handles packets in batches on 32 > >(as your code snippets suggest), but there is no drop of excess packets > >taking place. > > > >2. Venky also suggested to keep a larger burst as a single burst > >(Approach 1) rather than break the larger burst into a fixed/constant > >size burst while buffering the excess packets until complete burst is me= t > >in the future. > > > >Given this input and also the timing of the release, we think the best > >option is: > >- urgently send a quick patch to handle the bug fixes now > >- keep the existing code (burst size used as minimal burst size > >requirement, not constant) as is, at least for now, and if you feel it i= s > >not the best choice, we can continue to debate it for 16.7 release. > >What do you think? > > > >Jasvinder just send the bug fixing patches, hopefully they will make it > >into the 16.4 release: > >http://www.dpdk.org/ml/archives/dev/2016-April/037392.html > >http://www.dpdk.org/ml/archives/dev/2016-April/037393.html > > > >Many thanks for your work on this, Robert! > > > >Regards, > >Cristian