From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 076081B31D; Mon, 12 Feb 2018 14:18:39 +0100 (CET) Received: from lfbn-lil-1-110-231.w90-45.abo.wanadoo.fr ([90.45.197.231] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1elE0K-0002Ji-8f; Mon, 12 Feb 2018 14:18:49 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Mon, 12 Feb 2018 14:18:27 +0100 Date: Mon, 12 Feb 2018 14:18:27 +0100 From: Olivier Matz To: Maxime Coquelin Cc: tiwei.bie@intel.com, yliu@fridaylinux.org, ferruh.yigit@intel.com, victork@redhat.com, dev@dpdk.org, stable@dpdk.org, zhihong.wang@intel.com, qian.q.xu@intel.com, lei.a.yao@intel.com Message-ID: <20180212131827.ko5crddn2haihzac@platinum> References: <20180209142654.29409-1-maxime.coquelin@redhat.com> <20180209142654.29409-2-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180209142654.29409-2-maxime.coquelin@redhat.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH 1/2] virtio: fix resuming traffic with rx vector path X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Feb 2018 13:18:39 -0000 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 > Signed-off-by: Maxime Coquelin 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