On Fri, Feb 09, 2018 at 03:26:53PM +0100, Maxime Coquelin wrote: > This patch fixes traffic resuming issue seen when using > Rx vector path. > > Fixes: efc83a1e7fc3 ("net/virtio: fix queue setup consistency") > > Signed-off-by: Tiwei Bie <tiwei....@intel.com> > Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
Proposed change for a more detailed commit log (thanks Tiwei for the explanation): Since commit efc83a1e7fc3 ("net/virtio: fix queue setup consistency"), when resuming a virtio port, the rx rings are refilled with new mbufs until they are full (vq->vq_free_cnt == 0). This is done without ensuring that the descriptor index remains a multiple of RTE_VIRTIO_VPMD_RX_REARM_THRESH, which is a prerequisite when using the vector mode. This can cause an out of bound access in the rx ring. This commit changes the vector refill method from virtqueue_enqueue_recv_refill_simple() to virtio_rxq_rearm_vec(), which properly checks that the refill is done by batch of RTE_VIRTIO_VPMD_RX_REARM_THRESH. > --- > drivers/net/virtio/virtio_rxtx.c | 34 > ++++++++++++++++++--------------- > drivers/net/virtio/virtio_rxtx_simple.c | 2 +- > drivers/net/virtio/virtio_rxtx_simple.h | 2 +- > 3 files changed, 21 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/virtio/virtio_rxtx.c > b/drivers/net/virtio/virtio_rxtx.c > index 854af399e..505283edd 100644 > --- a/drivers/net/virtio/virtio_rxtx.c > +++ b/drivers/net/virtio/virtio_rxtx.c > @@ -30,6 +30,7 @@ > #include "virtio_pci.h" > #include "virtqueue.h" > #include "virtio_rxtx.h" > +#include "virtio_rxtx_simple.h" > > #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP > #define VIRTIO_DUMP_PACKET(m, len) rte_pktmbuf_dump(stdout, m, len) > @@ -446,25 +447,28 @@ virtio_dev_rx_queue_setup_finish(struct rte_eth_dev > *dev, uint16_t queue_idx) > &rxvq->fake_mbuf; > } > > - while (!virtqueue_full(vq)) { > - m = rte_mbuf_raw_alloc(rxvq->mpool); > - if (m == NULL) > - break; > + if (hw->use_simple_rx) { > + while (vq->vq_free_cnt >= RTE_VIRTIO_VPMD_RX_REARM_THRESH) { > + virtio_rxq_rearm_vec(rxvq); > + nbufs += RTE_VIRTIO_VPMD_RX_REARM_THRESH; > + } > + } else { > + while (!virtqueue_full(vq)) { > + m = rte_mbuf_raw_alloc(rxvq->mpool); > + if (m == NULL) > + break; > > - /* Enqueue allocated buffers */ > - if (hw->use_simple_rx) > - error = virtqueue_enqueue_recv_refill_simple(vq, m); > - else It looks that virtqueue_enqueue_recv_refill_simple() is not used anymore. We may want to remove it. It means this patch also fixes the issue related to the patch I've just submitted [1]. To ease backport process, I think the patch [1] is still relevant because it fixes an older commit. [1] http://dpdk.org/dev/patchwork/patch/35130/ > + /* Enqueue allocated buffers */ > error = virtqueue_enqueue_recv_refill(vq, m); > - > - if (error) { > - rte_pktmbuf_free(m); > - break; > + if (error) { > + rte_pktmbuf_free(m); > + break; > + } > + nbufs++; > } > - nbufs++; > - } > > - vq_update_avail_idx(vq); > + vq_update_avail_idx(vq); > + } > > PMD_INIT_LOG(DEBUG, "Allocated %d bufs", nbufs); > > virtio_rxq_vec_setup(rxvq); Another thing seen by Tiwei: virtio_rxq_vec_setup() should be called before virtio_rxq_rearm_vec() because it initializes the mbuf initializer. Thanks, Olivier