Hi,
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Tuesday, May 10, 2016 1:40 AM
> To: dev at dpdk.org
> Cc: konstantin.ananyev at intel.com; John Daley (johndale)
> <johndale at cisco.com>; helin.zhang at intel.com; arnon at qwilt.com;
> rolette at 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 at 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
