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

Message ID 20190605081005.15716-2-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: Fix packet segmentation bug |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation fail apply issues

Commit Message

Maxime Coquelin June 5, 2019, 8:10 a.m. UTC
  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(-)
  

Comments

David Marchand June 5, 2019, 9:34 a.m. UTC | #1
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>
  

Patch

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,