From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 425742E8B for ; Wed, 26 Nov 2014 14:28:26 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP; 26 Nov 2014 05:38:48 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.07,462,1413270000"; d="scan'208";a="638411793" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by fmsmga002.fm.intel.com with ESMTP; 26 Nov 2014 05:38:14 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.144]) by IRSMSX103.ger.corp.intel.com ([169.254.3.134]) with mapi id 14.03.0195.001; Wed, 26 Nov 2014 13:38:14 +0000 From: "Ananyev, Konstantin" To: Olivier MATZ , "Zhang, Helin" , "dev@dpdk.org" Thread-Topic: [PATCH] i40e: Use one bit flag for all hardware detected RX packet errors Thread-Index: AQHQCT9a48VJuIAuXkSUeu0M/l6yQJxyr+QQgAAUGwCAABu+kA== Date: Wed, 26 Nov 2014 13:38:13 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258213BA8CB@IRSMSX105.ger.corp.intel.com> References: <1416982032-28519-1-git-send-email-helin.zhang@intel.com> <2601191342CEEE43887BDE71AB977258213BA7CA@IRSMSX105.ger.corp.intel.com> <5475B7EE.4020400@6wind.com> In-Reply-To: <5475B7EE.4020400@6wind.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 13:28:27 -0000 > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Wednesday, November 26, 2014 11:22 AM > To: Ananyev, Konstantin; Zhang, Helin; dev@dpdk.org > Cc: Cao, Waterman; Cao, Min > Subject: Re: [PATCH] i40e: Use one bit flag for all hardware detected RX = packet errors >=20 > Hi Konstantin, Hi Helin, >=20 > 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.c= om; Zhang, Helin > >> Subject: [PATCH] i40e: Use one bit flag for all hardware detected RX p= acket errors > >> > >> There were some bit flags of 0 for RX packet errors detected by hardwa= re. > >> Actually only one bit of error flag is enough for all hardware detecte= d > >> 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 m= atch indicate. */ > >> #define PKT_RX_L4_CKSUM_BAD (1ULL << 3) /**< L4 cksum of RX pkt. i= s not OK. */ > >> #define PKT_RX_IP_CKSUM_BAD (1ULL << 4) /**< IP cksum of RX pkt. i= s not OK. */ > >> -#define PKT_RX_EIP_CKSUM_BAD (0ULL << 0) /**< External IP header che= cksum error. */ > >> -#define PKT_RX_OVERSIZE (0ULL << 0) /**< Num of desc of an RX p= kt oversize. */ > >> -#define PKT_RX_HBUF_OVERFLOW (0ULL << 0) /**< Header buffer overflow= . */ > >> -#define PKT_RX_RECIP_ERR (0ULL << 0) /**< Hardware processing er= ror. */ > >> -#define PKT_RX_MAC_ERR (0ULL << 0) /**< MAC error. */ > >> #define PKT_RX_IPV4_HDR (1ULL << 5) /**< RX packet with IPv4 h= eader. */ > >> #define PKT_RX_IPV4_HDR_EXT (1ULL << 6) /**< RX packet with extend= ed IPv4 header. */ > >> #define PKT_RX_IPV6_HDR (1ULL << 7) /**< RX packet with IPv6 h= eader. */ > >> @@ -95,6 +90,7 @@ extern "C" { > >> #define PKT_RX_TUNNEL_IPV6_HDR (1ULL << 12) /**< RX tunnel packet wi= th IPv6 header. */ > >> #define PKT_RX_FDIR_ID (1ULL << 13) /**< FD id reported if FDI= R match. */ > >> #define PKT_RX_FDIR_FLX (1ULL << 14) /**< Flexible bytes report= ed if FDIR match. */ > >> +#define PKT_RX_ERR_HW (1ULL << 15) /**< RX packet error detect= ed 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. c= omputed by NIC. */ > >> diff --git a/lib/librte_pmd_i40e/i40e_rxtx.c b/lib/librte_pmd_i40e/i40= e_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 =3D 0; > >> - uint64_t error_bits =3D (qword >> I40E_RXD_QW1_ERROR_SHIFT); > >> - > >> -#define I40E_RX_ERR_BITS 0x3f > >> - if (likely((error_bits & I40E_RX_ERR_BITS) =3D=3D 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 |=3D 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 |=3D PKT_RX_RECIP_ERR; > >> - return flags; > >> - } > >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_HBO_SHIFT))) > >> - flags |=3D PKT_RX_HBUF_OVERFLOW; > >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_IPE_SHIFT))) > >> - flags |=3D PKT_RX_IP_CKSUM_BAD; > >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_L4E_SHIFT))) > >> - flags |=3D PKT_RX_L4_CKSUM_BAD; > >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_EIPE_SHIFT))) > >> - flags |=3D PKT_RX_EIP_CKSUM_BAD; > >> - if (unlikely(error_bits & (1 << I40E_RX_DESC_ERROR_OVERSIZE_SHIFT))) > >> - flags |=3D 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 checksu= m 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 on= e: > > > > #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 erro= r. */ > > #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/L= 4 checksum check failed. > > > > Second - HW was not able to receive whole packet properly by whatever r= eason. > > From upper layer SW perspective - there it probably makes little diffe= rence, 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 p= rint what exactly HW error happened. >=20 > I agree with Konstantin that there are 2 different cases: >=20 > 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 =3D=3D 8 :)) >=20 > 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. >=20 > 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. So for b) you suggest to drop the packet straight in PMD RX function? Something like: if (unlikely(error_bits & ...)) { PMD_LOG(DEBUG, ...); rte_pktmbuf_free(mb); } ? That's probably a bit too radical.=20 Yes, mbuf doesn't contain the whole packet, but it may contain at least par= t of it, let say in case of 'packet oversize'.=20 So for debugging purposes the user may still like to examine the mbuf conte= nts. Konstantin >=20 > Regards, > Olivier