DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/2] Fix vhost performance regression
@ 2020-07-23 13:08 Maxime Coquelin
  2020-07-23 13:08 ` [dpdk-dev] [PATCH 1/2] vhost: fix guest notification setting Maxime Coquelin
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Maxime Coquelin @ 2020-07-23 13:08 UTC (permalink / raw)
  To: dev, matan, chenbo.xia, yong.liu, yinan.wang
  Cc: thomas, ferruh.yigit, david.marchand, Maxime Coquelin

Hi,

This series aims at fixing the performance degradation reported
by Intel QE. I managed to reproduce the issue, and this series
fixes it.

I only tested the first test case provided in the Bz[0], but wanted
to send early for Intel QE to try and confirm it solves the issue.

I will work on reproducing the other test cases, and see if this
also fixes them.

Thanks to Intel QE team for finding this issue.
Maxime

[0]: https://bugs.dpdk.org/show_bug.cgi?id=507#c0

Maxime Coquelin (2):
  vhost: fix guest notification setting
  net/vhost: fix queue update

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

-- 
2.26.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [PATCH 1/2] vhost: fix guest notification setting
  2020-07-23 13:08 [dpdk-dev] [PATCH 0/2] Fix vhost performance regression Maxime Coquelin
@ 2020-07-23 13:08 ` Maxime Coquelin
  2020-07-27 13:46   ` Xia, Chenbo
  2020-07-23 13:08 ` [dpdk-dev] [PATCH 2/2] net/vhost: fix queue update Maxime Coquelin
  2020-07-24  4:55 ` [dpdk-dev] [PATCH 0/2] Fix vhost performance regression Wang, Yinan
  2 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2020-07-23 13:08 UTC (permalink / raw)
  To: dev, matan, chenbo.xia, yong.liu, yinan.wang
  Cc: thomas, ferruh.yigit, david.marchand, 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>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [PATCH 2/2] net/vhost: fix queue update
  2020-07-23 13:08 [dpdk-dev] [PATCH 0/2] Fix vhost performance regression Maxime Coquelin
  2020-07-23 13:08 ` [dpdk-dev] [PATCH 1/2] vhost: fix guest notification setting Maxime Coquelin
@ 2020-07-23 13:08 ` Maxime Coquelin
  2020-07-27 13:54   ` Xia, Chenbo
  2020-07-24  4:55 ` [dpdk-dev] [PATCH 0/2] Fix vhost performance regression Wang, Yinan
  2 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2020-07-23 13:08 UTC (permalink / raw)
  To: dev, matan, chenbo.xia, yong.liu, yinan.wang
  Cc: thomas, ferruh.yigit, david.marchand, 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>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index bbf79b2c0e..14b7b59f67 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,14 @@ 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)
+			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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix vhost performance regression
  2020-07-23 13:08 [dpdk-dev] [PATCH 0/2] Fix vhost performance regression Maxime Coquelin
  2020-07-23 13:08 ` [dpdk-dev] [PATCH 1/2] vhost: fix guest notification setting Maxime Coquelin
  2020-07-23 13:08 ` [dpdk-dev] [PATCH 2/2] net/vhost: fix queue update Maxime Coquelin
@ 2020-07-24  4:55 ` Wang, Yinan
  2020-07-24  7:06   ` Maxime Coquelin
  2 siblings, 1 reply; 11+ messages in thread
From: Wang, Yinan @ 2020-07-24  4:55 UTC (permalink / raw)
  To: Maxime Coquelin, dev, matan, Xia, Chenbo, Liu, Yong
  Cc: thomas, Yigit, Ferruh, david.marchand

Hi Maxime,

The performance drop issue can be fixed, thanks!
The multi-queues interrupt issue still exist w/ this patch set.

BR,
Yinan

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2020?7?23? 21:09
> 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 0/2] Fix vhost performance regression
> 
> Hi,
> 
> This series aims at fixing the performance degradation reported
> by Intel QE. I managed to reproduce the issue, and this series
> fixes it.
> 
> I only tested the first test case provided in the Bz[0], but wanted
> to send early for Intel QE to try and confirm it solves the issue.
> 
> I will work on reproducing the other test cases, and see if this
> also fixes them.
> 
> Thanks to Intel QE team for finding this issue.
> Maxime
> 
> [0]: https://bugs.dpdk.org/show_bug.cgi?id=507#c0
> 
> Maxime Coquelin (2):
>   vhost: fix guest notification setting
>   net/vhost: fix queue update
> 
>  drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
>  lib/librte_vhost/vhost.c          | 24 ++++++++++++++++++++----
>  lib/librte_vhost/vhost.h          |  5 +++++
>  lib/librte_vhost/vhost_user.c     | 11 ++++++++---
>  4 files changed, 39 insertions(+), 26 deletions(-)
> 
> --
> 2.26.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix vhost performance regression
  2020-07-24  4:55 ` [dpdk-dev] [PATCH 0/2] Fix vhost performance regression Wang, Yinan
@ 2020-07-24  7:06   ` Maxime Coquelin
  2020-07-24  8:54     ` Maxime Coquelin
  2020-07-24 12:42     ` David Marchand
  0 siblings, 2 replies; 11+ messages in thread
From: Maxime Coquelin @ 2020-07-24  7:06 UTC (permalink / raw)
  To: Wang, Yinan, dev, matan, Xia, Chenbo, Liu, Yong
  Cc: thomas, Yigit, Ferruh, david.marchand

Hi Yinan,

On 7/24/20 6:55 AM, Wang, Yinan wrote:
> Hi Maxime,
> 
> The performance drop issue can be fixed, thanks!
> The multi-queues interrupt issue still exist w/ this patch set.

Thanks for the test report, so that's only half good.
I'm setting up the multi-queues interrupt test case to further debug it.

Regards,
Maxime

> BR,
> Yinan
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: 2020?7?23? 21:09
>> 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 0/2] Fix vhost performance regression
>>
>> Hi,
>>
>> This series aims at fixing the performance degradation reported
>> by Intel QE. I managed to reproduce the issue, and this series
>> fixes it.
>>
>> I only tested the first test case provided in the Bz[0], but wanted
>> to send early for Intel QE to try and confirm it solves the issue.
>>
>> I will work on reproducing the other test cases, and see if this
>> also fixes them.
>>
>> Thanks to Intel QE team for finding this issue.
>> Maxime
>>
>> [0]: https://bugs.dpdk.org/show_bug.cgi?id=507#c0
>>
>> Maxime Coquelin (2):
>>   vhost: fix guest notification setting
>>   net/vhost: fix queue update
>>
>>  drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
>>  lib/librte_vhost/vhost.c          | 24 ++++++++++++++++++++----
>>  lib/librte_vhost/vhost.h          |  5 +++++
>>  lib/librte_vhost/vhost_user.c     | 11 ++++++++---
>>  4 files changed, 39 insertions(+), 26 deletions(-)
>>
>> --
>> 2.26.2
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix vhost performance regression
  2020-07-24  7:06   ` Maxime Coquelin
@ 2020-07-24  8:54     ` Maxime Coquelin
  2020-07-24 15:43       ` Maxime Coquelin
  2020-07-24 12:42     ` David Marchand
  1 sibling, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2020-07-24  8:54 UTC (permalink / raw)
  To: Wang, Yinan, dev, matan, Xia, Chenbo, Liu, Yong
  Cc: thomas, Yigit, Ferruh, david.marchand



On 7/24/20 9:06 AM, Maxime Coquelin wrote:
> Hi Yinan,
> 
> On 7/24/20 6:55 AM, Wang, Yinan wrote:
>> Hi Maxime,
>>
>> The performance drop issue can be fixed, thanks!
>> The multi-queues interrupt issue still exist w/ this patch set.
> 
> Thanks for the test report, so that's only half good.
> I'm setting up the multi-queues interrupt test case to further debug it.

I have now a reproducer, i.e. only interrupts are received on rxq0.

(gdb) p *((struct internal_list *)internal_list)->eth_dev->intr_handle
$20 = {
  {
    vfio_dev_fd = 0,
    uio_cfg_fd = 0
  },
  fd = 0,
  type = RTE_INTR_HANDLE_VDEV,
  max_intr = 2,
  nb_efd = 1,
  efd_counter_size = 8 '\b',
  efds = {622, 621, 624, 626, 628, 630, 632, 634, 636, 638, 640, 642,
692, 646, 701, 650, 0 <repeats 496 times>},
  elist = {{
      status = 1,
      fd = 622,
      epfd = 645,
      epdata = {
        event = 2147483651,
        data = 0x1,
        cb_fun = 0x8af840 <eal_intr_proc_rxtx_intr>,
        cb_arg = 0x7f4df0001580
      }
    }, {
      status = 0,
      fd = 0,
      epfd = 0,
      epdata = {
        event = 0,
        data = 0x0,
        cb_fun = 0x0,
        cb_arg = 0x0
      }
    } <repeats 511 times>},
  intr_vec = 0x7f4df0007db0
}

In above dump, we can see the efds are well set via the fix provided by
Matan, but max_intr and nb_efd aren't so polling won't take them into
account.

I'm working on a fix.

Regards,
Maxime

> Regards,
> Maxime
> 
>> BR,
>> Yinan
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: 2020?7?23? 21:09
>>> 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 0/2] Fix vhost performance regression
>>>
>>> Hi,
>>>
>>> This series aims at fixing the performance degradation reported
>>> by Intel QE. I managed to reproduce the issue, and this series
>>> fixes it.
>>>
>>> I only tested the first test case provided in the Bz[0], but wanted
>>> to send early for Intel QE to try and confirm it solves the issue.
>>>
>>> I will work on reproducing the other test cases, and see if this
>>> also fixes them.
>>>
>>> Thanks to Intel QE team for finding this issue.
>>> Maxime
>>>
>>> [0]: https://bugs.dpdk.org/show_bug.cgi?id=507#c0
>>>
>>> Maxime Coquelin (2):
>>>   vhost: fix guest notification setting
>>>   net/vhost: fix queue update
>>>
>>>  drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
>>>  lib/librte_vhost/vhost.c          | 24 ++++++++++++++++++++----
>>>  lib/librte_vhost/vhost.h          |  5 +++++
>>>  lib/librte_vhost/vhost_user.c     | 11 ++++++++---
>>>  4 files changed, 39 insertions(+), 26 deletions(-)
>>>
>>> --
>>> 2.26.2
>>
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix vhost performance regression
  2020-07-24  7:06   ` Maxime Coquelin
  2020-07-24  8:54     ` Maxime Coquelin
@ 2020-07-24 12:42     ` David Marchand
  1 sibling, 0 replies; 11+ messages in thread
From: David Marchand @ 2020-07-24 12:42 UTC (permalink / raw)
  To: dts
  Cc: Wang, Yinan, dev, matan, Xia, Chenbo, Liu, Yong, thomas, Yigit,
	Ferruh, Maxime Coquelin, ci

On Fri, Jul 24, 2020 at 9:06 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> On 7/24/20 6:55 AM, Wang, Yinan wrote:
> > Hi Maxime,
> >
> > The performance drop issue can be fixed, thanks!
> > The multi-queues interrupt issue still exist w/ this patch set.
>
> Thanks for the test report, so that's only half good.
> I'm setting up the multi-queues interrupt test case to further debug it.

- We wasted time trying to understand why we could not start the
l3fwd-power application like in the test report.

I created a bz for dts.
https://bugs.dpdk.org/show_bug.cgi?id=515


- There are also changes on the default configuration + recompilation
of dpdk with make in DTS.
DTS should use the default configuration.
make will be dropped in 20.11.


Thanks.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix vhost performance regression
  2020-07-24  8:54     ` Maxime Coquelin
@ 2020-07-24 15:43       ` Maxime Coquelin
  2020-07-27  8:28         ` Matan Azrad
  0 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2020-07-24 15:43 UTC (permalink / raw)
  To: Wang, Yinan, dev, matan, Xia, Chenbo, Liu, Yong
  Cc: thomas, Yigit, Ferruh, david.marchand



On 7/24/20 10:54 AM, Maxime Coquelin wrote:
> 
> 
> On 7/24/20 9:06 AM, Maxime Coquelin wrote:
>> Hi Yinan,
>>
>> On 7/24/20 6:55 AM, Wang, Yinan wrote:
>>> Hi Maxime,
>>>
>>> The performance drop issue can be fixed, thanks!
>>> The multi-queues interrupt issue still exist w/ this patch set.
>>
>> Thanks for the test report, so that's only half good.
>> I'm setting up the multi-queues interrupt test case to further debug it.
> 
> I have now a reproducer, i.e. only interrupts are received on rxq0.
> 
> (gdb) p *((struct internal_list *)internal_list)->eth_dev->intr_handle
> $20 = {
>   {
>     vfio_dev_fd = 0,
>     uio_cfg_fd = 0
>   },
>   fd = 0,
>   type = RTE_INTR_HANDLE_VDEV,
>   max_intr = 2,
>   nb_efd = 1,
>   efd_counter_size = 8 '\b',
>   efds = {622, 621, 624, 626, 628, 630, 632, 634, 636, 638, 640, 642,
> 692, 646, 701, 650, 0 <repeats 496 times>},
>   elist = {{
>       status = 1,
>       fd = 622,
>       epfd = 645,
>       epdata = {
>         event = 2147483651,
>         data = 0x1,
>         cb_fun = 0x8af840 <eal_intr_proc_rxtx_intr>,
>         cb_arg = 0x7f4df0001580
>       }
>     }, {
>       status = 0,
>       fd = 0,
>       epfd = 0,
>       epdata = {
>         event = 0,
>         data = 0x0,
>         cb_fun = 0x0,
>         cb_arg = 0x0
>       }
>     } <repeats 511 times>},
>   intr_vec = 0x7f4df0007db0
> }
> 
> In above dump, we can see the efds are well set via the fix provided by
> Matan, but max_intr and nb_efd aren't so polling won't take them into
> account.
> 
> I'm working on a fix.

So it is a bit more complex than I imagined.
There are no DPDK API to update the FD in the epoll, so it seems we need
to do it directly in the driver by removing the old one and adding the
new one.

I have cooked a patch that makes it work, but I would like to know if
that would be acceptable  for this release? We could imagine introducing
new rte_epoll API to handle that properly in v20.11.

Matan, could you review below patch and confirm whether it is safe?

(The patch needs some style clean-up before being submitted).

Thanks in advance,
Maxime

==================================================================

diff --git a/drivers/net/vhost/rte_eth_vhost.c
b/drivers/net/vhost/rte_eth_vhost.c
index 14b7b59f67..f36ea4b24c 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.
@@ -852,9 +855,17 @@ vring_conf_update(int vid, struct rte_eth_dev
*eth_dev, uint16_t vring_id)
                        return ret;

                if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
+                       handle = eth_dev->intr_handle;
                        VHOST_LOG(INFO, "kickfd for rxq-%d was changed.\n",
                                          rx_idx);
-                       eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
+
+                       handle->efds[rx_idx] = vring.kickfd;
+                       epfd = handle->elist[rx_idx].epfd;
+                       rev = handle->elist[rx_idx];
+                       rev.fd = vring.kickfd;
+                       rte_epoll_ctl(epfd, EPOLL_CTL_DEL,
handle->elist[rx_idx].fd, &handle->elist[rx_idx]);
+                       handle->elist[rx_idx] = rev;
+                       rte_epoll_ctl(epfd, EPOLL_CTL_ADD, rev.fd,
&handle->elist[rx_idx]);
                }
        }


> Regards,
> Maxime
> 
>> Regards,
>> Maxime
>>
>>> BR,
>>> Yinan
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: 2020?7?23? 21:09
>>>> 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 0/2] Fix vhost performance regression
>>>>
>>>> Hi,
>>>>
>>>> This series aims at fixing the performance degradation reported
>>>> by Intel QE. I managed to reproduce the issue, and this series
>>>> fixes it.
>>>>
>>>> I only tested the first test case provided in the Bz[0], but wanted
>>>> to send early for Intel QE to try and confirm it solves the issue.
>>>>
>>>> I will work on reproducing the other test cases, and see if this
>>>> also fixes them.
>>>>
>>>> Thanks to Intel QE team for finding this issue.
>>>> Maxime
>>>>
>>>> [0]: https://bugs.dpdk.org/show_bug.cgi?id=507#c0
>>>>
>>>> Maxime Coquelin (2):
>>>>   vhost: fix guest notification setting
>>>>   net/vhost: fix queue update
>>>>
>>>>  drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
>>>>  lib/librte_vhost/vhost.c          | 24 ++++++++++++++++++++----
>>>>  lib/librte_vhost/vhost.h          |  5 +++++
>>>>  lib/librte_vhost/vhost_user.c     | 11 ++++++++---
>>>>  4 files changed, 39 insertions(+), 26 deletions(-)
>>>>
>>>> --
>>>> 2.26.2
>>>
>>
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Fix vhost performance regression
  2020-07-24 15:43       ` Maxime Coquelin
@ 2020-07-27  8:28         ` Matan Azrad
  0 siblings, 0 replies; 11+ messages in thread
From: Matan Azrad @ 2020-07-27  8:28 UTC (permalink / raw)
  To: Maxime Coquelin, Wang, Yinan, dev, Xia, Chenbo, Liu, Yong
  Cc: Thomas Monjalon, Yigit, Ferruh, david.marchand

Hi Maxime

I understand the direction...

See below

From: Maxime Coquelin 
> On 7/24/20 10:54 AM, Maxime Coquelin wrote:
> >
> >
> > On 7/24/20 9:06 AM, Maxime Coquelin wrote:
> >> Hi Yinan,
> >>
> >> On 7/24/20 6:55 AM, Wang, Yinan wrote:
> >>> Hi Maxime,
> >>>
> >>> The performance drop issue can be fixed, thanks!
> >>> The multi-queues interrupt issue still exist w/ this patch set.
> >>
> >> Thanks for the test report, so that's only half good.
> >> I'm setting up the multi-queues interrupt test case to further debug it.
> >
> > I have now a reproducer, i.e. only interrupts are received on rxq0.
> >
> > (gdb) p *((struct internal_list *)internal_list)->eth_dev->intr_handle
> > $20 = {
> >   {
> >     vfio_dev_fd = 0,
> >     uio_cfg_fd = 0
> >   },
> >   fd = 0,
> >   type = RTE_INTR_HANDLE_VDEV,
> >   max_intr = 2,
> >   nb_efd = 1,
> >   efd_counter_size = 8 '\b',
> >   efds = {622, 621, 624, 626, 628, 630, 632, 634, 636, 638, 640, 642,
> > 692, 646, 701, 650, 0 <repeats 496 times>},
> >   elist = {{
> >       status = 1,
> >       fd = 622,
> >       epfd = 645,
> >       epdata = {
> >         event = 2147483651,
> >         data = 0x1,
> >         cb_fun = 0x8af840 <eal_intr_proc_rxtx_intr>,
> >         cb_arg = 0x7f4df0001580
> >       }
> >     }, {
> >       status = 0,
> >       fd = 0,
> >       epfd = 0,
> >       epdata = {
> >         event = 0,
> >         data = 0x0,
> >         cb_fun = 0x0,
> >         cb_arg = 0x0
> >       }
> >     } <repeats 511 times>},
> >   intr_vec = 0x7f4df0007db0
> > }
> >
> > In above dump, we can see the efds are well set via the fix provided
> > by Matan, but max_intr and nb_efd aren't so polling won't take them
> > into account.
> >
> > I'm working on a fix.
> 
> So it is a bit more complex than I imagined.
> There are no DPDK API to update the FD in the epoll, so it seems we need to
> do it directly in the driver by removing the old one and adding the new one.
> 
> I have cooked a patch that makes it work, but I would like to know if that
> would be acceptable  for this release? We could imagine introducing new
> rte_epoll API to handle that properly in v20.11.
> 
> Matan, could you review below patch and confirm whether it is safe?
> 
> (The patch needs some style clean-up before being submitted).
> 
> Thanks in advance,
> Maxime
> 
> ==========================================================
> ========
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index 14b7b59f67..f36ea4b24c 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.
> @@ -852,9 +855,17 @@ vring_conf_update(int vid, struct rte_eth_dev
> *eth_dev, uint16_t vring_id)
>                         return ret;
> 
>                 if (vring.kickfd != eth_dev->intr_handle->efds[rx_idx]) {
> +                       handle = eth_dev->intr_handle;
>                         VHOST_LOG(INFO, "kickfd for rxq-%d was changed.\n",
>                                           rx_idx);
> -                       eth_dev->intr_handle->efds[rx_idx] = vring.kickfd;
> +
> +                       handle->efds[rx_idx] = vring.kickfd;
> +                       epfd = handle->elist[rx_idx].epfd;
> +                       rev = handle->elist[rx_idx];
> +                       rev.fd = vring.kickfd;
> +                       rte_epoll_ctl(epfd, EPOLL_CTL_DEL,

We may have race with application code here and below, no?

> handle->elist[rx_idx].fd, &handle->elist[rx_idx]);
> +                       handle->elist[rx_idx] = rev;
> +                       rte_epoll_ctl(epfd, EPOLL_CTL_ADD, rev.fd,
> &handle->elist[rx_idx]);
>                 }
>         }

One more option is to let application do it by the QUEUE_STATE event callback...
But it requires app change ☹

> 
> > Regards,
> > Maxime
> >
> >> Regards,
> >> Maxime
> >>
> >>> BR,
> >>> Yinan
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Sent: 2020?7?23? 21:09
> >>>> 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 0/2] Fix vhost performance regression
> >>>>
> >>>> Hi,
> >>>>
> >>>> This series aims at fixing the performance degradation reported by
> >>>> Intel QE. I managed to reproduce the issue, and this series fixes
> >>>> it.
> >>>>
> >>>> I only tested the first test case provided in the Bz[0], but wanted
> >>>> to send early for Intel QE to try and confirm it solves the issue.
> >>>>
> >>>> I will work on reproducing the other test cases, and see if this
> >>>> also fixes them.
> >>>>
> >>>> Thanks to Intel QE team for finding this issue.
> >>>> Maxime
> >>>>
> >>>> [0]:
> >>>>
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fb
> >>>>
> ugs.dpdk.org%2Fshow_bug.cgi%3Fid%3D507%23c0&amp;data=02%7C01%7C
> mata
> >>>>
> n%40mellanox.com%7Cb76cf49cc7f04f000f0108d82fe850cf%7Ca652971c7d2e
> 4
> >>>>
> d9ba6a4d149256f461b%7C0%7C0%7C637312022123706774&amp;sdata=r2m9
> XpBc
> >>>> rFs7tb14a4Pcs8Qx7fiBpB2E4beRXenuVa0%3D&amp;reserved=0
> >>>>
> >>>> Maxime Coquelin (2):
> >>>>   vhost: fix guest notification setting
> >>>>   net/vhost: fix queue update
> >>>>
> >>>>  drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
> >>>>  lib/librte_vhost/vhost.c          | 24 ++++++++++++++++++++----
> >>>>  lib/librte_vhost/vhost.h          |  5 +++++
> >>>>  lib/librte_vhost/vhost_user.c     | 11 ++++++++---
> >>>>  4 files changed, 39 insertions(+), 26 deletions(-)
> >>>>
> >>>> --
> >>>> 2.26.2
> >>>
> >>
> >


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] vhost: fix guest notification setting
  2020-07-23 13:08 ` [dpdk-dev] [PATCH 1/2] vhost: fix guest notification setting Maxime Coquelin
@ 2020-07-27 13:46   ` Xia, Chenbo
  0 siblings, 0 replies; 11+ messages in thread
From: Xia, Chenbo @ 2020-07-27 13:46 UTC (permalink / raw)
  To: Maxime Coquelin, dev, matan, Liu, Yong, Wang, Yinan
  Cc: thomas, Yigit, Ferruh, david.marchand


> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, July 23, 2020 9:09 PM
> 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 1/2] vhost: fix guest notification setting
> 
> 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>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] net/vhost: fix queue update
  2020-07-23 13:08 ` [dpdk-dev] [PATCH 2/2] net/vhost: fix queue update Maxime Coquelin
@ 2020-07-27 13:54   ` Xia, Chenbo
  0 siblings, 0 replies; 11+ messages in thread
From: Xia, Chenbo @ 2020-07-27 13:54 UTC (permalink / raw)
  To: Maxime Coquelin, dev, matan, Liu, Yong, Wang, Yinan
  Cc: thomas, Yigit, Ferruh, david.marchand

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, July 23, 2020 9:09 PM
> 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 2/2] 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>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index bbf79b2c0e..14b7b59f67 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,14 @@ 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)
> +			return ret;

Do you think it'll be better to add a VHOST_LOG here to show the get_vring failure
like it's done in other place? But since it's the only place that will fail, it's also easy
for user to find out. 😊

Thanks,
Chenbo

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-07-27 13:55 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23 13:08 [dpdk-dev] [PATCH 0/2] Fix vhost performance regression Maxime Coquelin
2020-07-23 13:08 ` [dpdk-dev] [PATCH 1/2] vhost: fix guest notification setting Maxime Coquelin
2020-07-27 13:46   ` Xia, Chenbo
2020-07-23 13:08 ` [dpdk-dev] [PATCH 2/2] net/vhost: fix queue update Maxime Coquelin
2020-07-27 13:54   ` Xia, Chenbo
2020-07-24  4:55 ` [dpdk-dev] [PATCH 0/2] Fix vhost performance regression Wang, Yinan
2020-07-24  7:06   ` Maxime Coquelin
2020-07-24  8:54     ` Maxime Coquelin
2020-07-24 15:43       ` Maxime Coquelin
2020-07-27  8:28         ` Matan Azrad
2020-07-24 12:42     ` David Marchand

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).