From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 92FF9A09D2; Wed, 11 Nov 2020 10:32:11 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 386A6137D; Wed, 11 Nov 2020 10:32:10 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 4B4A9F90 for ; Wed, 11 Nov 2020 10:32:09 +0100 (CET) IronPort-SDR: zEZV7PV5eSMA8ZPrqM6XFYk0x2Ai3pv6rIDYqi1Hl9ePxOmPfZnV5rJZP7Gh8qmEGbtBxifI15 j38cZlOpRjEA== X-IronPort-AV: E=McAfee;i="6000,8403,9801"; a="170280947" X-IronPort-AV: E=Sophos;i="5.77,469,1596524400"; d="scan'208";a="170280947" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Nov 2020 01:32:02 -0800 IronPort-SDR: RPHVk5BL7mSpuQZggfGcCdNNv17Eg5aacQ10kagblIKhkynSfjSKMSRIIJaq2SyV647SgQVFtN yOmkmCq4GCKQ== X-IronPort-AV: E=Sophos;i="5.77,469,1596524400"; d="scan'208";a="531627759" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.241.123]) ([10.213.241.123]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Nov 2020 01:31:59 -0800 To: Michael Pfeiffer , Keith Wiles , Olivier Matz Cc: dev@dpdk.org References: <20201109142217.115918-1-michael.pfeiffer@tu-ilmenau.de> <23c6f497eb31840f0a2260bb3b5521fba8c7ec3b.camel@tu-ilmenau.de> From: Ferruh Yigit Message-ID: <40d63e07-f179-b3a8-ee7a-ff37e87c1008@intel.com> Date: Wed, 11 Nov 2020 09:31:56 +0000 MIME-Version: 1.0 In-Reply-To: <23c6f497eb31840f0a2260bb3b5521fba8c7ec3b.camel@tu-ilmenau.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] net/tap: Allow all-zero checksum for UDP over IPv4 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 11/11/2020 7:23 AM, Michael Pfeiffer wrote: > Hi, > > On Tue, 2020-11-10 at 15:59 +0000, Ferruh Yigit wrote: >> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote: >>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not >>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add >>> this special case to the checksum offload emulation in net/tap. >>> >>> Signed-off-by: Michael Pfeiffer >>> --- >>>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++-- >>>   1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c >>> index 2f8abb12c..e486b41c5 100644 >>> --- a/drivers/net/tap/rte_eth_tap.c >>> +++ b/drivers/net/tap/rte_eth_tap.c >>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf) >>>         uint16_t cksum = 0; >>>         void *l3_hdr; >>>         void *l4_hdr; >>> +       struct rte_udp_hdr *udp_hdr; >>> >>>         if (l2 == RTE_PTYPE_L2_ETHER_VLAN) >>>                 l2_len += 4; >>> @@ -349,10 +350,18 @@ tap_verify_csum(struct rte_mbuf *mbuf) >>>                 /* Don't verify checksum for multi-segment packets. */ >>>                 if (mbuf->nb_segs > 1) >>>                         return; >>> -               if (l3 == RTE_PTYPE_L3_IPV4) >>> +               if (l3 == RTE_PTYPE_L3_IPV4) { >>> +                       if (l4 == RTE_PTYPE_L4_UDP) { >>> +                               udp_hdr = (struct rte_udp_hdr *)l4_hdr; >>> +                               if (udp_hdr->dgram_cksum == 0) { >> >> Overall patch looks good to me, but can you please add a comment on top of >> above >> check to describe why checksum can be zero, as done in the commit log. > > Sure, I will update the patch. I am also not completely sure whether > PKT_RX_L4_CKSUM_NONE is the right flag for this case (rather than _UNKNOWN). > From rte_core_mbuf.h: > > * - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum > * - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet > * data, but the integrity of the L4 data is verified. > > The second part after the "but" is not really the case here. I don't know how > relevant the distinction is, as most application side code will probably only > do something like > > if ((mbuf->ol_flags & PKT_RX_L4_CKSUM_MASK) == PKT_RX_L4_CKSUM_BAD) > rte_pktmbuf_free(mbuf); > > anyway. Do you have any opinions on that? > I also checked for that and wasn't sure about it :) cc'ed Olivier too for comment. I think it is NOT 'PKT_RX_L4_CKSUM_UNKNOWN', since we know that checksum value is 0x0000 which means it is not provided. 'PKT_RX_L4_CKSUM_NONE' suits better but not sure about the expectation on "integrity of the L4 data is verified" part, I assume that explanation is just to differentiate between 'CKSUM_BAD'.