From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerin Jacob Subject: Re: [PATCH v2 1/4] ethdev: add Rx offload outer UDP checksum definition Date: Wed, 3 Oct 2018 13:27:13 +0530 Message-ID: <20181003075712.GA2003@jerin> References: <20180913134707.23698-1-jerin.jacob@caviumnetworks.com> <20181002192451.19119-1-jerin.jacob@caviumnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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" To: Andrew Rybchenko Return-path: Received: from NAM02-SN1-obe.outbound.protection.outlook.com (mail-sn1nam02on0062.outbound.protection.outlook.com [104.47.36.62]) by dpdk.org (Postfix) with ESMTP id 525562C2B for ; Wed, 3 Oct 2018 09:57:36 +0200 (CEST) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" -----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. > > > Plus one nit below. > > > > --- > > v2: > - Removed DEV_RX_OFFLOAD_OUTER_TCP_CKSUM and DEV_RX_OFFLOAD_OUTER_SCTP_CKSUM > as there is no realworld use case for it. > See: http://patches.dpdk.org/patch/44692/ > > This patch series is depended on http://patches.dpdk.org/patch/45840/ > > -- > app/test-pmd/config.c | 9 +++++++++ > doc/guides/nics/features.rst | 3 +++ > lib/librte_ethdev/rte_ethdev.c | 1 + > lib/librte_ethdev/rte_ethdev.h | 1 + > lib/librte_mbuf/rte_mbuf.c | 2 ++ > lib/librte_mbuf/rte_mbuf.h | 3 +++ > 6 files changed, 19 insertions(+) > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index 1adc9b94b..d53c527e5 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -594,6 +594,15 @@ port_offload_cap_display(portid_t port_id) > printf("off\n"); > } > > + if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_OUTER_UDP_CKSUM) { > + printf("RX Outer UDP checksum: "); > + if (ports[port_id].dev_conf.rxmode.offloads & > + DEV_RX_OFFLOAD_OUTER_UDP_CKSUM) > + printf("on\n"); > + else > + printf("off\n"); > + } > + > if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_TCP_LRO) { > printf("Large receive offload: "); > if (ports[port_id].dev_conf.rxmode.offloads & > diff --git a/doc/guides/nics/features.rst b/doc/guides/nics/features.rst > index d42489b6d..2c2959e0b 100644 > --- a/doc/guides/nics/features.rst > +++ b/doc/guides/nics/features.rst > @@ -639,6 +639,9 @@ Inner L4 checksum > > Supports inner packet L4 checksum. > > +* **[uses] rte_eth_rxconf,rte_eth_rxmode**: ``offloads:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``. > +* **[provides] mbuf**: ``mbuf.ol_flags:PKT_RX_EL4_CKSUM_BAD``. > +* **[provides] rte_eth_dev_info**: ``rx_offload_capa,rx_queue_offload_capa:DEV_RX_OFFLOAD_OUTER_UDP_CKSUM``, > > > One more empty line should be added here to have two empty lines between features. OK. Thanks for the review. > > Andrew.