On 10/4/2018 6:59 AM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Wed, 3 Oct 2018 22:47:15 +0300
>> From: Andrew Rybchenko <arybche...@solarflare.com>
>> To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
>> CC: Wenzhuo Lu <wenzhuo...@intel.com>, Jingjing Wu <jingjing...@intel.com>,
>> Bernard Iremonger <bernard.iremon...@intel.com>, John McNamara
>> <john.mcnam...@intel.com>, Marko Kovacevic <marko.kovace...@intel.com>,
>> Thomas Monjalon <tho...@monjalon.net>, Ferruh Yigit
>> <ferruh.yi...@intel.com>, Olivier Matz <olivier.m...@6wind.com>,
>> dev@dpdk.org, shah...@mellanox.com, "Ananyev, Konstantin"
>> <konstantin.anan...@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 <arybche...@solarflare.com>
>>>> To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
>>>> CC: Wenzhuo Lu <wenzhuo...@intel.com>, Jingjing Wu <jingjing...@intel.com>,
>>>> Bernard Iremonger <bernard.iremon...@intel.com>, John McNamara
>>>> <john.mcnam...@intel.com>, Marko Kovacevic <marko.kovace...@intel.com>,
>>>> Thomas Monjalon <tho...@monjalon.net>, Ferruh Yigit
>>>> <ferruh.yi...@intel.com>, Olivier Matz <olivier.m...@6wind.com>,
>>>> dev@dpdk.org, shah...@mellanox.com, "Ananyev, Konstantin"
>>>> <konstantin.anan...@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.ja...@caviumnetworks.com>
>>>>>> To: Andrew Rybchenko <arybche...@solarflare.com>
>>>>>> CC: Wenzhuo Lu <wenzhuo...@intel.com>, Jingjing Wu
>>>>>> <jingjing...@intel.com>,
>>>>>> Bernard Iremonger <bernard.iremon...@intel.com>, John McNamara
>>>>>> <john.mcnam...@intel.com>, Marko Kovacevic
>>>>>> <marko.kovace...@intel.com>,
>>>>>> Thomas Monjalon <tho...@monjalon.net>, Ferruh Yigit
>>>>>> <ferruh.yi...@intel.com>, Olivier Matz <olivier.m...@6wind.com>,
>>>>>> dev@dpdk.org, shah...@mellanox.com, "Ananyev, Konstantin"
>>>>>> <konstantin.anan...@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 <arybche...@solarflare.com>
>>>>>>> To: Jerin Jacob <jerin.ja...@caviumnetworks.com>, Wenzhuo Lu
>>>>>>> <wenzhuo...@intel.com>, Jingjing Wu <jingjing...@intel.com>, Bernard
>>>>>>> Iremonger <bernard.iremon...@intel.com>, John McNamara
>>>>>>> <john.mcnam...@intel.com>, Marko Kovacevic
>>>>>>> <marko.kovace...@intel.com>,
>>>>>>> Thomas Monjalon <tho...@monjalon.net>, Ferruh Yigit
>>>>>>> <ferruh.yi...@intel.com>, Olivier Matz <olivier.m...@6wind.com>
>>>>>>> CC: dev@dpdk.org, shah...@mellanox.com, "Ananyev, Konstantin"
>>>>>>> <konstantin.anan...@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.ja...@caviumnetworks.com><mailto:jerin.ja...@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.
I don't know if PKT_RX_L4_CKSUM is inner or outer with current design,
but wouldn't be easier if PKT_RX_L4_CKSUM always refer to outer and if tunneled
use something like PKT_RX_INNER_L4_CKSUM, same for IP and TX.
>
>>
>>>> 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.
>
>>
>> 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.
>>
