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

["John Daley (johndale)" <johndale@cisco.com>]

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)
> <johndale@cisco.com>; helin.zhang@intel.com; arnon@qwilt.com;
> rolette@infinite.io
> Subject: [RFC] mbuf: remove unused rx error flags
> 
> 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.

Perhaps the change to enic to drop bad packets should be handled as a separate patch since it's not strictly related to not removing the use of the flags?

> 
> 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.
> 

The patch works for enic, thanks. There are a few minor changes for performance:
 - 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.
> 
> 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