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
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
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
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
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
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
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
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!
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 >
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 >>
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