[dpdk-dev] [PATCH 0/2] lib: add TCP IPv4 GRO support

Jiayu Hu jiayu.hu at intel.com
Fri Mar 24 09:06:33 CET 2017


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


More information about the dev mailing list