From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 65E49695D for ; Fri, 24 Mar 2017 12:43:44 +0100 (CET) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga105.fm.intel.com with ESMTP; 24 Mar 2017 04:43:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,214,1486454400"; d="scan'208";a="78945806" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by orsmga005.jf.intel.com with ESMTP; 24 Mar 2017 04:43:41 -0700 Received: from irsmsx109.ger.corp.intel.com ([169.254.13.175]) by IRSMSX102.ger.corp.intel.com ([169.254.2.7]) with mapi id 14.03.0248.002; Fri, 24 Mar 2017 11:43:41 +0000 From: "Ananyev, Konstantin" To: "Hu, Jiayu" , Yuanhan Liu CC: "Wiles, Keith" , "Richardson, Bruce" , Stephen Hemminger , "Yigit, Ferruh" , "dev@dpdk.org" , "Liang, Cunming" , Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH 0/2] lib: add TCP IPv4 GRO support Thread-Index: AQHSou9jpQ4UJraET0mXNmgGJzKXbKGh9nLjgAAqOwCAABfzgIAAKzIggADheACAAEHWAIAAEcwAgAAMT4CAADY0gA== Date: Fri, 24 Mar 2017 11:43:40 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772583FAD80A6@IRSMSX109.ger.corp.intel.com> References: <1B893F1B-4DA8-4F88-9583-8C0BAA570832@intel.com> <20170323021502.GA114662@localhost.localdomain> <20170323062433.GA120139@localhost.localdomain> <59AF69C657FD0841A61C55336867B5B066729E3F@IRSMSX103.ger.corp.intel.com> <20170323102135.GA124301@localhost.localdomain> <2601191342CEEE43887BDE71AB9772583FAD410A@IRSMSX109.ger.corp.intel.com> <20170324022310.GA129105@localhost.localdomain> <32FED22E-7116-4B6A-894E-C81CBF7B4BBE@intel.com> <20170324072230.GU18844@yliu-dev.sh.intel.com> <20170324080633.GA2865@localhost.localdomain> In-Reply-To: <20170324080633.GA2865@localhost.localdomain> 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/2] lib: add TCP IPv4 GRO support 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, 24 Mar 2017 11:43:45 -0000 > -----Original Message----- > From: Hu, Jiayu > Sent: Friday, March 24, 2017 8:07 AM > To: Yuanhan Liu > Cc: Wiles, Keith ; Ananyev, Konstantin ; Richardson, Bruce > ; Stephen Hemminger ; Yigit, Ferruh ; > dev@dpdk.org; Liang, Cunming ; Thomas Monjalon <= thomas.monjalon@6wind.com> > Subject: Re: [dpdk-dev] [PATCH 0/2] lib: add TCP IPv4 GRO support >=20 > On Fri, Mar 24, 2017 at 03:22:30PM +0800, Yuanhan Liu wrote: > > On Fri, Mar 24, 2017 at 06:18:48AM +0000, Wiles, Keith wrote: > > > >> I think that having a separate library for GRO is a step in a righ= t direction. > > > >>> From my perspective - it provides a clean and flexible way to use= that feature. > > > >> If later someone would like to put GRO into ethdev layer (or parti= cular PMD), > > > >> he can use existing librte_gro for that. > > > > > > > > Agree. I think introducing more flexibility is an important thing f= or applications. > > > > > > Creating a new library just for GRO is not a reasonable solution, but= adding that support to an existing library like librte_net would > be cleaner and not create yet another library. > > > > Librte_net seems like a good suggestion to me, especially when we are > > considering to add GSO in future. The only concern to me is "net" may > > be too generic. It maybe kind of hard to decide which should be in > > librte_net, and which should be added as a standalone lib. For example, > > shouldn't 'lpm' and 'ip_frag' also belong to librte_net? About librte_gro vs librte_net: Right now librte_net is quite lightweight one - it mostly contains a net pr= otocols definitions plus some extra helper functions: to parse the l2/l3 headers to determine p= type, to calculate cksum, etc. GRO code is quite different - it has to allocate and manage hash table(s), = etc. Again my understanding it would keep growing (with new proto support). Again as mentioned above if GRO should go into librte_net, then librte_ipfr= ag and future GSO should also be here. Which would create quite a monstrous library. =20 So I think it is better to keep librte_net small and tidy and put GRO funct= ionality into the new library. > > > > > Creating more flexibility is not the best goal as we really want to m= ake GRO easy and simple for the developer to use for any > device without having to change his applications to take advantage of the= feature. Some times providing more flexibility just means > making it more complexed and more APIs the developer needs to understand.= Providing GRO as a offload feature is the better > direction as it makes it simple for an application to use. > > > > > > If we provide GRO as a standard offload similar to the other offloads= we currently have makes it easy for the developer. The best > goal for a feature is the best performance for the application without ha= ving the application make even more APIs calls along with > simple and easy to use. > > > > In general, I'd agree with you, if no one is object to add a short piec= e > > of code at the end of rte_eth_rx_burst: > > > > + if (eth_gro_is_enabled(dev)) > > + nb_rx =3D rte_net_gro(...); > > + > > return nb_rx; > > } > > > > Objections? I'd better not to open that door. If we'll allow that for GRO - we'll have to allow that for every other stuf= f: - ip reassembly - l3/l4 cksum calculation if underlying HW doesn't support it - SW ptype recognition - etc. Adding all these things into rx_burst() would most likely slow things down (remember it is a data-path) and pretty soon would bring rx_burst() into messy monster that would be hard to maintain and debug. =20 My preference would be to keep rte_ethdev data-path as small and tidy as po= ssible. If in future we'd really like to introduce all these SW things into dev lay= er - my preference would be to create some sort of new abstraction on top of cur= rent ethdev: rte_eth_smartdev or so. So it would be: rte_eth_smartdev_rx_burst(....) { nb_rx =3D rte_eth_rx_burst(...); /* apply GRO, reassembly, etc. */ ... }=20 Something similar with what 6Wind trying to introduce with their failsafe d= ev concept. > > > > But one way or another, we need put the gro code at somewhere and we > > need introduce a generic API for that. It could be librte_net as you > > suggested. So the good thing is that we all at least come an agreement > > that it should be implemented in lib, right? The only controversy is > > should we export it to application and let them to invoke it, or hide > > it inside rte_eth_rx_burst. > > > > Though it may take some time for all of us to come an agreement on that= , > > but the good thing is that it would be a very trivial change once it's > > done. Agree? >=20 > Agree. >=20 > > > > Thus I'd suggest Jiayu to focus on the the GRO code developement, such > > as making it generic enough and adding other protocols support. And I > > would like to ask you guys to help review them. Makes sense to all? > > >=20 > Agree again. No matter where to put GRO code, the apis should be generic > and extensible. And more protocols should be supported. Yep, that's what my take from the beginning: Let's develop a librte_gro first and make it successful, then we can think = should we (and how) put into ethdev layer. Konstantin >=20 > Thanks, > Jiayu >=20 > > Thanks. > > > > --yliu > > > > > > > >> I didn't have a closer look yet, but I think that caught my atten= tion: > > > >> API fir the lib seems too IPv4/TCP oriented - > > > >> though I understand that the most common case and should be implem= ented first. > > > >> I wonder can we have it a bit more generic and extendable, so user= can specify what combination of protocols > > > >> he is interested in (let say: ipv4/tcp, ipv6/tcp, etc.). > > > >> Even if right now we'll have it implemented only for ipv4/tcp. > > > >> Then internally we can have some check is that supported or not an= d if yes setup things accordingly. > > > > > > > > Indeed, current apis are too specific. It's not very friendly to ap= plications. > > > > Maybe we can use macro to define the combination of protocols, like= GRO_TCP_IPV4 > > > > and GRO_UDP_IPV6; and provide a generic setup function and reassemb= ly function. > > > > Both of them perform different operations according to the macro va= lue inputted > > > > by the application. > > > > > > > >> BTW, that's for 17.08, right? > > > > > > > > Yes, it's for 17.08. > > > > > > > > Jiayu > > > >> > > > >> Konstantin > > > >> > > > >> > > > > > > Regards, > > > Keith