[dpdk-dev] [PATCH v3 0/4] et/virtio: Fix packet segmentation bug

[dpdk-dev] [PATCH v3 0/4] et/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.

v3:
- Fix properly haed segment's data_len (David)
v2:
- Fix head segment's data_len (Yaroslav)

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 | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

-- 
2.21.0

[dpdk-dev] [PATCH v3 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>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 1de28540cd..450023eaac 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,
@@ -1553,7 +1551,6 @@ virtio_recv_pkts_inorder(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

[dpdk-dev] [PATCH v3 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>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 450023eaac..acee4fcc07 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1613,7 +1613,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];
@@ -1700,7 +1700,6 @@ virtio_recv_mergeable_pkts(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;
@@ -1720,7 +1719,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,
@@ -1737,7 +1735,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

[dpdk-dev] [PATCH v3 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>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/virtio/virtio_rxtx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index acee4fcc07..358cc86627 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1875,7 +1875,6 @@ virtio_recv_mergeable_pkts_packed(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;
@@ -1912,7 +1911,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-dev] [PATCH v3 3/4] net/virtio: fix segment data len in mergeable packed Rx path

[Jens Freimann]

On Wed, Jun 05, 2019 at 12:00:38PM +0200, Maxime Coquelin 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.

s/of of/of/

Reviewed-by: Jens Freimann <jfreimann@redhat.com> 

regards,
Jens 

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

[Maxime Coquelin]

On 6/5/19 1:16 PM, Jens Freimann wrote:
> On Wed, Jun 05, 2019 at 12:00:38PM +0200, Maxime Coquelin 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.
> 
> s/of of/of/

Thanks, will fix while applying

> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> regards,
> Jens

[dpdk-dev] [PATCH v3 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.

Reviewed-by: David Marchand <david.marchand@redhat.com>
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 358cc86627..ca40ba9c5d 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;
 		}
@@ -1701,9 +1699,7 @@ virtio_recv_mergeable_pkts(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;
 		}
@@ -1876,9 +1872,7 @@ virtio_recv_mergeable_pkts_packed(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;
 		}
@@ -1921,8 +1915,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-dev] [PATCH v3 4/4] net/virtio: remove useless pointers checks

[Ye Xiaolong]

On 06/05, Maxime Coquelin wrote:
>This patch removes uses checks on 'prev' pointer, as it

s/uses/useless/

>is always set before with a valid value.
>
>Reviewed-by: David Marchand <david.marchand@redhat.com>
>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 358cc86627..ca40ba9c5d 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;
> 		}
>@@ -1701,9 +1699,7 @@ virtio_recv_mergeable_pkts(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;
> 		}
>@@ -1876,9 +1872,7 @@ virtio_recv_mergeable_pkts_packed(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;
> 		}
>@@ -1921,8 +1915,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-dev] [PATCH v3 4/4] net/virtio: remove useless pointers checks

[Maxime Coquelin]

On 6/6/19 9:15 AM, Ye Xiaolong wrote:
> On 06/05, Maxime Coquelin wrote:
>> This patch removes uses checks on 'prev' pointer, as it
> 
> s/uses/useless/

Thanks, will fix while applying.

Maxime
>> is always set before with a valid value.
>>
>> Reviewed-by: David Marchand <david.marchand@redhat.com>
>> 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 358cc86627..ca40ba9c5d 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;
>> 		}
>> @@ -1701,9 +1699,7 @@ virtio_recv_mergeable_pkts(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;
>> 		}
>> @@ -1876,9 +1872,7 @@ virtio_recv_mergeable_pkts_packed(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;
>> 		}
>> @@ -1921,8 +1915,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-dev] [PATCH v3 0/4] et/virtio: Fix packet segmentation bug

[Tiwei Bie]

On Wed, Jun 05, 2019 at 12:00:35PM +0200, Maxime Coquelin wrote:
> 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.
> 
> v3:
> - Fix properly haed segment's data_len (David)
> v2:
> - Fix head segment's data_len (Yaroslav)
> 
> 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 | 27 ++++++---------------------
>  1 file changed, 6 insertions(+), 21 deletions(-)

For the series,
Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Thanks!

Re: [dpdk-dev] [PATCH v3 0/4] et/virtio: Fix packet segmentation bug

[Maxime Coquelin]

On 6/5/19 12:00 PM, Maxime Coquelin wrote:
> 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.
> 
> v3:
> - Fix properly haed segment's data_len (David)
> v2:
> - Fix head segment's data_len (Yaroslav)
> 
> 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 | 27 ++++++---------------------
>   1 file changed, 6 insertions(+), 21 deletions(-)
> 

Series applied to dpdk-next-virtio/master with typos in commit messages
fixed.

Thanks,
Maxime