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

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

On 10/4/2018 6:59 AM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Wed, 3 Oct 2018 22:47:15 +0300
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>  Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>  <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>  Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>  <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>  dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>  <konstantin.ananyev@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>  checksum definition
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>  Thunderbird/52.9.1
>>
>>
>> Hi Jerin,
> 
> Hi Andrew,
> 
>>
>> On 03.10.2018 21:14, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Wed, 3 Oct 2018 21:00:37 +0300
>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>>>   Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>   <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>   Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>   <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>>>   dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>   <konstantin.ananyev@intel.com>
>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>   checksum definition
>>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>>>   Thunderbird/52.9.1
>>>>
>>>> On 03.10.2018 20:12, Jerin Jacob wrote:
>>>>> -----Original Message-----
>>>>>> Date: Wed, 3 Oct 2018 13:27:13 +0530
>>>>>> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>>> To: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>> CC: Wenzhuo Lu <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>,
>>>>>>    Bernard Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>>>    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>>>    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>>>    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>,
>>>>>>    dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>>>    <konstantin.ananyev@intel.com>
>>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>>>    checksum definition
>>>>>> User-Agent: Mutt/1.10.1 (2018-07-13)
>>>>>>
>>>>>> External Email
>>>>>>
>>>>>> -----Original Message-----
>>>>>>> Date: Wed, 3 Oct 2018 10:34:52 +0300
>>>>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, Wenzhuo Lu
>>>>>>>    <wenzhuo.lu@intel.com>, Jingjing Wu <jingjing.wu@intel.com>, Bernard
>>>>>>>    Iremonger <bernard.iremonger@intel.com>, John McNamara
>>>>>>>    <john.mcnamara@intel.com>, Marko Kovacevic <marko.kovacevic@intel.com>,
>>>>>>>    Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>>>>>>>    <ferruh.yigit@intel.com>, Olivier Matz <olivier.matz@6wind.com>
>>>>>>> CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin"
>>>>>>>    <konstantin.ananyev@intel.com>
>>>>>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP
>>>>>>>    checksum definition
>>>>>>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
>>>>>>>    Thunderbird/60.0
>>>>>>>
>>>>>>>
>>>>>>> On 10/2/18 10:24 PM, Jerin Jacob wrote:
>>>>>>>
>>>>>>> Introduced DEV_RX_OFFLOAD_OUTER_UDP_CKSUM Rx offload flag and
>>>>>>> PKT_RX_EL4_CKSUM_BAD mbuf ol_flags to detect outer UDP checksum
>>>>>>> failure.
>>>>>>>
>>>>>>> - To use hardware Rx outer UDP checksum offload, the user needs to
>>>>>>> configure DEV_RX_OFFLOAD_OUTER_UDP_CKSUM offload flags in slowpath.
>>>>>>>
>>>>>>> - Driver updates the PKT_RX_EL4_CKSUM_BAD mbuf ol_flag on checksum failure
>>>>>>> similar to the outer L3 PKT_RX_EIP_CKSUM_BAD flag.
>>>>>>>
>>>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com><mailto:jerin.jacob@caviumnetworks.com>
>>>>>>>
>>>>>>> 1. I'm not sure that it is OK that mbuf and ethdev changes go in one patch.
>>>>>>>      It seems typically mbuf changes go separately and mbuf changes should
>>>>>>>      be applied to main dpdk repo.
>>>>>> I don't have strong opinion on this. If there are no other objection, I
>>>>>> will split the patch further as mbuf and ethdev as you pointed out.
>>>>>>
>>>>>>> 2. I'd like to see thought why single bit is used for outer L2 checksum when
>>>>>>>      2 bits (UNKNOWN, BAD, GOOD, NONE) are used for PKT_RX_L4_CKSUM.
>>>>>>>      May be it is OK, but it would be useful to state explicitly why it is decided
>>>>>>>      to go this way.
>>>>>> I am following the scheme similar to OUTER IP checksum where we have only
>>>>>> one bit filed(PKT_RX_EIP_CKSUM_BAD). I will mention in the git commit.
>>>>>>
>>>>>>
>>>>>>> 3. PKT_RX_L4_CKSUM_MASK description says nothing if it is inner or outer.
>>>>>>>      May be it is not directly related to changeset, but I think it would be really
>>>>>>>      useful to clarify it.
>>>>>> I will update the comment.
>>>>> Hi Andrew,
>>>>>
>>>>> I looked at the other definitions in mbuf.h, according the documentation,
>>>>> If nothing is mentioned it is treated as inner if the packet is
>>>>> tunneled else it is outer most. So I would like avoid confusion by
>>>>> adding "inner" in the exiting PKT_RX_L4_CKSUM_MASK comment.
>>>>> Technically it is not correct to say "inner" if the packet is not
>>>>> tunneled. So I am untouching the exiting comment.
>>>>>
>>>> Yes, it is incorrect to say that it is inner. How does application find
>>>> how to treat PKT_RX_L4_CKSUM (inner or outer)?
>>>> Should it rely on packet type provided in mbuf?
>>> AFAIK, Finding is it a tunneled packet or not is through ptype or SW has
>>> to parse the packet. For example, testpmd chooses later method using
>>> "csum parse-tunnel on <port>" to detect the presence of the tunnel.
>>
>> SW parsing of the packet cannot help, since app should be sure
>> that HW has classified the packet as tunneled and provided information
>> about inner and outer checksum checks.
> 
> 
> I thought the question was, How does the application find how to treat
> PKT_RX_L4_CKSUM (inner or outer)?
> Obviously, ptype will help here
> Not sure why SW parsing won't help here if SW parses and find it is an
> inner packet or it is not a tunneled packet. PKT_RX_L4_CKSUM treat as
> inner checksum for former case and PKT_RX_L4_CKSUM treated as plain l4
> checksum in the latter case.
> 
>>
>>>> Is it specified/mentioned somewhere?
>>> I don't know. It it not directly related to this change set, Olivier may know
>>> additional details.
>>
>> I disagree. You're adding one more offload flag. Yes, it simply follows
>> existing RX_EIP_CKSUM_BAD pattern. But, IMHO, RX_EIP_CKSUM_BAD
>> has many open questions. Why should these open questions be preserved
>> here? It is similar to the code with a bug which is cloned once again with
>> the bug :)
> 
> 
> No disagreement here. I choose to follow the existing scheme, because if I
> do another way around, still I will get the question on why it is different
> than the outer IPV4 checksum scheme.
> 
> Looking at the history, the mbuf DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM change
> went part of ixgbe change
> d909af8f72ca ("ixgbe: offload VxLAN and NVGRE Rx checksum on X550").
> 
> Al east in the HW I know, We can support "two bit" fields for Outer IP
> checksum and Outer L4 checksum.
> 
> So I think, the decision was made based on ixgbe capability or probably
> no one noticed the change as the subject was ixgbe:
> 
> Anyway, i don't have strong preferences on 1 bit vs 2 bits. I think, 1
> bit can be supported by all the HW if supports this feature. Leaving the
> decision to ethdev and mbuf maintainers.

+1 to Andrew, only PKT_RX_EL4_CKSUM_BAD bit is not clear when it is not set.
PKT_RX_IP_CKSUM_* approach looks better.

And agreed PKT_RX_EIP_CKSUM_BAD has same problem.

> 
>>
>> If everyone else is fine with the description of Rx checksum offloads and
>> it is only me who is unhappy with it - no problem.
>>
>> Thanks for your patience and I'm sorry that I'm really boring with it.
>> My goal is just to make it clear and as the result have less bugs in
>> networking PMDs and applications.
> 
> No problem. :-)
> 
> 
>>
>> Andrew.
>>