Re: [dpdk-dev] [PATCH] examples: remove Rx checksum offload

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 7/30/2018 4:30 PM, Andrew Rybchenko wrote:
> On 30.07.2018 17:40, Jerin Jacob wrote:
>> -----Original Message-----
>>> Date: Mon, 30 Jul 2018 14:12:12 +0000
>>> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>> CC: Thomas Monjalon <thomas@monjalon.net>, "dev@dpdk.org" <dev@dpdk.org>,
>>>   "Yigit, Ferruh" <ferruh.yigit@intel.com>, "shahafs@mellanox.com"
>>>   <shahafs@mellanox.com>
>>> Subject: RE: [dpdk-dev] [PATCH] examples: remove Rx checksum offload
>>>
>>>
>>>> -----Original Message-----
>>>>> Date: Mon, 30 Jul 2018 11:00:02 +0000
>>>>> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
>>>>> To: Thomas Monjalon <thomas@monjalon.net>, Jerin Jacob
>>>>>   <jerin.jacob@caviumnetworks.com>
>>>>> CC: "dev@dpdk.org" <dev@dpdk.org>, "Yigit, Ferruh"
>>>>>   <ferruh.yigit@intel.com>, "shahafs@mellanox.com" <shahafs@mellanox.com>
>>>>> Subject: RE: [dpdk-dev] [PATCH] examples: remove Rx checksum offload
>>>>>
>>>>> External Email
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Thomas Monjalon [mailto:thomas@monjalon.net]
>>>>>> Sent: Monday, July 30, 2018 10:51 AM
>>>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>
>>>>>> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; shahafs@mellanox.com
>>>>>> Subject: Re: [dpdk-dev] [PATCH] examples: remove Rx checksum offload
>>>>>>
>>>>>> 30/07/2018 11:35, Jerin Jacob:
>>>>>>> From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
>>>>>>>>> As of now, application does not check PKT_RX_*_CKSUM_* flags per
>>>>>>>>> packet, so it does not matter DEV_RX_OFFLOAD_CHECKSUM enabled or not.
>>>>>>>>>
>>>>>>>>> Removing DEV_RX_OFFLOAD_CHECKSUM offload so that driver can save a few
>>>>>>>>> cycles if possible.
>>>>>>>> Personally, I'd move in other direction: keep RX checksum offload and add
>>>>>>>> checks inside sample apps to handle (drop) packets with invalid checksum.
>>>>>>> OK. Till someones add the DROP logic in application, Can we take
>>>>>>> this patch? Because there is no point in enabling DEV_RX_OFFLOAD_CHECKSUM
>>>>>>> without DROP or any meaning full action in application.
>>>>> Probably, but at least it gives users a right estimation how long the proper
>>>>> RX/TX routine would take.
>>>> For estimation, application can add any flag they want in local setup.
>>>> It does not need to be upstream with out feature complete.
>>>>
>>>>>  From other side what the point to disable these flags now, if we know that
>>>> At least nicvf Rx routines are crafted based DEV_RX_OFFLOAD_CHECKSUM
>>>> flags. If driver Rx routine crafted such case it will be useful.
>>>>
>>>>> we are doing wrong thing and will have to re-enable them again in future?
>>>> But it is not correct now either. Right?
>>> Yes, right now invalid cksum information is simply ignored.
>>> As you pointed - some PMD select RX routine based on checksum offload flags.
>>> Yes, removing these flags might produce better performance numbers.
>>> But from my perspective - it would be an artificial and temporary improvement,
>>> as for l3fwd like apps we'll need to revert it back and add code to drop invalid packets.
>> IMO, It is OK get a performance hit when do that support in l3fwd. There
>> is no harm in removing the DEV_RX_OFFLOAD_CHECKSUM flag for now and it
>> is correct from application perspective.(you are enabling an offload when
>> you are using it, else don't enable it. I believe, this was philosophy for
>> enabling Rx/Tx offloads)
>>
>> Since it is going in circles, I leave decision to ethdev maintainers.
> 
> I think that IPv4 checksum offload is essential for l3fwd. So, it should be
> enabled and taken into account. I'm not sure about TCP and UDP checksum
> offloads. It is not l3fwd business to take a look at upper layers.
> 
> In any case, there is no agreement on the patch and it is already RC3 stage
> of the release. There is no rush to apply it since it is not a critical 
> bug fix.
> I agree with Konstantin here.

Now it is in the scope of v18.11,

+1 to implement PKT_RX_*_CKSUM checks in applications instead of removing
DEV_RX_OFFLOAD_CHECKSUM

> 
> Andrew
> 
>>> Konstantin
>>>
>>>>>> If there is no patch sent to use this offload on August 1st,
>>>>>> then I will apply this patch to remove the offload request.
>>>>>>
>>>>> Isn't it too late to do such things right now?
>>>>> We are in RC3 stage and doesn't look like a critical issue.
>>>> Yes. We can add it when have we proper fix. Currently, it signaling a wrong
>>>> interpretation to application.
>>>>
>>>>
>>>>> Konstantin
>>>>>
>>>>>
>