[dpdk-dev] [PATCH v2 0/3] Fix Vhost regressions

[dpdk-dev] [PATCH v2 0/3] Fix Vhost regressions

[Maxime Coquelin]

This series aims at fixing the regressions reported by Intel QE.
I managed to reproduce the issues, and this series fixes them.

The two first patches fix the performance regression. They have
been tested by intel QE which confirms the fix.

The third patch fixes the interrupt regression. I tested it OK
with l3fwd-power use-case, but it has not been confirmed by Intel
QE yet. The fix could be further improved in the future by
introducing a dedicated API in rte_epoll library to update epoll
events.

Thanks to Intel QE team for all the validation work!
Maxime

Maxime Coquelin (3):
  vhost: fix guest notification setting
  net/vhost: fix queue update
  net/vhost: fix interrupt mode

 drivers/net/vhost/rte_eth_vhost.c | 61 ++++++++++++++++++++-----------
 lib/librte_vhost/vhost.c          | 24 ++++++++++--
 lib/librte_vhost/vhost.h          |  5 +++
 lib/librte_vhost/vhost_user.c     | 11 ++++--
 4 files changed, 72 insertions(+), 29 deletions(-)

-- 
2.26.2

[dpdk-dev] [PATCH v2 1/3] vhost: fix guest notification setting

[Maxime Coquelin]

If rte_vhost_enable_guest_notification is called before
the virtqueue is ready, the configuration is lost.

This patch fixes this by saving the guest notification
enablement value requested by the application, and apply
it before the virtqueue is made ready to the application.

Fixes: 604052ae5395 ("net/vhost: support queue update")

Reported-by: Yinan Wang <yinan.wang@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
 lib/librte_vhost/vhost.c      | 24 ++++++++++++++++++++----
 lib/librte_vhost/vhost.h      |  5 +++++
 lib/librte_vhost/vhost_user.c | 11 ++++++++---
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 14b3e253e8..8f20a0818f 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -534,6 +534,7 @@ init_vring_queue(struct virtio_net *dev, uint32_t vring_idx)
 
 	vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;
 	vq->callfd = VIRTIO_UNINITIALIZED_EVENTFD;
+	vq->notif_enable = VIRTIO_UNINITIALIZED_NOTIF;
 
 	vhost_user_iotlb_init(dev, vring_idx);
 	/* Backends are set to -1 indicating an inactive device. */
@@ -1311,6 +1312,23 @@ vhost_enable_notify_packed(struct virtio_net *dev,
 	return 0;
 }
 
+int
+vhost_enable_guest_notification(struct virtio_net *dev,
+		struct vhost_virtqueue *vq, int enable)
+{
+	/*
+	 * If the virtqueue is not ready yet, it will be applied
+	 * when it will become ready.
+	 */
+	if (!vq->ready)
+		return 0;
+
+	if (vq_is_packed(dev))
+		return vhost_enable_notify_packed(dev, vq, enable);
+	else
+		return vhost_enable_notify_split(dev, vq, enable);
+}
+
 int
 rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable)
 {
@@ -1325,10 +1343,8 @@ rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable)
 
 	rte_spinlock_lock(&vq->access_lock);
 
-	if (vq_is_packed(dev))
-		ret = vhost_enable_notify_packed(dev, vq, enable);
-	else
-		ret = vhost_enable_notify_split(dev, vq, enable);
+	vq->notif_enable = enable;
+	ret = vhost_enable_guest_notification(dev, vq, enable);
 
 	rte_spinlock_unlock(&vq->access_lock);
 
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 0f7212f888..a29c6638e2 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -164,6 +164,9 @@ struct vhost_virtqueue {
 	int			enabled;
 	int			access_ok;
 	int			ready;
+	int			notif_enable;
+#define VIRTIO_UNINITIALIZED_NOTIF	(-1)
+
 	rte_spinlock_t		access_lock;
 
 	/* Used to notify the guest (trigger interrupt) */
@@ -668,6 +671,8 @@ void vhost_enable_dequeue_zero_copy(int vid);
 void vhost_set_builtin_virtio_net(int vid, bool enable);
 void vhost_enable_extbuf(int vid);
 void vhost_enable_linearbuf(int vid);
+int vhost_enable_guest_notification(struct virtio_net *dev,
+		struct vhost_virtqueue *vq, int enable);
 
 struct vhost_device_ops const *vhost_driver_callback_get(const char *path);
 
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 9ddeae3622..c3c924faec 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -235,6 +235,11 @@ vhost_user_notify_queue_state(struct virtio_net *dev, uint16_t index,
 			      int enable)
 {
 	struct rte_vdpa_device *vdpa_dev = dev->vdpa_dev;
+	struct vhost_virtqueue *vq = dev->virtqueue[index];
+
+	/* Configure guest notifications on enable */
+	if (enable && vq->notif_enable != VIRTIO_UNINITIALIZED_NOTIF)
+		vhost_enable_guest_notification(dev, vq, vq->notif_enable);
 
 	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
 		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
@@ -1640,8 +1645,8 @@ vhost_user_set_vring_call(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	vq = dev->virtqueue[file.index];
 
 	if (vq->ready) {
-		vhost_user_notify_queue_state(dev, file.index, 0);
 		vq->ready = 0;
+		vhost_user_notify_queue_state(dev, file.index, 0);
 	}
 
 	if (vq->callfd >= 0)
@@ -1903,8 +1908,8 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	}
 
 	if (vq->ready) {
-		vhost_user_notify_queue_state(dev, file.index, 0);
 		vq->ready = 0;
+		vhost_user_notify_queue_state(dev, file.index, 0);
 	}
 
 	if (vq->kickfd >= 0)
@@ -2917,8 +2922,8 @@ vhost_user_msg_handler(int vid, int fd)
 		bool cur_ready = vq_is_ready(dev, vq);
 
 		if (cur_ready != (vq && vq->ready)) {
-			vhost_user_notify_queue_state(dev, i, cur_ready);
 			vq->ready = cur_ready;
+			vhost_user_notify_queue_state(dev, i, cur_ready);
 		}
 	}
 
-- 
2.26.2

[dpdk-dev] [PATCH v2 2/3] net/vhost: fix queue update

[Maxime Coquelin]

Now that the vhost library saves the guest notifications
enablement value in its virtqueues metadata, it is not
necessary to do it in the vring_state_changed callback.

One effect of the patch is also to prevent possible
deadlock happening in vhost library.

Fixes: 604052ae5395 ("net/vhost: support queue update")

Reported-by: Yinan Wang <yinan.wang@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index bbf79b2c0e..951929c663 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -94,7 +94,6 @@ struct vhost_queue {
 	struct rte_mempool *mb_pool;
 	uint16_t port;
 	uint16_t virtqueue_id;
-	bool intr_en;
 	struct vhost_stats stats;
 };
 
@@ -547,8 +546,6 @@ eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t qid)
 	rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
 	rte_wmb();
 
-	vq->intr_en = true;
-
 	return ret;
 }
 
@@ -574,8 +571,6 @@ eth_rxq_intr_disable(struct rte_eth_dev *dev, uint16_t qid)
 	rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
 	rte_wmb();
 
-	vq->intr_en = false;
-
 	return 0;
 }
 
@@ -841,7 +836,6 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id)
 	struct rte_eth_conf *dev_conf = &eth_dev->data->dev_conf;
 	struct pmd_internal *internal = eth_dev->data->dev_private;
 	struct rte_vhost_vring vring;
-	struct vhost_queue *vq;
 	int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
 	int ret = 0;
 
@@ -853,21 +847,17 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id)
 	    rte_atomic32_read(&internal->dev_attached) &&
 	    rte_atomic32_read(&internal->started) &&
 	    dev_conf->intr_conf.rxq) {
-		vq = eth_dev->data->rx_queues[rx_idx];
 		ret = rte_vhost_get_vhost_vring(vid, vring_id, &vring);
-		if (!ret) {
-			if (vring.kickfd !=
-			    eth_dev->intr_handle->efds[rx_idx]) {
-				VHOST_LOG(INFO,
-					  "kickfd for rxq-%d was changed.\n",
-					  rx_idx);
-				eth_dev->intr_handle->efds[rx_idx] =
-								   vring.kickfd;
-			}
+		if (ret) {
+			VHOST_LOG(ERR, "Failed to get vring %d information.\n",
+					vring_id);
+			return ret;
+		}
 
-			rte_vhost_enable_guest_notification(vid, vring_id,
-							    vq->intr_en);
-			rte_wmb();
+		if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
+			VHOST_LOG(INFO, "kickfd for rxq-%d was changed.\n",
+					  rx_idx);
+			eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
 		}
 	}
 
-- 
2.26.2

Re: [dpdk-dev] [PATCH v2 2/3] net/vhost: fix queue update

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, July 29, 2020 12:50 AM
> To: dev@dpdk.org; matan@mellanox.com; Xia, Chenbo
> <chenbo.xia@intel.com>; Liu, Yong <yong.liu@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>
> Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> david.marchand@redhat.com; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Subject: [PATCH v2 2/3] net/vhost: fix queue update
> 
> Now that the vhost library saves the guest notifications enablement value in its
> virtqueues metadata, it is not necessary to do it in the vring_state_changed
> callback.
> 
> One effect of the patch is also to prevent possible deadlock happening in vhost
> library.
> 
> Fixes: 604052ae5395 ("net/vhost: support queue update")
> 
> Reported-by: Yinan Wang <yinan.wang@intel.com>
> Tested-by: Yinan Wang <yinan.wang@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 28 +++++++++-------------------
>  1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index bbf79b2c0e..951929c663 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -94,7 +94,6 @@ struct vhost_queue {
>  	struct rte_mempool *mb_pool;
>  	uint16_t port;
>  	uint16_t virtqueue_id;
> -	bool intr_en;
>  	struct vhost_stats stats;
>  };
> 
> @@ -547,8 +546,6 @@ eth_rxq_intr_enable(struct rte_eth_dev *dev, uint16_t
> qid)
>  	rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
>  	rte_wmb();
> 
> -	vq->intr_en = true;
> -
>  	return ret;
>  }
> 
> @@ -574,8 +571,6 @@ eth_rxq_intr_disable(struct rte_eth_dev *dev, uint16_t
> qid)
>  	rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
>  	rte_wmb();
> 
> -	vq->intr_en = false;
> -
>  	return 0;
>  }
> 
> @@ -841,7 +836,6 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev,
> uint16_t vring_id)
>  	struct rte_eth_conf *dev_conf = &eth_dev->data->dev_conf;
>  	struct pmd_internal *internal = eth_dev->data->dev_private;
>  	struct rte_vhost_vring vring;
> -	struct vhost_queue *vq;
>  	int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
>  	int ret = 0;
> 
> @@ -853,21 +847,17 @@ vring_conf_update(int vid, struct rte_eth_dev
> *eth_dev, uint16_t vring_id)
>  	    rte_atomic32_read(&internal->dev_attached) &&
>  	    rte_atomic32_read(&internal->started) &&
>  	    dev_conf->intr_conf.rxq) {
> -		vq = eth_dev->data->rx_queues[rx_idx];
>  		ret = rte_vhost_get_vhost_vring(vid, vring_id, &vring);
> -		if (!ret) {
> -			if (vring.kickfd !=
> -			    eth_dev->intr_handle->efds[rx_idx]) {
> -				VHOST_LOG(INFO,
> -					  "kickfd for rxq-%d was changed.\n",
> -					  rx_idx);
> -				eth_dev->intr_handle->efds[rx_idx] =
> -								   vring.kickfd;
> -			}
> +		if (ret) {
> +			VHOST_LOG(ERR, "Failed to get vring %d
> information.\n",
> +					vring_id);
> +			return ret;
> +		}
> 
> -			rte_vhost_enable_guest_notification(vid, vring_id,
> -							    vq->intr_en);
> -			rte_wmb();
> +		if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
> +			VHOST_LOG(INFO, "kickfd for rxq-%d was changed.\n",
> +					  rx_idx);
> +			eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
>  		}
>  	}
> 
> --
> 2.26.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

Re: [dpdk-dev] [PATCH v2 2/3] net/vhost: fix queue update

[Wang, Yinan]

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: 2020?7?29? 10:53
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org;
> matan@mellanox.com; Liu, Yong <yong.liu@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>
> Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> david.marchand@redhat.com
> Subject: RE: [PATCH v2 2/3] net/vhost: fix queue update
> 
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Wednesday, July 29, 2020 12:50 AM
> > To: dev@dpdk.org; matan@mellanox.com; Xia, Chenbo
> > <chenbo.xia@intel.com>; Liu, Yong <yong.liu@intel.com>; Wang, Yinan
> > <yinan.wang@intel.com>
> > Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> > david.marchand@redhat.com; Maxime Coquelin
> > <maxime.coquelin@redhat.com>
> > Subject: [PATCH v2 2/3] net/vhost: fix queue update
> >
> > Now that the vhost library saves the guest notifications enablement value
> in its
> > virtqueues metadata, it is not necessary to do it in the vring_state_changed
> > callback.
> >
> > One effect of the patch is also to prevent possible deadlock happening in
> vhost
> > library.
> >
> > Fixes: 604052ae5395 ("net/vhost: support queue update")
> >
> > Reported-by: Yinan Wang <yinan.wang@intel.com>
> > Tested-by: Yinan Wang <yinan.wang@intel.com>
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> >  drivers/net/vhost/rte_eth_vhost.c | 28 +++++++++-------------------
> >  1 file changed, 9 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/vhost/rte_eth_vhost.c
> > b/drivers/net/vhost/rte_eth_vhost.c
> > index bbf79b2c0e..951929c663 100644
> > --- a/drivers/net/vhost/rte_eth_vhost.c
> > +++ b/drivers/net/vhost/rte_eth_vhost.c
> > @@ -94,7 +94,6 @@ struct vhost_queue {
> >  	struct rte_mempool *mb_pool;
> >  	uint16_t port;
> >  	uint16_t virtqueue_id;
> > -	bool intr_en;
> >  	struct vhost_stats stats;
> >  };
> >
> > @@ -547,8 +546,6 @@ eth_rxq_intr_enable(struct rte_eth_dev *dev,
> uint16_t
> > qid)
> >  	rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 1);
> >  	rte_wmb();
> >
> > -	vq->intr_en = true;
> > -
> >  	return ret;
> >  }
> >
> > @@ -574,8 +571,6 @@ eth_rxq_intr_disable(struct rte_eth_dev *dev,
> uint16_t
> > qid)
> >  	rte_vhost_enable_guest_notification(vq->vid, (qid << 1) + 1, 0);
> >  	rte_wmb();
> >
> > -	vq->intr_en = false;
> > -
> >  	return 0;
> >  }
> >
> > @@ -841,7 +836,6 @@ vring_conf_update(int vid, struct rte_eth_dev
> *eth_dev,
> > uint16_t vring_id)
> >  	struct rte_eth_conf *dev_conf = &eth_dev->data->dev_conf;
> >  	struct pmd_internal *internal = eth_dev->data->dev_private;
> >  	struct rte_vhost_vring vring;
> > -	struct vhost_queue *vq;
> >  	int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
> >  	int ret = 0;
> >
> > @@ -853,21 +847,17 @@ vring_conf_update(int vid, struct rte_eth_dev
> > *eth_dev, uint16_t vring_id)
> >  	    rte_atomic32_read(&internal->dev_attached) &&
> >  	    rte_atomic32_read(&internal->started) &&
> >  	    dev_conf->intr_conf.rxq) {
> > -		vq = eth_dev->data->rx_queues[rx_idx];
> >  		ret = rte_vhost_get_vhost_vring(vid, vring_id, &vring);
> > -		if (!ret) {
> > -			if (vring.kickfd !=
> > -			    eth_dev->intr_handle->efds[rx_idx]) {
> > -				VHOST_LOG(INFO,
> > -					  "kickfd for rxq-%d was changed.\n",
> > -					  rx_idx);
> > -				eth_dev->intr_handle->efds[rx_idx] =
> > -								   vring.kickfd;
> > -			}
> > +		if (ret) {
> > +			VHOST_LOG(ERR, "Failed to get vring %d
> > information.\n",
> > +					vring_id);
> > +			return ret;
> > +		}
> >
> > -			rte_vhost_enable_guest_notification(vid, vring_id,
> > -							    vq->intr_en);
> > -			rte_wmb();
> > +		if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
> > +			VHOST_LOG(INFO, "kickfd for rxq-%d was
> changed.\n",
> > +					  rx_idx);
> > +			eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
> >  		}
> >  	}
> >
> > --
> > 2.26.2
> 
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
Tested-by: Yinan Wang <yinan.wang@intel.com>

[dpdk-dev] [PATCH v2 3/3] net/vhost: fix interrupt mode

[Maxime Coquelin]

At .new_device() time, only the first vring pair is
now ready, other vrings are consfigured later.

Problem is that when application will setup and enable
interrupts, only the first queue pair Rx interrupt will
be enabled.

This patches fixes the issue by setting the number of
max interrupts to the number of Rx queues that will be
later initialized. Then, as soon as a Rx vring is ready,
it removes the corresponding uninitialized epoll event,
and install a new one with the valid FD.

Fixes: 604052ae5395 ("net/vhost: support queue update")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 39 ++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 951929c663..0f6a0bbbf7 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -5,6 +5,7 @@
 #include <unistd.h>
 #include <pthread.h>
 #include <stdbool.h>
+#include <sys/epoll.h>
 
 #include <rte_mbuf.h>
 #include <rte_ethdev_driver.h>
@@ -593,7 +594,6 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
 {
 	struct rte_vhost_vring vring;
 	struct vhost_queue *vq;
-	int count = 0;
 	int nb_rxq = dev->data->nb_rx_queues;
 	int i;
 	int ret;
@@ -623,6 +623,8 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
 
 	VHOST_LOG(INFO, "Prepare intr vec\n");
 	for (i = 0; i < nb_rxq; i++) {
+		dev->intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + i;
+		dev->intr_handle->efds[i] = -1;
 		vq = dev->data->rx_queues[i];
 		if (!vq) {
 			VHOST_LOG(INFO, "rxq-%d not setup yet, skip!\n", i);
@@ -641,14 +643,12 @@ eth_vhost_install_intr(struct rte_eth_dev *dev)
 				"rxq-%d's kickfd is invalid, skip!\n", i);
 			continue;
 		}
-		dev->intr_handle->intr_vec[i] = RTE_INTR_VEC_RXTX_OFFSET + i;
 		dev->intr_handle->efds[i] = vring.kickfd;
-		count++;
 		VHOST_LOG(INFO, "Installed intr vec for rxq-%d\n", i);
 	}
 
-	dev->intr_handle->nb_efd = count;
-	dev->intr_handle->max_intr = count + 1;
+	dev->intr_handle->nb_efd = nb_rxq;
+	dev->intr_handle->max_intr = nb_rxq + 1;
 	dev->intr_handle->type = RTE_INTR_HANDLE_VDEV;
 
 	return 0;
@@ -836,8 +836,11 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id)
 	struct rte_eth_conf *dev_conf = &eth_dev->data->dev_conf;
 	struct pmd_internal *internal = eth_dev->data->dev_private;
 	struct rte_vhost_vring vring;
+	struct rte_intr_handle *handle;
+	struct rte_epoll_event rev;
 	int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
 	int ret = 0;
+	int epfd;
 
 	/*
 	 * The vring kickfd may be changed after the new device notification.
@@ -857,7 +860,31 @@ vring_conf_update(int vid, struct rte_eth_dev *eth_dev, uint16_t vring_id)
 		if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
 			VHOST_LOG(INFO, "kickfd for rxq-%d was changed.\n",
 					  rx_idx);
-			eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
+
+			/*
+			 * First remove invalid epoll event, and then isntall
+			 * the new one. May be solved with a proper API in the
+			 * future.
+			 */
+			handle = eth_dev->intr_handle;
+			handle->efds[rx_idx] = vring.kickfd;
+			epfd = handle->elist[rx_idx].epfd;
+			rev = handle->elist[rx_idx];
+			ret = rte_epoll_ctl(epfd, EPOLL_CTL_DEL, rev.fd,
+					&handle->elist[rx_idx]);
+			if (ret) {
+				VHOST_LOG(ERR, "Delete epoll event failed.\n");
+				return ret;
+			}
+
+			rev.fd = vring.kickfd;
+			handle->elist[rx_idx] = rev;
+			ret = rte_epoll_ctl(epfd, EPOLL_CTL_ADD, rev.fd,
+					&handle->elist[rx_idx]);
+			if (ret) {
+				VHOST_LOG(ERR, "Add epoll event failed.\n");
+				return ret;
+			}
 		}
 	}
 
-- 
2.26.2

Re: [dpdk-dev] [PATCH v2 3/3] net/vhost: fix interrupt mode

[Matan Azrad]

From: Maxime Coquelin:
> At .new_device() time, only the first vring pair is now ready, other vrings are
> consfigured later.
> 
> Problem is that when application will setup and enable interrupts, only the
> first queue pair Rx interrupt will be enabled.
> 
> This patches fixes the issue by setting the number of max interrupts to the
> number of Rx queues that will be later initialized. Then, as soon as a Rx vring
> is ready, it removes the corresponding uninitialized epoll event, and install a
> new one with the valid FD.

Doesn't it race condition to the application decision?
App may change the configuration per queue in any time by the app control thread.
The vhost PMD may change it usynchronically from the vhost control thread in the vring state callback.

I already mentioned it in other thread on this topic but didn't get reply.

> Fixes: 604052ae5395 ("net/vhost: support queue update")
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Re: [dpdk-dev] [PATCH v2 3/3] net/vhost: fix interrupt mode

[Maxime Coquelin]

On 7/28/20 9:03 PM, Matan Azrad wrote:
> 
> 
> From: Maxime Coquelin:
>> At .new_device() time, only the first vring pair is now ready, other vrings are
>> consfigured later.
>>
>> Problem is that when application will setup and enable interrupts, only the
>> first queue pair Rx interrupt will be enabled.
>>
>> This patches fixes the issue by setting the number of max interrupts to the
>> number of Rx queues that will be later initialized. Then, as soon as a Rx vring
>> is ready, it removes the corresponding uninitialized epoll event, and install a
>> new one with the valid FD.
> 
> Doesn't it race condition to the application decision?
> App may change the configuration per queue in any time by the app control thread.
> The vhost PMD may change it usynchronically from the vhost control thread in the vring state callback.

Yes you are right there could be a race here,I'm looking into getting it
done in a safe way. Yet it is good to get the confirmation from Intel
that it does fix the problem on their side.

Based on David suggestion, it might be made safe by relying on
eth_rxq_intr_enable()/eth_rxq_intr_disable().

If we cannot solve it in a safe way, then we'll have no other choice
than reverting partially your patch.

Maxime

> I already mentioned it in other thread on this topic but didn't get reply.
> 
>> Fixes: 604052ae5395 ("net/vhost: support queue update")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 

Re: [dpdk-dev] [PATCH v2 3/3] net/vhost: fix interrupt mode

[Matan Azrad]

Hi Maxime

From: Maxime Coquelin:
> On 7/28/20 9:03 PM, Matan Azrad wrote:
> >
> >
> > From: Maxime Coquelin:
> >> At .new_device() time, only the first vring pair is now ready, other
> >> vrings are consfigured later.
> >>
> >> Problem is that when application will setup and enable interrupts,
> >> only the first queue pair Rx interrupt will be enabled.
> >>
> >> This patches fixes the issue by setting the number of max interrupts
> >> to the number of Rx queues that will be later initialized. Then, as
> >> soon as a Rx vring is ready, it removes the corresponding
> >> uninitialized epoll event, and install a new one with the valid FD.
> >
> > Doesn't it race condition to the application decision?
> > App may change the configuration per queue in any time by the app
> control thread.
> > The vhost PMD may change it usynchronically from the vhost control
> thread in the vring state callback.
> 
> Yes you are right there could be a race here,I'm looking into getting it done in
> a safe way. Yet it is good to get the confirmation from Intel that it does fix the
> problem on their side.

Intel case\l3fw-power is only one case, we can put out the fire now by solving specific case but we need a fix for the global usage.

> Based on David suggestion, it might be made safe by relying on
> eth_rxq_intr_enable()/eth_rxq_intr_disable().

Yes, maybe.

One more option to adjust the vhost PMD is to do the new_device callback logic when the last queue is ready as was done before,
By this way, the vhost PMD will use the same timing as before.

> If we cannot solve it in a safe way, then we'll have no other choice than
> reverting partially your patch.

I'm sure we can find a solution.

As you probably remember we did the design for the readiness series together (a long discussion in other thread),
It came to solve an architecture issue in QEMU last versions which might affect vhost lib users in other cases.
We took it into account that some vhost lib users should do adjustment for the new behavior.


Matan 

> 
> Maxime
> 
> > I already mentioned it in other thread on this topic but didn't get reply.
> >
> >> Fixes: 604052ae5395 ("net/vhost: support queue update")
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >

Re: [dpdk-dev] [PATCH v2 3/3] net/vhost: fix interrupt mode

[Maxime Coquelin]

On 7/29/20 10:43 AM, Matan Azrad wrote:
> Hi Maxime
> 
> From: Maxime Coquelin:
>> On 7/28/20 9:03 PM, Matan Azrad wrote:
>>>
>>>
>>> From: Maxime Coquelin:
>>>> At .new_device() time, only the first vring pair is now ready, other
>>>> vrings are consfigured later.
>>>>
>>>> Problem is that when application will setup and enable interrupts,
>>>> only the first queue pair Rx interrupt will be enabled.
>>>>
>>>> This patches fixes the issue by setting the number of max interrupts
>>>> to the number of Rx queues that will be later initialized. Then, as
>>>> soon as a Rx vring is ready, it removes the corresponding
>>>> uninitialized epoll event, and install a new one with the valid FD.
>>>
>>> Doesn't it race condition to the application decision?
>>> App may change the configuration per queue in any time by the app
>> control thread.
>>> The vhost PMD may change it usynchronically from the vhost control
>> thread in the vring state callback.
>>
>> Yes you are right there could be a race here,I'm looking into getting it done in
>> a safe way. Yet it is good to get the confirmation from Intel that it does fix the
>> problem on their side.
> 
> Intel case\l3fw-power is only one case, we can put out the fire now by solving specific case but we need a fix for the global usage.
> 
>> Based on David suggestion, it might be made safe by relying on
>> eth_rxq_intr_enable()/eth_rxq_intr_disable().
> 
> Yes, maybe.
> 
> One more option to adjust the vhost PMD is to do the new_device callback logic when the last queue is ready as was done before,
> By this way, the vhost PMD will use the same timing as before.

Yes, that's one option but that would not be ideal because we would
lose the benefits of what the initial series brings, and it is not a
light change that late in the release.

Trying to fix interrupts, we mainly risk to break interrupt support
which is not widely used. Trying to wait for all queue event to be
received may cause other regressions in all use-cases.

>> If we cannot solve it in a safe way, then we'll have no other choice than
>> reverting partially your patch.
> 
> I'm sure we can find a solution.
> 
> As you probably remember we did the design for the readiness series together (a long discussion in other thread),
> It came to solve an architecture issue in QEMU last versions which might affect vhost lib users in other cases.
> We took it into account that some vhost lib users should do adjustment for the new behavior.
> 
> 
> Matan 
> 
>>
>> Maxime
>>
>>> I already mentioned it in other thread on this topic but didn't get reply.
>>>
>>>> Fixes: 604052ae5395 ("net/vhost: support queue update")
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
> 

Re: [dpdk-dev] [PATCH v2 0/3] Fix Vhost regressions

[Wang, Yinan]

This patch can fix multi-queue performance drop issue and interrupt issue.Thanks!

BR,
Yinan

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2020?7?29? 0:50
> To: dev@dpdk.org; matan@mellanox.com; Xia, Chenbo
> <chenbo.xia@intel.com>; Liu, Yong <yong.liu@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>
> Cc: thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>;
> david.marchand@redhat.com; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> Subject: [PATCH v2 0/3] Fix Vhost regressions
> 
> This series aims at fixing the regressions reported by Intel QE.
> I managed to reproduce the issues, and this series fixes them.
> 
> The two first patches fix the performance regression. They have
> been tested by intel QE which confirms the fix.
> 
> The third patch fixes the interrupt regression. I tested it OK
> with l3fwd-power use-case, but it has not been confirmed by Intel
> QE yet. The fix could be further improved in the future by
> introducing a dedicated API in rte_epoll library to update epoll
> events.
> 
> Thanks to Intel QE team for all the validation work!
> Maxime
> 
> Maxime Coquelin (3):
>   vhost: fix guest notification setting
>   net/vhost: fix queue update
>   net/vhost: fix interrupt mode
> 
>  drivers/net/vhost/rte_eth_vhost.c | 61 ++++++++++++++++++++-----------
>  lib/librte_vhost/vhost.c          | 24 ++++++++++--
>  lib/librte_vhost/vhost.h          |  5 +++
>  lib/librte_vhost/vhost_user.c     | 11 ++++--
>  4 files changed, 72 insertions(+), 29 deletions(-)
> 
> --
> 2.26.2