[dpdk-dev] [RFC] mbuf: remove unused rx error flags
Hi, On 05/12/2016 03:32 AM, John Daley (johndale) wrote: >> 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? Yes, it's probably better to have it in a separate patch. >> 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. I'll do it, thanks for reviewing. I'll wait a bit for other comments before sending a first version of the patchset. Regards, Olivier
[dpdk-dev] [RFC] mbuf: remove unused rx error flags
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)
> ; 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
> ---
>
> 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(,
> _err_flags);
> + packet_error = enic_cq_rx_check_err();
>
> /* 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();
> rxmb->packet_type =
> enic_cq_rx_flags_to_pkt_type();
>
[dpdk-dev] [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.
Fixes: c22265f6 ("mbuf: add new packet flags for i40e")
Signed-off-by: Olivier Matz
---
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(, _err_flags);
+ packet_error = enic_cq_rx_check_err();
/* 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();
rxmb->packet_type = enic_cq_rx_flags_to_pkt_type();
enic_cq_rx_to_pkt_flags(, 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
