From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id E2D491B173 for ; Fri, 5 Oct 2018 21:48:42 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Oct 2018 12:48:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,345,1534834800"; d="scan'208";a="78830188" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.49]) ([10.237.221.49]) by orsmga007.jf.intel.com with ESMTP; 05 Oct 2018 12:48:20 -0700 To: Jerin Jacob , Andrew Rybchenko Cc: Wenzhuo Lu , Jingjing Wu , Bernard Iremonger , John McNamara , Marko Kovacevic , Thomas Monjalon , Olivier Matz , dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin" References: <20180913134707.23698-1-jerin.jacob@caviumnetworks.com> <20181002192451.19119-1-jerin.jacob@caviumnetworks.com> <20181003075712.GA2003@jerin> <20181003171252.GA3193@jerin> <209397d1-f1ee-befb-1593-5adb58045bc5@solarflare.com> <20181003181412.GA8662@jerin> <8195dbcb-4b05-b47b-ebdf-2a8c36fa2974@solarflare.com> <20181004055930.GA4406@jerin> From: Ferruh Yigit Openpgp: preference=signencrypt Message-ID: <33afd274-71bd-2aa2-7f2c-a91838d32a58@intel.com> Date: Fri, 5 Oct 2018 20:48:20 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181004055930.GA4406@jerin> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Oct 2018 19:48:43 -0000 On 10/4/2018 6:59 AM, Jerin Jacob wrote: > -----Original Message----- >> Date: Wed, 3 Oct 2018 22:47:15 +0300 >> From: Andrew Rybchenko >> To: Jerin Jacob >> CC: Wenzhuo Lu , Jingjing Wu , >> Bernard Iremonger , John McNamara >> , Marko Kovacevic , >> Thomas Monjalon , Ferruh Yigit >> , Olivier Matz , >> dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin" >> >> 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 >>>> To: Jerin Jacob >>>> CC: Wenzhuo Lu , Jingjing Wu , >>>> Bernard Iremonger , John McNamara >>>> , Marko Kovacevic , >>>> Thomas Monjalon , Ferruh Yigit >>>> , Olivier Matz , >>>> dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin" >>>> >>>> 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 >>>>>> To: Andrew Rybchenko >>>>>> CC: Wenzhuo Lu , Jingjing Wu , >>>>>> Bernard Iremonger , John McNamara >>>>>> , Marko Kovacevic , >>>>>> Thomas Monjalon , Ferruh Yigit >>>>>> , Olivier Matz , >>>>>> dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin" >>>>>> >>>>>> 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 >>>>>>> To: Jerin Jacob , Wenzhuo Lu >>>>>>> , Jingjing Wu , Bernard >>>>>>> Iremonger , John McNamara >>>>>>> , Marko Kovacevic , >>>>>>> Thomas Monjalon , Ferruh Yigit >>>>>>> , Olivier Matz >>>>>>> CC: dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin" >>>>>>> >>>>>>> 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 >>>>>>> >>>>>>> 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 " 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. >>