From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rcdn-iport-2.cisco.com (rcdn-iport-2.cisco.com [173.37.86.73]) by dpdk.org (Postfix) with ESMTP id E3DCC68D1 for ; Thu, 12 May 2016 03:32:19 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=10211; q=dns/txt; s=iport; t=1463016740; x=1464226340; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=8UUK+7sxB4uSlrNg3AJM7uLn0IhQhTHEJa1SGR752YU=; b=m9yisidHFwio81F2jQ5SqweNOD1+6na+7PXUKHFie1kB1TN//tgMgdSw 8r5DvtdTrEuJ8ex0XzaY0PEbj1CnvMYeIvs+tNCyQJsn5zTkA7+TevEaT Z2Sj48Xytxo25jZftA5/hCElW9Q2RiJKHoeIY/oNAemakhpPwAGDogSyl o=; X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0D1AQDs2zNX/4wNJK1egzhVfQa5QAENg?= =?us-ascii?q?XYihXICgT04FAEBAQEBAQFlJ4RCAQEBAwEnEz8FBwQCAQgRBAEBHwkHMhQJCAI?= =?us-ascii?q?EAQ0FCIgfCA66XwEBAQEBAQEBAQEBAQEBAQEBAQEBARWGIIRMhHGFJwWHfpApA?= =?us-ascii?q?YV9iBmBcE6EAYhhj0ABHgEBQoIEARuBS26HMn8BAQE?= Received: from alln-core-7.cisco.com ([173.36.13.140]) by rcdn-iport-2.cisco.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 12 May 2016 01:32:18 +0000 Received: from XCH-RCD-008.cisco.com (xch-rcd-008.cisco.com [173.37.102.18]) by alln-core-7.cisco.com (8.14.5/8.14.5) with ESMTP id u4C1WIpK002107 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Thu, 12 May 2016 01:32:18 GMT Received: from xch-rcd-007.cisco.com (173.37.102.17) by XCH-RCD-008.cisco.com (173.37.102.18) with Microsoft SMTP Server (TLS) id 15.0.1104.5; Wed, 11 May 2016 20:32:17 -0500 Received: from xch-rcd-007.cisco.com ([173.37.102.17]) by XCH-RCD-007.cisco.com ([173.37.102.17]) with mapi id 15.00.1104.009; Wed, 11 May 2016 20:32:18 -0500 From: "John Daley (johndale)" To: Olivier Matz , "dev@dpdk.org" CC: "konstantin.ananyev@intel.com" , "helin.zhang@intel.com" , "arnon@qwilt.com" , "rolette@infinite.io" Thread-Topic: [RFC] mbuf: remove unused rx error flags Thread-Index: AQHRqpekJCVz9rpkUkWFfSPKcJddUJ+0gOGw Date: Thu, 12 May 2016 01:32:17 +0000 Message-ID: <78fd3211f25f4d42aa481f6d741c8121@XCH-RCD-007.cisco.com> References: <1462869609-13139-1-git-send-email-olivier.matz@6wind.com> In-Reply-To: <1462869609-13139-1-git-send-email-olivier.matz@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.24.39.101] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [RFC] mbuf: remove unused rx error flags 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: Thu, 12 May 2016 01:32:20 -0000 Hi, > -----Original Message----- > From: Olivier Matz [mailto:olivier.matz@6wind.com] > Sent: Tuesday, May 10, 2016 1:40 AM > To: dev@dpdk.org > Cc: konstantin.ananyev@intel.com; John Daley (johndale) > ; helin.zhang@intel.com; arnon@qwilt.com; > rolette@infinite.io > Subject: [RFC] mbuf: remove unused rx error flags >=20 > Following the discussions from: > http://dpdk.org/ml/archives/dev/2015-July/021721.html > http://dpdk.org/ml/archives/dev/2016-April/038143.html >=20 > The value of these flags is 0, making them useless. Today, no example > application checks them on RX, and only few drivers sets them, and silent= ly > give wrong packets to the application, which should not happen. >=20 > This patch removes the unused flags from rte_mbuf, and their use in the > drivers. The enic driver is modified to drop bad packets, but i40e and fm= 10k > are kept as they are today and should be fixed to drop bad packets. Perhaps the change to enic to drop bad packets should be handled as a separ= ate patch since it's not strictly related to not removing the use of the fl= ags? >=20 > Fixes: c22265f6 ("mbuf: add new packet flags for i40e") > Signed-off-by: Olivier Matz > --- >=20 > Hi, >=20 > Here is a draft patch that removes the rx mbuf flags, as discussed. >=20 > John, I did not test the patch on the enic driver, so please review it ca= refully. >=20 The patch works for enic, thanks. There are a few minor changes for perform= ance: - put an unlikely in the if (packet_error) conditional - remove the if (!packet_error) conditional since it will always be true. Let me know if you would prefer I submit the enic patch separately. > Comments are welcome. >=20 > Olivier >=20 >=20 > drivers/net/enic/enic_rx.c | 31 ++++++++++++++----------------- > drivers/net/fm10k/fm10k_rxtx.c | 16 ---------------- > drivers/net/fm10k/fm10k_rxtx_vec.c | 2 +- > drivers/net/i40e/i40e_rxtx.c | 15 --------------- > lib/librte_mbuf/rte_mbuf.c | 4 ---- > lib/librte_mbuf/rte_mbuf.h | 5 +---- > 6 files changed, 16 insertions(+), 57 deletions(-) >=20 > diff --git a/drivers/net/enic/enic_rx.c b/drivers/net/enic/enic_rx.c inde= x > b3ad9ea..bad802e 100644 > --- a/drivers/net/enic/enic_rx.c > +++ b/drivers/net/enic/enic_rx.c > @@ -144,20 +144,15 @@ enic_cq_rx_desc_n_bytes(struct cq_desc *cqd) } >=20 > static inline uint8_t > -enic_cq_rx_to_pkt_err_flags(struct cq_desc *cqd, uint64_t > *pkt_err_flags_out) > +enic_cq_rx_check_err(struct cq_desc *cqd) > { > struct cq_enet_rq_desc *cqrd =3D (struct cq_enet_rq_desc *)cqd; > uint16_t bwflags; > - int ret =3D 0; > - uint64_t pkt_err_flags =3D 0; >=20 > bwflags =3D enic_cq_rx_desc_bwflags(cqrd); > - if (unlikely(enic_cq_rx_desc_packet_error(bwflags))) { > - pkt_err_flags =3D PKT_RX_MAC_ERR; > - ret =3D 1; > - } > - *pkt_err_flags_out =3D pkt_err_flags; > - return ret; > + if (unlikely(enic_cq_rx_desc_packet_error(bwflags))) > + return 1; > + return 0; > } >=20 > /* > @@ -253,7 +248,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, > struct enic *enic =3D vnic_dev_priv(rq->vdev); > unsigned int rx_id; > struct rte_mbuf *nmb, *rxmb; > - uint16_t nb_rx =3D 0; > + uint16_t nb_rx =3D 0, nb_err =3D 0; > uint16_t nb_hold; > struct vnic_cq *cq; > volatile struct cq_desc *cqd_ptr; > @@ -269,7 +264,6 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, > volatile struct rq_enet_desc *rqd_ptr; > dma_addr_t dma_addr; > struct cq_desc cqd; > - uint64_t ol_err_flags; > uint8_t packet_error; >=20 > /* Check for pkts available */ > @@ -293,7 +287,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, > } >=20 > /* A packet error means descriptor and data are untrusted */ > - packet_error =3D enic_cq_rx_to_pkt_err_flags(&cqd, > &ol_err_flags); > + packet_error =3D enic_cq_rx_check_err(&cqd); >=20 > /* Get the mbuf to return and replace with one just allocated > */ > rxmb =3D rq->mbuf_ring[rx_id]; > @@ -318,6 +312,13 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, > rqd_ptr->address =3D rte_cpu_to_le_64(dma_addr); > rqd_ptr->length_type =3D cpu_to_le16(nmb->buf_len); >=20 > + /* Drop incoming bad packet */ > + if (packet_error) { > + rte_pktmbuf_free(rxmb); > + nb_err++; > + continue; > + } > + > /* Fill in the rest of the mbuf */ > rxmb->data_off =3D RTE_PKTMBUF_HEADROOM; > rxmb->nb_segs =3D 1; > @@ -327,10 +328,6 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, > rxmb->pkt_len =3D enic_cq_rx_desc_n_bytes(&cqd); > rxmb->packet_type =3D > enic_cq_rx_flags_to_pkt_type(&cqd); > enic_cq_rx_to_pkt_flags(&cqd, rxmb); > - } else { > - rxmb->pkt_len =3D 0; > - rxmb->packet_type =3D 0; > - rxmb->ol_flags =3D 0; > } > rxmb->data_len =3D rxmb->pkt_len; >=20 > @@ -342,7 +339,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf > **rx_pkts, > rx_pkts[nb_rx++] =3D rxmb; > } >=20 > - nb_hold +=3D nb_rx; > + nb_hold +=3D nb_rx + nb_err; > cq->to_clean =3D rx_id; >=20 > if (nb_hold > rq->rx_free_thresh) { > diff --git a/drivers/net/fm10k/fm10k_rxtx.c > b/drivers/net/fm10k/fm10k_rxtx.c index 81ed4e7..dd92a91 100644 > --- a/drivers/net/fm10k/fm10k_rxtx.c > +++ b/drivers/net/fm10k/fm10k_rxtx.c > @@ -96,22 +96,6 @@ rx_desc_to_ol_flags(struct rte_mbuf *m, const union > fm10k_rx_desc *d) >=20 > if (d->w.pkt_info & FM10K_RXD_RSSTYPE_MASK) > m->ol_flags |=3D PKT_RX_RSS_HASH; > - > - if (unlikely((d->d.staterr & > - (FM10K_RXD_STATUS_IPCS | FM10K_RXD_STATUS_IPE)) =3D=3D > - (FM10K_RXD_STATUS_IPCS | FM10K_RXD_STATUS_IPE))) > - m->ol_flags |=3D PKT_RX_IP_CKSUM_BAD; > - > - if (unlikely((d->d.staterr & > - (FM10K_RXD_STATUS_L4CS | FM10K_RXD_STATUS_L4E)) =3D=3D > - (FM10K_RXD_STATUS_L4CS | FM10K_RXD_STATUS_L4E))) > - m->ol_flags |=3D PKT_RX_L4_CKSUM_BAD; > - > - if (unlikely(d->d.staterr & FM10K_RXD_STATUS_HBO)) > - m->ol_flags |=3D PKT_RX_HBUF_OVERFLOW; > - > - if (unlikely(d->d.staterr & FM10K_RXD_STATUS_RXE)) > - m->ol_flags |=3D PKT_RX_RECIP_ERR; > } >=20 > uint16_t > diff --git a/drivers/net/fm10k/fm10k_rxtx_vec.c > b/drivers/net/fm10k/fm10k_rxtx_vec.c > index 2f3ccfe..b368289 100644 > --- a/drivers/net/fm10k/fm10k_rxtx_vec.c > +++ b/drivers/net/fm10k/fm10k_rxtx_vec.c > @@ -101,7 +101,7 @@ fm10k_desc_to_olflags_v(__m128i descs[4], struct > rte_mbuf **rx_pkts) > const __m128i rxe_flag =3D _mm_set_epi8(0, 0, 0, 0, > 0, 0, 0, 0, > 0, 0, 0, 0, > - 0, 0, PKT_RX_RECIP_ERR, 0); > + 0, 0, 0, 0); >=20 > /* map rss type to rss hash flag */ > const __m128i rss_flags =3D _mm_set_epi8(0, 0, 0, 0, diff --git > a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index > 4d35d83..76da7d2 100644 > --- a/drivers/net/i40e/i40e_rxtx.c > +++ b/drivers/net/i40e/i40e_rxtx.c > @@ -140,27 +140,12 @@ i40e_rxd_error_to_pkt_flags(uint64_t qword) > #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; >=20 > return flags; > } > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c inde= x > eec1456..878db89 100644 > --- a/lib/librte_mbuf/rte_mbuf.c > +++ b/lib/librte_mbuf/rte_mbuf.c > @@ -254,10 +254,6 @@ const char *rte_get_rx_ol_flag_name(uint64_t > mask) > case PKT_RX_L4_CKSUM_BAD: return "PKT_RX_L4_CKSUM_BAD"; > case PKT_RX_IP_CKSUM_BAD: return "PKT_RX_IP_CKSUM_BAD"; > case PKT_RX_EIP_CKSUM_BAD: return "PKT_RX_EIP_CKSUM_BAD"; > - /* case PKT_RX_OVERSIZE: return "PKT_RX_OVERSIZE"; */ > - /* case PKT_RX_HBUF_OVERFLOW: return > "PKT_RX_HBUF_OVERFLOW"; */ > - /* case PKT_RX_RECIP_ERR: return "PKT_RX_RECIP_ERR"; */ > - /* case PKT_RX_MAC_ERR: return "PKT_RX_MAC_ERR"; */ > case PKT_RX_IEEE1588_PTP: return "PKT_RX_IEEE1588_PTP"; > case PKT_RX_IEEE1588_TMST: return "PKT_RX_IEEE1588_TMST"; > default: return NULL; > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h inde= x > e3ee0b3..3663d17 100644 > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -85,10 +85,7 @@ extern "C" { > #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 (1ULL << 5) /**< > 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. */ > +/* hole, some bits can be reused here */ > #define PKT_RX_IEEE1588_PTP (1ULL << 9) /**< RX IEEE1588 L2 Ethernet P= T > Packet. */ #define PKT_RX_IEEE1588_TMST (1ULL << 10) /**< RX IEEE1588 > L2/L4 timestamped packet.*/ > #define PKT_RX_FDIR_ID (1ULL << 13) /**< FD id reported if FDIR ma= tch. > */ > -- > 2.8.0.rc3