* [dpdk-stable] [PATCH 0/2] Vhost & Virtio fixes for -rc4 @ 2018-02-09 14:26 Maxime Coquelin 2018-02-09 14:26 ` [dpdk-stable] [PATCH 1/2] virtio: fix resuming traffic with rx vector path Maxime Coquelin 2018-02-09 14:26 ` [dpdk-stable] [PATCH 2/2] vhost: don't take access_lock on VHOST_USER_RESET_OWNER Maxime Coquelin 0 siblings, 2 replies; 6+ messages in thread From: Maxime Coquelin @ 2018-02-09 14:26 UTC (permalink / raw) To: tiwei.bie, yliu, ferruh.yigit, victork Cc: dev, stable, zhihong.wang, qian.q.xu, lei.a.yao, Maxime Coquelin This two patches series fixes two regressions met with virtio-user. The first one is only reproduced when using the Vector path, the second one may be met whatever the rx function. Maxime Coquelin (2): virtio: fix resuming traffic with rx vector path vhost: don't take access_lock on VHOST_USER_RESET_OWNER drivers/net/virtio/virtio_rxtx.c | 34 ++++++++++++++++++--------------- drivers/net/virtio/virtio_rxtx_simple.c | 2 +- drivers/net/virtio/virtio_rxtx_simple.h | 2 +- lib/librte_vhost/vhost_user.c | 10 +++++----- 4 files changed, 26 insertions(+), 22 deletions(-) -- 2.14.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-stable] [PATCH 1/2] virtio: fix resuming traffic with rx vector path 2018-02-09 14:26 [dpdk-stable] [PATCH 0/2] Vhost & Virtio fixes for -rc4 Maxime Coquelin @ 2018-02-09 14:26 ` Maxime Coquelin 2018-02-11 8:02 ` Yao, Lei A 2018-02-12 13:18 ` [dpdk-stable] [dpdk-dev] " Olivier Matz 2018-02-09 14:26 ` [dpdk-stable] [PATCH 2/2] vhost: don't take access_lock on VHOST_USER_RESET_OWNER Maxime Coquelin 1 sibling, 2 replies; 6+ messages in thread From: Maxime Coquelin @ 2018-02-09 14:26 UTC (permalink / raw) To: tiwei.bie, yliu, ferruh.yigit, victork Cc: dev, stable, zhihong.wang, qian.q.xu, lei.a.yao, Maxime Coquelin 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.bie@intel.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- 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 + /* 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); diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c index 7247a0822..0a79d1d5b 100644 --- a/drivers/net/virtio/virtio_rxtx_simple.c +++ b/drivers/net/virtio/virtio_rxtx_simple.c @@ -77,7 +77,7 @@ virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts, rte_compiler_barrier(); if (nb_used >= VIRTIO_TX_FREE_THRESH) - virtio_xmit_cleanup(vq); + virtio_xmit_cleanup_simple(vq); nb_commit = nb_pkts = RTE_MIN((vq->vq_free_cnt >> 1), nb_pkts); desc_idx = (uint16_t)(vq->vq_avail_idx & desc_idx_max); diff --git a/drivers/net/virtio/virtio_rxtx_simple.h b/drivers/net/virtio/virtio_rxtx_simple.h index 2d8e6b14a..303904d64 100644 --- a/drivers/net/virtio/virtio_rxtx_simple.h +++ b/drivers/net/virtio/virtio_rxtx_simple.h @@ -60,7 +60,7 @@ virtio_rxq_rearm_vec(struct virtnet_rx *rxvq) #define VIRTIO_TX_FREE_NR 32 /* TODO: vq->tx_free_cnt could mean num of free slots so we could avoid shift */ static inline void -virtio_xmit_cleanup(struct virtqueue *vq) +virtio_xmit_cleanup_simple(struct virtqueue *vq) { uint16_t i, desc_idx; uint32_t nb_free = 0; -- 2.14.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH 1/2] virtio: fix resuming traffic with rx vector path 2018-02-09 14:26 ` [dpdk-stable] [PATCH 1/2] virtio: fix resuming traffic with rx vector path Maxime Coquelin @ 2018-02-11 8:02 ` Yao, Lei A 2018-02-12 13:18 ` [dpdk-stable] [dpdk-dev] " Olivier Matz 1 sibling, 0 replies; 6+ messages in thread From: Yao, Lei A @ 2018-02-11 8:02 UTC (permalink / raw) To: Maxime Coquelin, Bie, Tiwei, yliu, Yigit, Ferruh, victork, Thomas Monjalon Cc: dev, stable, Wang, Zhihong, Xu, Qian Q > -----Original Message----- > From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] > Sent: Friday, February 9, 2018 10:27 PM > To: Bie, Tiwei <tiwei.bie@intel.com>; yliu@fridaylinux.org; Yigit, Ferruh > <ferruh.yigit@intel.com>; victork@redhat.com > Cc: dev@dpdk.org; stable@dpdk.org; Wang, Zhihong > <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>; Yao, Lei A > <lei.a.yao@intel.com>; Maxime Coquelin <maxime.coquelin@redhat.com> > Subject: [PATCH 1/2] virtio: fix resuming traffic with rx vector path > > 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.bie@intel.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Tested-by: Lei Yao <lei.a.yao@intel.com> This patch has been tested by regression test suite. It can fix the traffic resume issue with vector path. No performance drop during PVP test: Following test are also checked and passed: Vhost/virtio multi queue Virtio-user Virtio-user as exception path Vhost/virtio reconnect My server info: OS: Ubuntu 16.04 Kernel: 4.4.0-110 CPU: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz BR Lei > --- > 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 > + /* 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); > > diff --git a/drivers/net/virtio/virtio_rxtx_simple.c > b/drivers/net/virtio/virtio_rxtx_simple.c > index 7247a0822..0a79d1d5b 100644 > --- a/drivers/net/virtio/virtio_rxtx_simple.c > +++ b/drivers/net/virtio/virtio_rxtx_simple.c > @@ -77,7 +77,7 @@ virtio_xmit_pkts_simple(void *tx_queue, struct > rte_mbuf **tx_pkts, > rte_compiler_barrier(); > > if (nb_used >= VIRTIO_TX_FREE_THRESH) > - virtio_xmit_cleanup(vq); > + virtio_xmit_cleanup_simple(vq); > > nb_commit = nb_pkts = RTE_MIN((vq->vq_free_cnt >> 1), nb_pkts); > desc_idx = (uint16_t)(vq->vq_avail_idx & desc_idx_max); > diff --git a/drivers/net/virtio/virtio_rxtx_simple.h > b/drivers/net/virtio/virtio_rxtx_simple.h > index 2d8e6b14a..303904d64 100644 > --- a/drivers/net/virtio/virtio_rxtx_simple.h > +++ b/drivers/net/virtio/virtio_rxtx_simple.h > @@ -60,7 +60,7 @@ virtio_rxq_rearm_vec(struct virtnet_rx *rxvq) > #define VIRTIO_TX_FREE_NR 32 > /* TODO: vq->tx_free_cnt could mean num of free slots so we could avoid > shift */ > static inline void > -virtio_xmit_cleanup(struct virtqueue *vq) > +virtio_xmit_cleanup_simple(struct virtqueue *vq) > { > uint16_t i, desc_idx; > uint32_t nb_free = 0; > -- > 2.14.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [dpdk-dev] [PATCH 1/2] virtio: fix resuming traffic with rx vector path 2018-02-09 14:26 ` [dpdk-stable] [PATCH 1/2] virtio: fix resuming traffic with rx vector path Maxime Coquelin 2018-02-11 8:02 ` Yao, Lei A @ 2018-02-12 13:18 ` Olivier Matz 1 sibling, 0 replies; 6+ messages in thread From: Olivier Matz @ 2018-02-12 13:18 UTC (permalink / raw) To: Maxime Coquelin Cc: tiwei.bie, yliu, ferruh.yigit, victork, dev, stable, zhihong.wang, qian.q.xu, lei.a.yao 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.bie@intel.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [dpdk-stable] [PATCH 2/2] vhost: don't take access_lock on VHOST_USER_RESET_OWNER 2018-02-09 14:26 [dpdk-stable] [PATCH 0/2] Vhost & Virtio fixes for -rc4 Maxime Coquelin 2018-02-09 14:26 ` [dpdk-stable] [PATCH 1/2] virtio: fix resuming traffic with rx vector path Maxime Coquelin @ 2018-02-09 14:26 ` Maxime Coquelin 2018-02-12 9:40 ` Tiwei Bie 1 sibling, 1 reply; 6+ messages in thread From: Maxime Coquelin @ 2018-02-09 14:26 UTC (permalink / raw) To: tiwei.bie, yliu, ferruh.yigit, victork Cc: dev, stable, zhihong.wang, qian.q.xu, lei.a.yao, Maxime Coquelin A deadlock happens when handling VHOST_USER_RESET_OWNER request for the same reason the lock is not taken for VHOST_USER_GET_VRING_BASE. It is safe not to take the lock, as the queues are no more used by the application when the virtqueues and the device are reset. Fixes: a3688046995f ("vhost: protect active rings from async ring changes") Cc: stable@dpdk.org Cc: Victor Kaplansky <victork@redhat.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost_user.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index 65ee33919..90ed2112e 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -1348,16 +1348,16 @@ vhost_user_msg_handler(int vid, int fd) } /* - * Note: we don't lock all queues on VHOST_USER_GET_VRING_BASE, - * since it is sent when virtio stops and device is destroyed. - * destroy_device waits for queues to be inactive, so it is safe. - * Otherwise taking the access_lock would cause a dead lock. + * Note: we don't lock all queues on VHOST_USER_GET_VRING_BASE + * and VHOST_USER_RESET_OWNER, since it is sent when virtio stops + * and device is destroyed. destroy_device waits for queues to be + * inactive, so it is safe. Otherwise taking the access_lock + * would cause a dead lock. */ switch (msg.request.master) { case VHOST_USER_SET_FEATURES: case VHOST_USER_SET_PROTOCOL_FEATURES: case VHOST_USER_SET_OWNER: - case VHOST_USER_RESET_OWNER: case VHOST_USER_SET_MEM_TABLE: case VHOST_USER_SET_LOG_BASE: case VHOST_USER_SET_LOG_FD: -- 2.14.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [dpdk-stable] [PATCH 2/2] vhost: don't take access_lock on VHOST_USER_RESET_OWNER 2018-02-09 14:26 ` [dpdk-stable] [PATCH 2/2] vhost: don't take access_lock on VHOST_USER_RESET_OWNER Maxime Coquelin @ 2018-02-12 9:40 ` Tiwei Bie 0 siblings, 0 replies; 6+ messages in thread From: Tiwei Bie @ 2018-02-12 9:40 UTC (permalink / raw) To: Maxime Coquelin Cc: yliu, ferruh.yigit, victork, dev, stable, zhihong.wang, qian.q.xu, lei.a.yao On Fri, Feb 09, 2018 at 03:26:54PM +0100, Maxime Coquelin wrote: > A deadlock happens when handling VHOST_USER_RESET_OWNER request > for the same reason the lock is not taken for > VHOST_USER_GET_VRING_BASE. > > It is safe not to take the lock, as the queues are no more used > by the application when the virtqueues and the device are reset. > > Fixes: a3688046995f ("vhost: protect active rings from async ring changes") > Cc: stable@dpdk.org > > Cc: Victor Kaplansky <victork@redhat.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com> Thanks for the work! Best regards, Tiwei Bie ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-12 13:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-09 14:26 [dpdk-stable] [PATCH 0/2] Vhost & Virtio fixes for -rc4 Maxime Coquelin 2018-02-09 14:26 ` [dpdk-stable] [PATCH 1/2] virtio: fix resuming traffic with rx vector path Maxime Coquelin 2018-02-11 8:02 ` Yao, Lei A 2018-02-12 13:18 ` [dpdk-stable] [dpdk-dev] " Olivier Matz 2018-02-09 14:26 ` [dpdk-stable] [PATCH 2/2] vhost: don't take access_lock on VHOST_USER_RESET_OWNER Maxime Coquelin 2018-02-12 9:40 ` Tiwei Bie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).