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

[Wiles, Keith]

> On Mar 23, 2017, at 9:23 PM, Hu, Jiayu <jiayu.hu at intel.com> wrote:
> 
> On Thu, Mar 23, 2017 at 09:12:56PM +0800, Ananyev, Konstantin wrote:
>> Hi everyone,
>> 
>>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: Hu, Jiayu
>>>>> Sent: Thursday, March 23, 2017 6:25 AM
>>>>> To: Wiles, Keith <keith.wiles at intel.com>
>>>>> Cc: Yuanhan Liu <yuanhan.liu at linux.intel.com>; Richardson, Bruce
>>>>> <bruce.richardson at intel.com>; Yigit, Ferruh <ferruh.yigit at intel.com>;
>>>>> Ananyev, Konstantin <konstantin.ananyev at intel.com>
>>>>> Subject: Re: [dpdk-dev] [PATCH 0/2] lib: add TCP IPv4 GRO support
>>>>> 
>>>>> On Thu, Mar 23, 2017 at 01:29:06PM +0800, Wiles, Keith wrote:
>>>>>> 
>>>>>>> On Mar 22, 2017, at 9:15 PM, Hu, Jiayu <jiayu.hu at intel.com> wrote:
>>>>>>> 
>>>>>>> On Wed, Mar 22, 2017 at 10:19:41PM +0800, Wiles, Keith wrote:
>>>>>>>> Off list.
>>>>>>>> 
>>>>>>>> This support for GRO, seems it needs to be a feature for all ethernet
>>>>> devices and some way the developer can enable this feature like the other
>>>>> offloads in DPDK. The GRO support should be set by the developer and then
>>>>> the apis are called within ethdev or the PMD to process the packets. The
>>>>> code is generic and creating a library is not the best overall solution
>>>>> IMO.
>>>>>>> 
>>>>>>> Indeed, in this patchset, GRO is just proposed as a application-used
>>>>> library.
>>>>>>> But actually, these GRO APIs can also be used by ethdev and PMD. For
>>>>>>> example, register rte_gro_tcp4_reassemble_burst as a rx callback.
>>>>>>> Therefore, maybe we can support GRO in two ways. The one is a
>>>>>>> application-used library, the other is a device feature. Applications
>>>>> decide which one to use.
>>>>>>> 
>>>>>>> How do you think of it?
>>>>>> 
>>>>>> I would prefer to use it only in a offload design, meaning the GRO is
>>>>> just another ethernet offload the user can turn on. Using something like a
>>>>> RX callback to handle the GRO for the developer. This way he just turns it
>>>>> on in via a ethdev offload support feature and then setup the RX callback
>>>>> via ethdev. The developer only needs to enable the feature and never calls
>>>>> GRO APIs.
>>>>> 
>>>>> The advantage of providing an application-used GRO library can enable more
>>>>> flexibility to applications. Applications can flexibly use GRO functions
>>>>> according to own realistic scenario. Therefore, I think it makes sense to
>>>>> provide an application-used GRO library.
>>>>>> 
>>>>>> Adding a new GRO library may not get much support and having a whole
>>>>> library for GRO seems a bit odd.
>>>>> 
>>>>> In my opinion, we just need to provide one GRO library. But it can be used
>>>>> by ethernet devices and applications at the same time. Ethernet devices
>>>>> use it to provide an offload feature. If applications want more
>>>>> flexibility, they can just turn off this device feature, and use GRO APIs
>>>>> directly.
>>>>> 
>>>>> +Konstantin
>>>>> 
>>>> 
>>>> [Apologies for the basic questions, I haven't studied the patchset in detail]
>>>> Rather than adding a whole new library for it, can it just fit into librte_net or an existing lib? Are we planning a sample to show off
>>> tighter integration with ethdev or changes to the ethdev library to transparently use the library when needed?
>>> 
>>> Currently, we have an individual library, librte_ip_frag, which provides IP fragment
>>> and ressembly abilities. Similiarly, DPDK GRO will provide reassembly ability for
>>> various of protocols, not only TCP. So I think it's good to make a new library for
>>> this feature.
>>> 
>>> About GRO, we had a discussion two monthes ago. You can see it in
>>> http://dpdk.org/ml/archives/dev/2017-January/056276.html
>>> In that discussion, we agree to support GRO in two steps. The first is to implement
>>> GRO as a standalone library, and see how much performance we can get. The second
>>> step is to discuss how to integrate GRO into DPDK. Therefore, if we agree to support
>>> GRO as a device feature, we need to discuss how to enable/disable this device feature.
>>> Once we reach an agreement, there will be a sample to demonstrate the integration.
>> 
>> 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.

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.

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