From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition Date: Wed, 03 Oct 2018 22:08:13 +0200 Message-ID: <1780274.oBxpks6VXQ@xps> References: <20180913134707.23698-1-jerin.jacob@caviumnetworks.com> <20181003181412.GA8662@jerin> <8195dbcb-4b05-b47b-ebdf-2a8c36fa2974@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: Wenzhuo Lu , Jingjing Wu , Bernard Iremonger , John McNamara , Marko Kovacevic , Ferruh Yigit , Olivier Matz , dev@dpdk.org, shahafs@mellanox.com, "Ananyev, Konstantin" To: Andrew Rybchenko , Jerin Jacob Return-path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id 7C8591B137 for ; Wed, 3 Oct 2018 22:08:17 +0200 (CEST) In-Reply-To: <8195dbcb-4b05-b47b-ebdf-2a8c36fa2974@solarflare.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 03/10/2018 21:47, Andrew Rybchenko: > On 03.10.2018 21:14, Jerin Jacob wrote: > > From: Andrew Rybchenko > >>> From: Jerin Jacob > >>>> From: Andrew Rybchenko > >>>>> 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. > > >> 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 :) > > If everyone else is fine with the description of Rx checksum offloads and > it is only me who is unhappy with it - no problem. I agree we must better define these checksum flags. Olivier, please could you give your opinion here?