From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f42.google.com (mail-wg0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 6996C2E8B for ; Wed, 26 Nov 2014 12:11:27 +0100 (CET) Received: by mail-wg0-f42.google.com with SMTP id z12so3434493wgg.15 for ; Wed, 26 Nov 2014 03:22:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=HTB7IxjmT2DcehvHyIFSA4XaqjUKPelnjT/bexhimOw=; b=Dj2EPPPleNC7ofW6TE0/JM16XT4OVyddveDeNwvFH80y9vKh/n75+0hS8WCON7MzWO lgKkdZSJTPaeFds6HsW0US5kH76z2iYMASgv9pqHt6UoBO2jShpnewHBZuBB3y11Y9uZ ob513VCXpmITL+wYSajSeR+A8PVIwxrbr47QYIyng5rbsIh2mmDrS9ULXMe9aN/gQcMh VrpK8707NIjIdAkclqvUZ00H6n2lOueUX+Y2+qK3ko+deWRP+TXm8AvY9zEuGUAGlBl+ xpzLWyE8F6FxBchZV7uwmPnmUJ8UPWfkNKVRzZ9eXMD8lIXiUg5DLo+BTDw5+BuTz56u Uk1A== X-Gm-Message-State: ALoCoQn0m6avRPOcs6uGVdxKOcGL6RZUTJJqNzm34YztE5yszp/1LeNoXtvshgbvLxP8W3wMTyAN X-Received: by 10.194.79.199 with SMTP id l7mr48167815wjx.136.1417000943837; Wed, 26 Nov 2014 03:22:23 -0800 (PST) Received: from [10.16.0.195] (guy78-3-82-239-227-177.fbx.proxad.net. [82.239.227.177]) by mx.google.com with ESMTPSA id ji10sm20529374wid.7.2014.11.26.03.22.22 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 26 Nov 2014 03:22:23 -0800 (PST) Message-ID: <5475B7EE.4020400@6wind.com> Date: Wed, 26 Nov 2014 12:22:22 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Icedove/24.5.0 MIME-Version: 1.0 To: "Ananyev, Konstantin" , "Zhang, Helin" , "dev@dpdk.org" References: <1416982032-28519-1-git-send-email-helin.zhang@intel.com> <2601191342CEEE43887BDE71AB977258213BA7CA@IRSMSX105.ger.corp.intel.com> In-Reply-To: <2601191342CEEE43887BDE71AB977258213BA7CA@IRSMSX105.ger.corp.intel.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 26 Nov 2014 11:11:27 -0000 Hi Konstantin, Hi Helin, On 11/26/2014 11:49 AM, Ananyev, Konstantin wrote: > Hi Helin, > >> -----Original Message----- >> From: Zhang, Helin >> Sent: Wednesday, November 26, 2014 6:07 AM >> To: dev@dpdk.org >> Cc: Cao, Waterman; Cao, Min; Ananyev, Konstantin; olivier.matz@6wind.com; Zhang, Helin >> Subject: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors >> >> There were some bit flags of 0 for RX packet errors detected by hardware. >> Actually only one bit of error flag is enough for all hardware detected >> RX packet errors. >> >> Signed-off-by: Helin Zhang >> --- >> lib/librte_mbuf/rte_mbuf.h | 6 +----- >> lib/librte_pmd_i40e/i40e_rxtx.c | 31 +++---------------------------- >> 2 files changed, 4 insertions(+), 33 deletions(-) >> >> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h >> index 5899e5c..897fd26 100644 >> --- a/lib/librte_mbuf/rte_mbuf.h >> +++ b/lib/librte_mbuf/rte_mbuf.h >> @@ -80,11 +80,6 @@ extern "C" { >> #define PKT_RX_FDIR (1ULL << 2) /**< RX packet with FDIR match indicate. */ >> #define PKT_RX_L4_CKSUM_BAD (1ULL << 3) /**< L4 cksum of RX pkt. is not OK. */ >> #define PKT_RX_IP_CKSUM_BAD (1ULL << 4) /**< IP cksum of RX pkt. is not OK. */ >> -#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0) /**< External IP header checksum error. */ >> -#define PKT_RX_OVERSIZE (0ULL << 0) /**< Num of desc of an RX pkt oversize. */ >> -#define PKT_RX_HBUF_OVERFLOW (0ULL << 0) /**< Header buffer overflow. */ >> -#define PKT_RX_RECIP_ERR (0ULL << 0) /**< Hardware processing error. */ >> -#define PKT_RX_MAC_ERR (0ULL << 0) /**< MAC error. */ >> #define PKT_RX_IPV4_HDR (1ULL << 5) /**< RX packet with IPv4 header. */ >> #define PKT_RX_IPV4_HDR_EXT (1ULL << 6) /**< RX packet with extended IPv4 header. */ >> #define PKT_RX_IPV6_HDR (1ULL << 7) /**< RX packet with IPv6 header. */ >> @@ -95,6 +90,7 @@ extern "C" { >> #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet with IPv6 header. */ >> #define PKT_RX_FDIR_ID (1ULL << 13) /**< FD id reported if FDIR match. */ >> #define PKT_RX_FDIR_FLX (1ULL << 14) /**< Flexible bytes reported if FDIR match. */ >> +#define PKT_RX_ERR_HW (1ULL << 15) /**< RX packet error detected by hardware. */ >> >> #define PKT_TX_VLAN_PKT (1ULL << 55) /**< TX packet is a 802.1q VLAN packet. */ >> #define PKT_TX_IP_CKSUM (1ULL << 54) /**< IP cksum of TX pkt. computed by NIC. */ >> diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40e_rxtx.c >> index cce6911..3b2195d 100644 >> --- a/lib/librte_pmd_i40e/i40e_rxtx.c >> +++ b/lib/librte_pmd_i40e/i40e_rxtx.c >> @@ -115,35 +115,10 @@ i40e_rxd_status_to_pkt_flags(uint64_t qword) >> static inline uint64_t >> i40e_rxd_error_to_pkt_flags(uint64_t qword) >> { >> - uint64_t flags = 0; >> - uint64_t error_bits = (qword >> I40E_RXD_QW1_ERROR_SHIFT); >> - >> -#define I40E_RX_ERR_BITS 0x3f >> - if (likely((error_bits & I40E_RX_ERR_BITS) == 0)) >> - return flags; >> - /* If RXE bit set, all other status bits are meaningless */ >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_RXE_SHIFT))) { >> - flags |= PKT_RX_MAC_ERR; >> - return flags; >> - } >> - >> - /* If RECIPE bit set, all other status indications should be ignored */ >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_RECIPE_SHIFT))) { >> - flags |= PKT_RX_RECIP_ERR; >> - return flags; >> - } >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_HBO_SHIFT))) >> - flags |= PKT_RX_HBUF_OVERFLOW; >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_IPE_SHIFT))) >> - flags |= PKT_RX_IP_CKSUM_BAD; >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_L4E_SHIFT))) >> - flags |= PKT_RX_L4_CKSUM_BAD; >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_EIPE_SHIFT))) >> - flags |= PKT_RX_EIP_CKSUM_BAD; >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT))) >> - flags |= PKT_RX_OVERSIZE; >> + if (unlikely(qword & I40E_RXD_QW1_ERROR_MASK)) >> + return PKT_RX_ERR_HW; > > Probably I didn't explain myself clear enough, sorry. > I didn't suggest to get rid of setting bits that indicate L3/L4 checksum errors: > PKT_RX_IP_CKSUM_BAD, PKT_RX_L4_CKSUM_BAD, PKT_RX_EIP_CKSUM_BAD. > I think these flags should be set as before. > > I was talking only about collapsing only these 4 RX error flags into one: > > #define PKT_RX_OVERSIZE (0ULL << 0) /**< Num of desc of an RX pkt oversize. */ > #define PKT_RX_HBUF_OVERFLOW (0ULL << 0) /**< Header buffer overflow. */ > #define PKT_RX_RECIP_ERR (0ULL << 0) /**< Hardware processing error. */ > #define PKT_RX_MAC_ERR (0ULL << 0) /**< MAC error. */ > > From my point of view the difference of these 2 groups are: > First - HW was able to receive whole packet without a problem, but L3/L4 checksum check failed. > > Second - HW was not able to receive whole packet properly by whatever reason. > From upper layer SW perspective - there it probably makes little difference, what caused it, > as most likely SW has to throw away erroneous packet. > And for debugging purposes, we can add PMD_LOG(DEBUG, ...) that would print what exactly HW error happened. I agree with Konstantin that there are 2 different cases: a) the packet is properly received by the hardware, but has a bad checksum (or another protocol error, for instance an invalid ip len, a ip_version == 8 :)) in this case, it is useful to the application to have the mbuf with the data + an error flag. Then using a tcpdump-like tool could help to debug what is the cause of the error and what equipment generates a bad packet. b) the packet is not properly received by the hardware. In this case the data is invalid in the mbuf and not useable by the application. I suggest to only have a stats counter in this case, as receiving the mbuf is cpu time consuming and the only thing the application can do is to drop the packet. Regards, Olivier