[dpdk-stable] [PATCH v2 0/4] net/virtio: Fix packet segmentation bug

[dpdk-stable] [PATCH v2 0/4] net/virtio: Fix packet segmentation bug

[Maxime Coquelin]

This series fixes a bug reported by Yaroslav, where segmented
packets miss some segments and head segment's data_len was set
to a wrong value.

Last patch is not a fix, but it removes some useless checks
in Rx paths.

Maxime Coquelin (4):
  net/virtio: fix segmented packet issue in in-order Rx path
  net/virtio: fix segmented packet issue in mergeable Rx path
  net/virtio: fix segment data len in mergeable packed Rx path
  net/virtio: remove useless pointers checks

 drivers/net/virtio/virtio_rxtx.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

-- 
2.21.0

[dpdk-stable] [PATCH v2 1/4] net/virtio: fix segmented packet issue in in-order Rx path

[Maxime Coquelin]

After having dequeued a burst of descriptors, there may be a
need to dequeue a few more if the last packet was segmented
and not complete. When it happens, the extra segments were
not properly attached to the mbuf chain, and so were lost.

Also, head segment data_len field is wrongly summed with
the length of all the segments of the chain.

This patch fixes both the mbuf chaining and head segment's
data_len field.

Fixes: e5f456a98d3c ("net/virtio: support in-order Rx and Tx")
Cc: stable@dpdk.org

Reported-by: Yaroslav Brustinov <ybrustin@cisco.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 1de28540cd..6b3baf0423 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1424,7 +1424,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
 	struct virtqueue *vq = rxvq->vq;
 	struct virtio_hw *hw = vq->hw;
 	struct rte_mbuf *rxm;
-	struct rte_mbuf *prev;
+	struct rte_mbuf *prev = NULL;
 	uint16_t nb_used, num, nb_rx;
 	uint32_t len[VIRTIO_MBUF_BURST_SZ];
 	struct rte_mbuf *rcv_pkts[VIRTIO_MBUF_BURST_SZ];
@@ -1516,7 +1516,6 @@ virtio_recv_pkts_inorder(void *rx_queue,
 			rxm->data_len = (uint16_t)(len[i]);
 
 			rx_pkts[nb_rx]->pkt_len += (uint32_t)(len[i]);
-			rx_pkts[nb_rx]->data_len += (uint16_t)(len[i]);
 
 			if (prev)
 				prev->next = rxm;
@@ -1536,7 +1535,6 @@ virtio_recv_pkts_inorder(void *rx_queue,
 		uint16_t rcv_cnt = RTE_MIN((uint16_t)seg_res,
 					VIRTIO_MBUF_BURST_SZ);
 
-		prev = rcv_pkts[nb_rx];
 		if (likely(VIRTQUEUE_NUSED(vq) >= rcv_cnt)) {
 			virtio_rmb(hw->weak_barriers);
 			num = virtqueue_dequeue_rx_inorder(vq, rcv_pkts, len,
-- 
2.21.0

Re: [dpdk-stable] [PATCH v2 1/4] net/virtio: fix segmented packet issue in in-order Rx path

[David Marchand]

On Wed, Jun 5, 2019 at 10:10 AM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

> After having dequeued a burst of descriptors, there may be a
> need to dequeue a few more if the last packet was segmented
> and not complete. When it happens, the extra segments were
> not properly attached to the mbuf chain, and so were lost.
>
> Also, head segment data_len field is wrongly summed with
> the length of all the segments of the chain.
>
> This patch fixes both the mbuf chaining and head segment's
> data_len field.
>
> Fixes: e5f456a98d3c ("net/virtio: support in-order Rx and Tx")
> Cc: stable@dpdk.org
>
> Reported-by: Yaroslav Brustinov <ybrustin@cisco.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_rxtx.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c
> b/drivers/net/virtio/virtio_rxtx.c
> index 1de28540cd..6b3baf0423 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -1424,7 +1424,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
>         struct virtqueue *vq = rxvq->vq;
>         struct virtio_hw *hw = vq->hw;
>         struct rte_mbuf *rxm;
> -       struct rte_mbuf *prev;
> +       struct rte_mbuf *prev = NULL;
>         uint16_t nb_used, num, nb_rx;
>         uint32_t len[VIRTIO_MBUF_BURST_SZ];
>         struct rte_mbuf *rcv_pkts[VIRTIO_MBUF_BURST_SZ];
> @@ -1516,7 +1516,6 @@ virtio_recv_pkts_inorder(void *rx_queue,
>                         rxm->data_len = (uint16_t)(len[i]);
>
>                         rx_pkts[nb_rx]->pkt_len += (uint32_t)(len[i]);
> -                       rx_pkts[nb_rx]->data_len += (uint16_t)(len[i]);
>
>                         if (prev)
>                                 prev->next = rxm;
> @@ -1536,7 +1535,6 @@ virtio_recv_pkts_inorder(void *rx_queue,
>                 uint16_t rcv_cnt = RTE_MIN((uint16_t)seg_res,
>                                         VIRTIO_MBUF_BURST_SZ);
>
> -               prev = rcv_pkts[nb_rx];
>                 if (likely(VIRTQUEUE_NUSED(vq) >= rcv_cnt)) {
>                         virtio_rmb(hw->weak_barriers);
>                         num = virtqueue_dequeue_rx_inorder(vq, rcv_pkts,
> len,
>

I think you have missed another data_len update line 1554.

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

[dpdk-stable] [PATCH v2 2/4] net/virtio: fix segmented packet issue in mergeable Rx path

[Maxime Coquelin]

After having dequeued a burst of descriptors, there may be a
need to dequeue a few more if the last packet was segmented
and not complete. When it happens, the extra segments were
not properly attached to the mbuf chain, and so were lost.

Also, head segment data_len field is wrongly summed with
the length of all the segments of the chain.

This patch fixes both the mbuf chaining and head segment's
data_len field

Fixes: bcac5aa207f8 ("net/virtio: improve batching in mergeable path")
Cc: stable@dpdk.org

Reported-by: Yaroslav Brustinov <ybrustin@cisco.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 6b3baf0423..9469ecb743 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1614,7 +1614,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 	struct virtqueue *vq = rxvq->vq;
 	struct virtio_hw *hw = vq->hw;
 	struct rte_mbuf *rxm;
-	struct rte_mbuf *prev;
+	struct rte_mbuf *prev = NULL;
 	uint16_t nb_used, num, nb_rx = 0;
 	uint32_t len[VIRTIO_MBUF_BURST_SZ];
 	struct rte_mbuf *rcv_pkts[VIRTIO_MBUF_BURST_SZ];
@@ -1721,7 +1721,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 		uint16_t rcv_cnt = RTE_MIN((uint16_t)seg_res,
 					VIRTIO_MBUF_BURST_SZ);
 
-		prev = rcv_pkts[nb_rx];
 		if (likely(VIRTQUEUE_NUSED(vq) >= rcv_cnt)) {
 			virtio_rmb(hw->weak_barriers);
 			num = virtqueue_dequeue_burst_rx(vq, rcv_pkts, len,
@@ -1738,7 +1737,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 				prev->next = rxm;
 				prev = rxm;
 				rx_pkts[nb_rx]->pkt_len += len[extra_idx];
-				rx_pkts[nb_rx]->data_len += len[extra_idx];
 				extra_idx += 1;
 			};
 			seg_res -= rcv_cnt;
-- 
2.21.0

Re: [dpdk-stable] [PATCH v2 2/4] net/virtio: fix segmented packet issue in mergeable Rx path

[David Marchand]

On Wed, Jun 5, 2019 at 10:11 AM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

> After having dequeued a burst of descriptors, there may be a
> need to dequeue a few more if the last packet was segmented
> and not complete. When it happens, the extra segments were
> not properly attached to the mbuf chain, and so were lost.
>
> Also, head segment data_len field is wrongly summed with
> the length of all the segments of the chain.
>
> This patch fixes both the mbuf chaining and head segment's
> data_len field
>
> Fixes: bcac5aa207f8 ("net/virtio: improve batching in mergeable path")
> Cc: stable@dpdk.org
>
> Reported-by: Yaroslav Brustinov <ybrustin@cisco.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_rxtx.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c
> b/drivers/net/virtio/virtio_rxtx.c
> index 6b3baf0423..9469ecb743 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -1614,7 +1614,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
>         struct virtqueue *vq = rxvq->vq;
>         struct virtio_hw *hw = vq->hw;
>         struct rte_mbuf *rxm;
> -       struct rte_mbuf *prev;
> +       struct rte_mbuf *prev = NULL;
>         uint16_t nb_used, num, nb_rx = 0;
>         uint32_t len[VIRTIO_MBUF_BURST_SZ];
>         struct rte_mbuf *rcv_pkts[VIRTIO_MBUF_BURST_SZ];
> @@ -1721,7 +1721,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
>                 uint16_t rcv_cnt = RTE_MIN((uint16_t)seg_res,
>                                         VIRTIO_MBUF_BURST_SZ);
>
> -               prev = rcv_pkts[nb_rx];
>                 if (likely(VIRTQUEUE_NUSED(vq) >= rcv_cnt)) {
>                         virtio_rmb(hw->weak_barriers);
>                         num = virtqueue_dequeue_burst_rx(vq, rcv_pkts, len,
> @@ -1738,7 +1737,6 @@ virtio_recv_mergeable_pkts(void *rx_queue,
>                                 prev->next = rxm;
>                                 prev = rxm;
>                                 rx_pkts[nb_rx]->pkt_len += len[extra_idx];
> -                               rx_pkts[nb_rx]->data_len += len[extra_idx];
>                                 extra_idx += 1;
>                         };
>                         seg_res -= rcv_cnt;
> --
> 2.21.0
>
>
Idem previous patch, you have missed a data_len line 1704.
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

[dpdk-stable] [PATCH v2 3/4] net/virtio: fix segment data len in mergeable packed Rx path

[Maxime Coquelin]

Head segment data_len field is wrongly summed with the length
of all the segments of the chain, whereas it should be the
length of of the first segment only.

Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
Cc: stable@dpdk.org

Reported-by: Yaroslav Brustinov <ybrustin@cisco.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 9469ecb743..33dbc78906 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1914,7 +1914,6 @@ virtio_recv_mergeable_pkts_packed(void *rx_queue,
 				prev->next = rxm;
 				prev = rxm;
 				rx_pkts[nb_rx]->pkt_len += len[extra_idx];
-				rx_pkts[nb_rx]->data_len += len[extra_idx];
 				extra_idx += 1;
 			}
 			seg_res -= rcv_cnt;
-- 
2.21.0

Re: [dpdk-stable] [PATCH v2 3/4] net/virtio: fix segment data len in mergeable packed Rx path

[David Marchand]

On Wed, Jun 5, 2019 at 10:11 AM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

> Head segment data_len field is wrongly summed with the length
> of all the segments of the chain, whereas it should be the
> length of of the first segment only.
>
> Fixes: a76290c8f1cf ("net/virtio: implement Rx path for packed queues")
> Cc: stable@dpdk.org
>
> Reported-by: Yaroslav Brustinov <ybrustin@cisco.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_rxtx.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c
> b/drivers/net/virtio/virtio_rxtx.c
> index 9469ecb743..33dbc78906 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -1914,7 +1914,6 @@ virtio_recv_mergeable_pkts_packed(void *rx_queue,
>                                 prev->next = rxm;
>                                 prev = rxm;
>                                 rx_pkts[nb_rx]->pkt_len += len[extra_idx];
> -                               rx_pkts[nb_rx]->data_len += len[extra_idx];
>                                 extra_idx += 1;
>                         }
>                         seg_res -= rcv_cnt;
> --
> 2.21.0
>

And another one, line 1880 :-)
Reviewed-by: David Marchand <david.marchand@redhat.com>

-- 
David Marchand

[dpdk-stable] [PATCH v2 4/4] net/virtio: remove useless pointers checks

[Maxime Coquelin]

This patch removes uses checks on 'prev' pointer, as it
is always set before with a valid value.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 33dbc78906..fabd2e31d5 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1517,9 +1517,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
 
 			rx_pkts[nb_rx]->pkt_len += (uint32_t)(len[i]);
 
-			if (prev)
-				prev->next = rxm;
-
+			prev->next = rxm;
 			prev = rxm;
 			seg_res -= 1;
 		}
@@ -1703,9 +1701,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 			rx_pkts[nb_rx]->pkt_len += (uint32_t)(len[i]);
 			rx_pkts[nb_rx]->data_len += (uint16_t)(len[i]);
 
-			if (prev)
-				prev->next = rxm;
-
+			prev->next = rxm;
 			prev = rxm;
 			seg_res -= 1;
 		}
@@ -1879,9 +1875,7 @@ virtio_recv_mergeable_pkts_packed(void *rx_queue,
 			rx_pkts[nb_rx]->pkt_len += (uint32_t)(len[i]);
 			rx_pkts[nb_rx]->data_len += (uint16_t)(len[i]);
 
-			if (prev)
-				prev->next = rxm;
-
+			prev->next = rxm;
 			prev = rxm;
 			seg_res -= 1;
 		}
@@ -1924,8 +1918,7 @@ virtio_recv_mergeable_pkts_packed(void *rx_queue,
 		} else {
 			PMD_RX_LOG(ERR,
 					"No enough segments for packet.");
-			if (prev)
-				virtio_discard_rxbuf(vq, prev);
+			virtio_discard_rxbuf(vq, prev);
 			rxvq->stats.errors++;
 			break;
 		}
-- 
2.21.0

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 4/4] net/virtio: remove useless pointers checks

[David Marchand]

On Wed, Jun 5, 2019 at 10:11 AM Maxime Coquelin <maxime.coquelin@redhat.com>
wrote:

> This patch removes uses checks on 'prev' pointer, as it
> is always set before with a valid value.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/virtio/virtio_rxtx.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c
> b/drivers/net/virtio/virtio_rxtx.c
> index 33dbc78906..fabd2e31d5 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -1517,9 +1517,7 @@ virtio_recv_pkts_inorder(void *rx_queue,
>
>                         rx_pkts[nb_rx]->pkt_len += (uint32_t)(len[i]);
>
> -                       if (prev)
> -                               prev->next = rxm;
> -
> +                       prev->next = rxm;
>                         prev = rxm;
>                         seg_res -= 1;
>                 }
> @@ -1703,9 +1701,7 @@ virtio_recv_mergeable_pkts(void *rx_queue,
>                         rx_pkts[nb_rx]->pkt_len += (uint32_t)(len[i]);
>                         rx_pkts[nb_rx]->data_len += (uint16_t)(len[i]);
>
> -                       if (prev)
> -                               prev->next = rxm;
> -
> +                       prev->next = rxm;
>                         prev = rxm;
>                         seg_res -= 1;
>                 }
> @@ -1879,9 +1875,7 @@ virtio_recv_mergeable_pkts_packed(void *rx_queue,
>                         rx_pkts[nb_rx]->pkt_len += (uint32_t)(len[i]);
>                         rx_pkts[nb_rx]->data_len += (uint16_t)(len[i]);
>
> -                       if (prev)
> -                               prev->next = rxm;
> -
> +                       prev->next = rxm;
>                         prev = rxm;
>                         seg_res -= 1;
>                 }
> @@ -1924,8 +1918,7 @@ virtio_recv_mergeable_pkts_packed(void *rx_queue,
>                 } else {
>                         PMD_RX_LOG(ERR,
>                                         "No enough segments for packet.");
> -                       if (prev)
> -                               virtio_discard_rxbuf(vq, prev);
> +                       virtio_discard_rxbuf(vq, prev);
>                         rxvq->stats.errors++;
>                         break;
>                 }
> --
> 2.21.0
>
>
Reviewed-by: David Marchand <david.marchand@redhat.com>

-- 
David Marchand