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

Wiles, Keith keith.wiles at intel.com
Fri Mar 24 15:37:04 CET 2017


> On Mar 24, 2017, at 6:43 AM, Ananyev, Konstantin <konstantin.ananyev at intel.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Hu, Jiayu
>> Sent: Friday, March 24, 2017 8:07 AM
>> To: Yuanhan Liu <yuanhan.liu at linux.intel.com>
>> Cc: Wiles, Keith <keith.wiles at intel.com>; Ananyev, Konstantin <konstantin.ananyev at intel.com>; Richardson, Bruce
>> <bruce.richardson at intel.com>; Stephen Hemminger <stephen at networkplumber.org>; Yigit, Ferruh <ferruh.yigit at intel.com>;
>> dev at dpdk.org; Liang, Cunming <cunming.liang at intel.com>; Thomas Monjalon <thomas.monjalon at 6wind.com>
>> Subject: Re: [dpdk-dev] [PATCH 0/2] lib: add TCP IPv4 GRO support
>> 
>> 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?
> 
> About librte_gro vs librte_net:
> 
> Right now librte_net is quite lightweight one - it mostly contains a net protocols definitions
> plus some extra helper functions: to parse the l2/l3 headers to determine ptype, 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_ipfrag and future GSO should also be here.
> Which would create quite a monstrous library.  
> So I think it is better to keep librte_net small and tidy and put GRO functionality into the new library.

The size of a library is not a concern we have some pretty big ones already.

-rw-rw-r-- 2 rkwiles rkwiles 1.2M Mar 21 15:22 librte_acl.a
-rw-rw-r-- 1 rkwiles rkwiles  39K Mar 21 15:22 librte_cfgfile.a
-rw-rw-r-- 1 rkwiles rkwiles 292K Mar 21 15:22 librte_cmdline.a
-rw-rw-r-- 1 rkwiles rkwiles 211K Mar 21 15:23 librte_cryptodev.a
-rw-rw-r-- 1 rkwiles rkwiles  75K Mar 21 15:22 librte_distributor.a
-rw-rw-r-- 1 rkwiles rkwiles 1.4M Mar 21 15:22 librte_eal.a
-rw-rw-r-- 1 rkwiles rkwiles 323K Mar 21 15:23 librte_efd.a
-rw-rw-r-- 1 rkwiles rkwiles 374K Mar 22 09:31 librte_ethdev.a
-rw-rw-r-- 1 rkwiles rkwiles 675K Mar 21 15:22 librte_hash.a
-rw-rw-r-- 1 rkwiles rkwiles 366K Mar 21 15:23 librte_ip_frag.a
-rw-rw-r-- 1 rkwiles rkwiles  29K Mar 21 15:22 librte_jobstats.a
-rw-rw-r-- 1 rkwiles rkwiles 167K Mar 21 15:23 librte_kni.a
-rw-rw-r-- 1 rkwiles rkwiles  19K Mar 21 15:22 librte_kvargs.a
-rw-rw-r-- 1 rkwiles rkwiles 309K Mar 21 15:22 librte_lpm.a
-rw-rw-r-- 1 rkwiles rkwiles  93K Mar 21 15:22 librte_mbuf.a
-rw-rw-r-- 1 rkwiles rkwiles 270K Mar 21 15:22 librte_mempool.a
-rw-rw-r-- 1 rkwiles rkwiles  17K Mar 21 15:22 librte_meter.a
-rw-rw-r-- 1 rkwiles rkwiles  71K Mar 21 15:22 librte_net.a
-rw-rw-r-- 1 rkwiles rkwiles 211K Mar 21 15:23 librte_pdump.a
-rw-rw-r-- 1 rkwiles rkwiles 140K Mar 21 15:23 librte_pipeline.a
-rw-rw-r-- 1 rkwiles rkwiles 1.4M Mar 21 15:23 librte_port.a
-rw-rw-r-- 1 rkwiles rkwiles 122K Mar 21 15:22 librte_power.a
-rw-rw-r-- 1 rkwiles rkwiles 154K Mar 21 15:22 librte_reorder.a
-rw-rw-r-- 1 rkwiles rkwiles  63K Mar 21 15:22 librte_ring.a
-rw-rw-r-- 1 rkwiles rkwiles 377K Mar 21 15:23 librte_sched.a
-rw-rw-r-- 1 rkwiles rkwiles 1.4M Mar 21 15:23 librte_table.a
-rw-rw-r-- 1 rkwiles rkwiles  53K Mar 21 15:22 librte_timer.a
-rw-rw-r-- 1 rkwiles rkwiles 609K Mar 21 15:23 librte_vhost.a

Removed the PMD archives and ls is not a great way to get the true size compared to using ‘size’.

If you look at the size values, the ‘size *.a’ is interesting too.

The size of librte_net is 71K + ip_frag 366K is pretty small compared to a few others. I would assume GRO is pretty small too, so adding GRO into librte_net is very reasonable. We could leave ip_frag out as currently it is a standalone lib, but continue to add GSO to librte_net. I would not assume the size would that large and it seems like the best place to put the code.

If you still want to create a gso lib then I guess you can, just seems unreasonable to me.

> 
>>> 
>>>> 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?

Why do we need to modify every driver, why not use the RX callback feature then no drivers were harmed in this patch :-)

> 
> I'd better not to open that door.
> If we'll allow that for GRO - we'll have to allow that for every other stuff:
> - 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.    
> 
> My preference would be to keep rte_ethdev data-path as small and tidy as possible.
> If in future we'd really like to introduce all these SW things into dev layer -
> my preference would be to create some sort of new abstraction on top of current ethdev:
> rte_eth_smartdev or so.
> So it would be:
> rte_eth_smartdev_rx_burst(....)
> {
>   nb_rx =  rte_eth_rx_burst(...);
>   /* apply GRO, reassembly, etc. */
>  ...
> } 

Adding a new API is still not required, as the RX callback code is already in place for these types of post processing of mbufs.

> 
> Something similar with what 6Wind trying to introduce with their failsafe dev concept.

The 6Wind is a different story here and not a post processing of RX mbufs.

> 
>>> 
>>> 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.
> 
> 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.

Let not create a gro library and put the code into librte_net as size is not a concern yet and it is the best place to put the code. As for ip_frag someone can move it into librte_net if someone writes the patch.

> 
> Konstantin
> 
>> 
>> 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

Regards,
Keith



More information about the dev mailing list