[dpdk-dev] [RFC] mbuf: remove unused rx error flags

[Olivier Matz <olivier.matz@6wind.com>]

Following the discussions from:
http://dpdk.org/ml/archives/dev/2015-July/021721.html
http://dpdk.org/ml/archives/dev/2016-April/038143.html

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
silently give wrong packets to the application, which should not happen.

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
fm10k are kept as they are today and should be fixed to drop bad packets.

Fixes: c22265f6 ("mbuf: add new packet flags for i40e")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---

Hi,

Here is a draft patch that removes the rx mbuf flags, as discussed.

John, I did not test the patch on the enic driver, so please review it
carefully.

Comments are welcome.

Olivier


 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(-)

diff --git a/drivers/net/enic/enic_rx.c b/drivers/net/enic/enic_rx.c
index 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)
 }
 
 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 = (struct cq_enet_rq_desc *)cqd;
 	uint16_t bwflags;
-	int ret = 0;
-	uint64_t pkt_err_flags = 0;
 
 	bwflags = enic_cq_rx_desc_bwflags(cqrd);
-	if (unlikely(enic_cq_rx_desc_packet_error(bwflags))) {
-		pkt_err_flags = PKT_RX_MAC_ERR;
-		ret = 1;
-	}
-	*pkt_err_flags_out = pkt_err_flags;
-	return ret;
+	if (unlikely(enic_cq_rx_desc_packet_error(bwflags)))
+		return 1;
+	return 0;
 }
 
 /*
@@ -253,7 +248,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 	struct enic *enic = vnic_dev_priv(rq->vdev);
 	unsigned int rx_id;
 	struct rte_mbuf *nmb, *rxmb;
-	uint16_t nb_rx = 0;
+	uint16_t nb_rx = 0, nb_err = 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;
 
 		/* Check for pkts available */
@@ -293,7 +287,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		}
 
 		/* A packet error means descriptor and data are untrusted */
-		packet_error = enic_cq_rx_to_pkt_err_flags(&cqd, &ol_err_flags);
+		packet_error = enic_cq_rx_check_err(&cqd);
 
 		/* Get the mbuf to return and replace with one just allocated */
 		rxmb = rq->mbuf_ring[rx_id];
@@ -318,6 +312,13 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		rqd_ptr->address = rte_cpu_to_le_64(dma_addr);
 		rqd_ptr->length_type = cpu_to_le16(nmb->buf_len);
 
+		/* Drop incoming bad packet */
+		if (packet_error) {
+			rte_pktmbuf_free(rxmb);
+			nb_err++;
+			continue;
+		}
+
 		/* Fill in the rest of the mbuf */
 		rxmb->data_off = RTE_PKTMBUF_HEADROOM;
 		rxmb->nb_segs = 1;
@@ -327,10 +328,6 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 			rxmb->pkt_len = enic_cq_rx_desc_n_bytes(&cqd);
 			rxmb->packet_type = enic_cq_rx_flags_to_pkt_type(&cqd);
 			enic_cq_rx_to_pkt_flags(&cqd, rxmb);
-		} else {
-			rxmb->pkt_len = 0;
-			rxmb->packet_type = 0;
-			rxmb->ol_flags = 0;
 		}
 		rxmb->data_len = rxmb->pkt_len;
 
@@ -342,7 +339,7 @@ enic_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		rx_pkts[nb_rx++] = rxmb;
 	}
 
-	nb_hold += nb_rx;
+	nb_hold += nb_rx + nb_err;
 	cq->to_clean = rx_id;
 
 	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)
 
 	if (d->w.pkt_info & FM10K_RXD_RSSTYPE_MASK)
 		m->ol_flags |= PKT_RX_RSS_HASH;
-
-	if (unlikely((d->d.staterr &
-		(FM10K_RXD_STATUS_IPCS | FM10K_RXD_STATUS_IPE)) ==
-		(FM10K_RXD_STATUS_IPCS | FM10K_RXD_STATUS_IPE)))
-		m->ol_flags |= PKT_RX_IP_CKSUM_BAD;
-
-	if (unlikely((d->d.staterr &
-		(FM10K_RXD_STATUS_L4CS | FM10K_RXD_STATUS_L4E)) ==
-		(FM10K_RXD_STATUS_L4CS | FM10K_RXD_STATUS_L4E)))
-		m->ol_flags |= PKT_RX_L4_CKSUM_BAD;
-
-	if (unlikely(d->d.staterr & FM10K_RXD_STATUS_HBO))
-		m->ol_flags |= PKT_RX_HBUF_OVERFLOW;
-
-	if (unlikely(d->d.staterr & FM10K_RXD_STATUS_RXE))
-		m->ol_flags |= PKT_RX_RECIP_ERR;
 }
 
 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 = _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);
 
 	/* map rss type to rss hash flag */
 	const __m128i rss_flags = _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) == 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;
 
 	return flags;
 }
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 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
index 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 PT 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 match. */
-- 
2.8.0.rc3