From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id BDCC92B93 for ; Fri, 31 Mar 2017 11:58:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490954292; x=1522490292; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=+ou14nJwsbIhu7wwzPUKCBRtAK60/iBxcelkKPMf9vI=; b=J2FB3/FuziCP4SvutAiSssHEi3TfaLhVUBbrfUOtqn0RwIZAz1JXCbC3 uXHBgShTbFdwCcUSAXs1NU6IP/ZPWQ==; Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Mar 2017 02:58:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,251,1486454400"; d="scan'208";a="72182420" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by orsmga004.jf.intel.com with ESMTP; 31 Mar 2017 02:58:09 -0700 Received: from irsmsx109.ger.corp.intel.com ([169.254.13.12]) by IRSMSX152.ger.corp.intel.com ([169.254.6.231]) with mapi id 14.03.0319.002; Fri, 31 Mar 2017 10:58:09 +0100 From: "Ananyev, Konstantin" To: Olivier Matz CC: "Richardson, Bruce" , "dev@dpdk.org" , "mb@smartsharesystems.com" , "Chilikin, Andrey" , "jblunck@infradead.org" , "nelio.laranjeiro@6wind.com" , "arybchenko@solarflare.com" Thread-Topic: [dpdk-dev] [PATCH 0/9] mbuf: structure reorganization Thread-Index: AQHSl/BM96NM/aKG30C3i/t54/q6k6GsCWqAgABGqICAAOACAIAAKlIAgAAFugCAAFkDYIAAAV1QgAAS2dCAAOcWgIAAI4tQ Date: Fri, 31 Mar 2017 09:58:08 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772583FAE3171@IRSMSX109.ger.corp.intel.com> References: <1485271173-13408-1-git-send-email-olivier.matz@6wind.com> <1488966121-22853-1-git-send-email-olivier.matz@6wind.com> <20170329175629.68810924@platinum> <20170329200923.GA11516@bricha3-MOBL3.ger.corp.intel.com> <20170330093108.GA10652@bricha3-MOBL3.ger.corp.intel.com> <20170330140236.0d2ebac8@platinum> <20170330122305.GA14272@bricha3-MOBL3.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772583FAE2A51@IRSMSX109.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772583FAE2A6E@IRSMSX109.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772583FAE2B2F@IRSMSX109.ger.corp.intel.com> <20170331104107.5b29cc0e@platinum> In-Reply-To: <20170331104107.5b29cc0e@platinum> Accept-Language: en-IE, 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 Subject: Re: [dpdk-dev] [PATCH 0/9] mbuf: structure reorganization X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 31 Mar 2017 09:58:13 -0000 Hi Olivier, >=20 > On Thu, 30 Mar 2017 18:06:35 +0000, "Ananyev, Konstantin" wrote: > > > -----Original Message----- > > > From: Ananyev, Konstantin > > > Sent: Thursday, March 30, 2017 5:48 PM > > > To: Ananyev, Konstantin ; Richardson, B= ruce ; Olivier Matz > > > > > > Cc: dev@dpdk.org; mb@smartsharesystems.com; Chilikin, Andrey ; jblunck@infradead.org; > > > nelio.laranjeiro@6wind.com; arybchenko@solarflare.com > > > Subject: RE: [dpdk-dev] [PATCH 0/9] mbuf: structure reorganization > > > > > > > > > > > > > -----Original Message----- > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konst= antin > > > > Sent: Thursday, March 30, 2017 5:45 PM > > > > To: Richardson, Bruce ; Olivier Matz > > > > Cc: dev@dpdk.org; mb@smartsharesystems.com; Chilikin, Andrey ; jblunck@infradead.org; > > > > nelio.laranjeiro@6wind.com; arybchenko@solarflare.com > > > > Subject: Re: [dpdk-dev] [PATCH 0/9] mbuf: structure reorganization > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: Richardson, Bruce > > > > > Sent: Thursday, March 30, 2017 1:23 PM > > > > > To: Olivier Matz > > > > > Cc: dev@dpdk.org; Ananyev, Konstantin ; mb@smartsharesystems.com; Chilikin, Andrey > > > > > ; jblunck@infradead.org; nelio.laranje= iro@6wind.com; arybchenko@solarflare.com > > > > > Subject: Re: [dpdk-dev] [PATCH 0/9] mbuf: structure reorganizatio= n > > > > > > > > > > On Thu, Mar 30, 2017 at 02:02:36PM +0200, Olivier Matz wrote: > > > > > > On Thu, 30 Mar 2017 10:31:08 +0100, Bruce Richardson wrote: > > > > > > > On Wed, Mar 29, 2017 at 09:09:23PM +0100, Bruce Richardson wr= ote: > > > > > > > > On Wed, Mar 29, 2017 at 05:56:29PM +0200, Olivier Matz wrot= e: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > Does anyone have any other comment on this series? > > > > > > > > > Can it be applied? > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Olivier > > > > > > > > > > > > > > > > > > > > > > > > > I assume all driver maintainers have done performance analy= sis to check > > > > > > > > for regressions. Perhaps they can confirm this is the case. > > > > > > > > > > > > > > > > /Bruce > > > > > > > > > > > > > > > > In the absence, of anyone else reporting performance numbers = with this > > > > > > > patchset, I ran a single-thread testpmd test using 2 x 40G po= rts (i40e) > > > > > > > driver. With RX & TX descriptor ring sizes of 512 or above, I= 'm seeing a > > > > > > > fairly noticable performance drop. I still need to dig in mor= e, e.g. do > > > > > > > an RFC2544 zero-loss test, and also bisect the patchset to se= e what > > > > > > > parts may be causing the problem. > > > > > > > > > > > > > > Has anyone else tried any other drivers or systems to see wha= t the perf > > > > > > > impact of this set may be? > > > > > > > > > > > > I did, of course. I didn't see any noticeable performance drop = on > > > > > > ixgbe (4 NICs, one port per NIC, 1 core). I can replay the test= with > > > > > > current version. > > > > > > > > > > > I had no doubt you did some perf testing! :-) > > > > > > > > > > Perhaps the regression I see is limited to i40e driver. I've conf= irmed I > > > > > still see it with that driver in zero-loss tests, so next step is= to try > > > > > and localise what change in the patchset is causing it. > > > > > > > > > > Ideally, though, I think we should see acks or other comments fro= m > > > > > driver maintainers at least confirming that they have tested. You= cannot > > > > > be held responsible for testing every DPDK driver before you subm= it work > > > > > like this. > > > > > > > > > > > > > Unfortunately I also see a regression. > > > > Did a quick flood test on 2.8 GHZ IVB with 4x10Gb. > > > > > > Sorry, forgot to mention - it is on ixgbe. > > > So it doesn't look like i40e specific. > > > > > > > Observed a drop even with default testpmd RXD/TXD numbers (128/512)= : > > > > from 50.8 Mpps down to 47.8 Mpps. > > > > From what I am seeing the particular patch that causing it: > > > > [dpdk-dev,3/9] mbuf: set mbuf fields while in pool > > > > > > > > cc version 5.3.1 20160406 (Red Hat 5.3.1-6) (GCC) > > > > cmdline: > > > > ./dpdk.org-1705-mbuf1/x86_64-native-linuxapp-gcc/app/testpmd --lco= res=3D'7,8' -n 4 --socket-mem=3D'1024,0' -w 04:00.1 -w 07:00.1 - > w > > > > 0b:00.1 -w 0e:00.1 -- -i > > > > > > > > Actually one more question regarding: > > [dpdk-dev,9/9] mbuf: reorder VLAN tci and buffer len fields > > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h > > index fd97bd3..ada98d5 100644 > > --- a/lib/librte_mbuf/rte_mbuf.h > > +++ b/lib/librte_mbuf/rte_mbuf.h > > @@ -449,8 +449,7 @@ struct rte_mbuf { > > > > uint32_t pkt_len; /**< Total pkt len: sum of all segments. */ > > uint16_t data_len; /**< Amount of data in segment buffer. */ > > - /** VLAN TCI (CPU order), valid if PKT_RX_VLAN_STRIPPED is set. */ > > - uint16_t vlan_tci; > > + uint16_t buf_len; /**< Size of segment buffer. */ > > > > union { > > uint32_t rss; /**< RSS hash result if RSS enabled */ > > @@ -475,11 +474,11 @@ struct rte_mbuf { > > uint32_t usr; /**< User defined tags. See rte_distributor_process(= ) */ > > } hash; /**< hash information */ > > > > + /** VLAN TCI (CPU order), valid if PKT_RX_VLAN_STRIPPED is set. */ > > + uint16_t vlan_tci; > > /** Outer VLAN TCI (CPU order), valid if PKT_RX_QINQ_STRIPPED is set.= */ > > uint16_t vlan_tci_outer; > > > > - uint16_t buf_len; /**< Length of segment buffer. */ > > - > > /** Valid if PKT_RX_TIMESTAMP is set. The unit and time reference > > * are not normalized but are always the same for a given port. > > */ > > > > How ixgbe and i40e SSE version supposed to work correctly after that c= hange? > > As I remember both of them sets vlan_tci as part of 16B shuffle operati= on. > > Something like that: > > pkt_mb4 =3D _mm_shuffle_epi8(descs[3], shuf_msk); > > ... > > mm_storeu_si128((void *)&rx_pkts[pos+3]->rx_descriptor_fields1, > > pkt_mb4); > > > > But now vlan_tci is swapped with buf_len. > > Which means 2 things to me: > > It is more than 16B away from rx_descriptor_fields1 and can't be update= d in one go anymore, > > and instead of vlan_tci we are updating buf_len. >=20 >=20 > Sorry, I missed it. But this shows something problematic: changing the > order of fields in a structure breaks code without notification. I think > that drivers expecting a field at a specific position should have some > BUG_ON() to check that the condition is still valid. We can't expect anyo= ne > to know all the constraints of all vectors PMDs in DPDK. >=20 > The original idea of this patch was to group vlan_tci and vlan_outer_tci, > which looked to be a good idea at first glance. If it requires to change > all vector code, let's drop it. >=20 > Just for the exercice, let's imagine we need that patch. What would be > the procedure to have it integrated? How can we detect there is an issue? > Who would be in charge of modifying all the vector code in PMDs? >=20 Indeed right now there is no way to know what is PMD requirement on mbuf la= yout. Adding BUG_ON() into particular RX/TX implementation that has such constrai= ns seems like a very good idea to me. Apart from that I don't know off-hand how we can make restructuring mbuf le= ss painful. Konstantin =20