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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.

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