Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition

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

On 10/8/2018 12:55 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Mon, 8 Oct 2018 11:53:01 +0100
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Thomas Monjalon
>>  <thomas@monjalon.net>
>> CC: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Andrew Rybchenko
>>  <arybchenko@solarflare.com>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com>, "Wu,
>>  Jingjing" <jingjing.wu@intel.com>, "Iremonger, Bernard"
>>  <bernard.iremonger@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>,
>>  "Kovacevic, Marko" <marko.kovacevic@intel.com>, Olivier Matz
>>  <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>>  <didier.pallard@6wind.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>  checksum definition
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.9.1
>>
>> On 10/8/2018 10:37 AM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Mon, 08 Oct 2018 11:04:51 +0200
>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Ferruh Yigit
>>>>  <ferruh.yigit@intel.com>, "Ananyev, Konstantin"
>>>>  <konstantin.ananyev@intel.com>
>>>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>, "Lu, Wenzhuo"
>>>>  <wenzhuo.lu@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
>>>>  "Iremonger, Bernard" <bernard.iremonger@intel.com>, "Mcnamara, John"
>>>>  <john.mcnamara@intel.com>, "Kovacevic, Marko" <marko.kovacevic@intel.com>,
>>>>  Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>,
>>>>  "shahafs@mellanox.com" <shahafs@mellanox.com>, "didier.pallard@6wind.com"
>>>>  <didier.pallard@6wind.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>  checksum definition
>>>>
>>>> 08/10/2018 10:24, Jerin Jacob:
>>>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>> On 10/6/2018 1:18 PM, Ananyev, Konstantin wrote:
>>>>>>> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
>>>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
>>>>>>>>> However, we should re-visit the flag PKT_RX_EIP_CKSUM_BAD.
>>>>>>>>
>>>>>>>> Do we need to block this patch due to the exiting PKT_RX_EIP_CKSUM_BAD
>>>>>>>> definition?
>>>>>>>>
>>>>>>>> I already added the author of the PKT_RX_EIP_CKSUM_BAD flag and ethdev and mbuf
>>>>>>>> maintainers in this list. So what else I need make forward progress
>>>>>>>> on this patch?
>>>>>>>>
>>>>>>>> I think, the definition of PKT_RX_EIP_CKSUM_BAD based on HW capability. It
>>>>>>>> is safe to assume that ALL HW can support CKSUM BAD if the feature is
>>>>>>>> available and hence it is more portable.
>>>>>>>
>>>>>>> Yes, as I remember PKT_RX_EIP_CKSUM_BAD is based on DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM.
>>>>>>
>>>>>> Switching to two bit won't reduce the portability, HW supports only reporting
>>>>>> CKSUM_BAD can set BAD || UNKNOWN.
>>>>>
>>>>> UNKNOWN is not a bit. It is represented as 0. It spec has 2 bit, then
>>>>> driver need to report GOOD as well.
>>>>>
>>>>> Same applies for PKT_RX_EL4_CKSUM as well.
>>>>>
>>>>>>
>>>>>> And I think patch is not blocked by PKT_RX_EIP_CKSUM_BAD, it can be changed
>>>>>> separately, for this patch question is can we represent PKT_RX_EL4_CKSUM_* with
>>>>>> two bits, to have BAD/GOOD/UNKNOWN?
>>>>
>>>> Yes, exact.
>>>>
>>>> PKT_RX_EIP_CKSUM_BAD must be left aside.
>>>> We should just avoid taking it as a reference.
>>>> And we can reconsider its definition later.
>>>
>>> OK.
>>>
>>> IMO, Using 2 bit scheme for tunneled checksum has following performance
>>> issue from driver side.
>>>
>>> Driver need to mark the packet as GOOD. All the HW can support
>>> detection of BAD. That not necessary mean GOOD in case of tunnel packet,
>>> so driver has to detect the packet is tunneled and packet is not BAD
>>> then mark GOOD.
>>
>> Yes UNKNOWN is not a bit, but a state, why don't use it? Why driver has to check
>> it is GOOD?
> 
> The application is going to check is it GOOD or not. Not the driver,
> Right? My concern was, If application starts dropping the packet instead checking the BAD, if
> it checks == !GOOD.

Got it, but when 2 bits state introduced, app should check if check == BAD for
drop decision, because it is not GOOD || BAD anymore.

> 
>>
>> 0x0 => UNKNOWN
>> 0x1 => BAD
>> 0x2 => GOOD
>> 0x3 => ? (invalid perhaps)
>>
>> HW that supports detecting good packets can set BAD || GOOD state, HW can detect
>> only BAD packet can set BAD || UNKNOWN state.
>>
>> If BAD is not set, there is an ambiguity of state, lets clarify it in lower
>> level, if it is UNKNOWN, let application know it is UNKNOWN.
> 
> OK.
> 
> How about the following then?
> 
> /**
>  * Mask of bits used to determine the status of outer RX L4 checksum.
>  * - PKT_RX_EL4_CKSUM_UNKNOWN: no information about the outer RX L4 checksum
>  * - PKT_RX_EL4_CKSUM_BAD: the outer L4 checksum in the packet is wrong
>  * - PKT_RX_EL4_CKSUM_GOOD: the outer L4 checksum in the packet is valid
>  * - PKT_RX_EL4_CKSUM_INVALID: invalid outer L4 checksum state.
>  *
>  * The detection of PKT_RX_EL4_CKSUM_GOOD shall be based on the given
>  * HW capability, At minimum, the PMD should support
>  * PKT_RX_EL4_CKSUM_UNKNOWN  and PKT_RX_EL4_CKSUM_BAD states
>  * if the offload is available.
>  */
> #define PKT_RX_EL4_CKSUM_MASK   ((1ULL << 21) | (1ULL << 22))
> 
> #define PKT_RX_IP_CKSUM_UNKNOWN 0
> #define PKT_RX_IP_CKSUM_BAD     (1ULL << 21)
> #define PKT_RX_IP_CKSUM_GOOD    (1ULL << 22)
> #define PKT_RX_IP_CKSUM_INVALID ((1ULL << 21) | (1ULL << 22))

Looks good to me.


> 
> 
>