From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 095F4695D for ; Fri, 24 Mar 2017 08:24:40 +0100 (CET) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP; 24 Mar 2017 00:24:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,213,1486454400"; d="scan'208";a="80132307" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga005.fm.intel.com with ESMTP; 24 Mar 2017 00:24:32 -0700 Date: Fri, 24 Mar 2017 15:22:30 +0800 From: Yuanhan Liu To: "Wiles, Keith" Cc: "Hu, Jiayu" , "Ananyev, Konstantin" , "Richardson, Bruce" , Stephen Hemminger , "Yigit, Ferruh" , "dev@dpdk.org" , "Liang, Cunming" , Thomas Monjalon Message-ID: <20170324072230.GU18844@yliu-dev.sh.intel.com> References: <1490175137-108413-1-git-send-email-jiayu.hu@intel.com> <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <32FED22E-7116-4B6A-894E-C81CBF7B4BBE@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) 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 07:24:41 -0000 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 right 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 particular PMD), > >> he can use existing librte_gro for that. > > > > Agree. I think introducing more flexibility is an important thing for 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? > Creating more flexibility is not the best goal as we really want to make 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 having 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 piece of code at the end of rte_eth_rx_burst: + if (eth_gro_is_enabled(dev)) + nb_rx = rte_net_gro(...); + return nb_rx; } Objections? 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? 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? Thanks. --yliu > >> I didn't have a closer look yet, but I think that caught my attention: > >> API fir the lib seems too IPv4/TCP oriented - > >> though I understand that the most common case and should be implemented 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 and if yes setup things accordingly. > > > > Indeed, current apis are too specific. It's not very friendly to applications. > > 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 reassembly function. > > Both of them perform different operations according to the macro value inputted > > by the application. > > > >> BTW, that's for 17.08, right? > > > > Yes, it's for 17.08. > > > > Jiayu > >> > >> Konstantin > >> > >> > > Regards, > Keith