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 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?
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? > Agree again. No matter where to put GRO code, the apis should be generic and extensible. And more protocols should be supported. Thanks, Jiayu > 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
