DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v1] net/vhost: support queue update
@ 2020-07-19 16:18 Matan Azrad
  2020-07-21 16:38 ` [dpdk-dev] [PATCH v2] " Matan Azrad
  0 siblings, 1 reply; 4+ messages in thread
From: Matan Azrad @ 2020-07-19 16:18 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: dev, Ferruh Yigit, chenbo.xia, david.marchand, thomas

The commit below changed the readiness condition of vhost device to fix
multi-queues issues showed with QEMU versions.

Now, the vhost device is ready when the first queue-pair is ready.
When more queues are being ready, the queue state callback will be
triggered to notify the vhost manager.

In case of Rx interrupt configuration, the vhost driver set the
kickfd queue file descriptor in order to be notified on Rx traffic.

So, when queue becomes ready, the kickfd may be changed and should be
updated in the Rx interrupt structure.

Update kickfd when the queue state callback is invoked.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 9fbf39f..805d75e 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -831,6 +831,39 @@ struct vhost_xstats_name_off {
 }
 
 static int
+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;
+	int rx_idx = vring_id % 2 ? (vring_id - 1) >> 1 : -1;
+	int ret = 0;
+
+	/*
+	 * The vring kickfd may be changed after the new device notification.
+	 * Update it when the vring state is updated.
+	 */
+	if (rx_idx >= 0 && rx_idx < eth_dev->data->nb_rx_queues &&
+	    rte_atomic32_read(&internal->dev_attached) &&
+	    rte_atomic32_read(&internal->started) &&
+	    dev_conf->intr_conf.rxq) {
+		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;
+			}
+		}
+	}
+
+	return ret;
+}
+
+static int
 vring_state_changed(int vid, uint16_t vring, int enable)
 {
 	struct rte_vhost_vring_state *state;
@@ -848,6 +881,11 @@ struct vhost_xstats_name_off {
 	eth_dev = list->eth_dev;
 	/* won't be NULL */
 	state = vring_states[eth_dev->data->port_id];
+
+	if (vring_conf_update(vid, eth_dev, vring))
+		VHOST_LOG(INFO, "Failed to update vring-%d configuration.\n",
+			  (int)vring);
+
 	rte_spinlock_lock(&state->lock);
 	if (state->cur[vring] == enable) {
 		rte_spinlock_unlock(&state->lock);
-- 
1.8.3.1


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

* [dpdk-dev] [PATCH v2] net/vhost: support queue update
  2020-07-19 16:18 [dpdk-dev] [PATCH v1] net/vhost: support queue update Matan Azrad
@ 2020-07-21 16:38 ` Matan Azrad
  2020-07-21 17:25   ` Ferruh Yigit
  2020-07-21 17:27   ` Maxime Coquelin
  0 siblings, 2 replies; 4+ messages in thread
From: Matan Azrad @ 2020-07-21 16:38 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: dev, Ferruh Yigit, chenbo.xia, david.marchand, thomas

The commit below changed the readiness condition of vhost device to fix
multi-queues issues showed with QEMU versions.

Now, the vhost device is ready when the first queue-pair is ready.
When more queues are being ready, the queue state callback will be
triggered to notify the vhost manager.

In case of Rx interrupt configuration, the vhost driver set the
kickfd queue file descriptor in order to be notified on Rx traffic.

So, when queue becomes ready, the kickfd may be changed and should be
updated in the Rx interrupt structure.

Update kickfd when the queue state callback is invoked.
Also update event notification when it is enabled by the user.

Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 49 +++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 9fbf39f66b..bbf79b2c0e 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -94,6 +94,7 @@ struct vhost_queue {
 	struct rte_mempool *mb_pool;
 	uint16_t port;
 	uint16_t virtqueue_id;
+	bool intr_en;
 	struct vhost_stats stats;
 };
 
@@ -546,6 +547,8 @@ 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;
 }
 
@@ -571,6 +574,8 @@ 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;
 }
 
@@ -830,6 +835,45 @@ destroy_device(int vid)
 	_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, NULL);
 }
 
+static int
+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;
+
+	/*
+	 * The vring kickfd may be changed after the new device notification.
+	 * Update it when the vring state is updated.
+	 */
+	if (rx_idx >= 0 && rx_idx < eth_dev->data->nb_rx_queues &&
+	    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;
+			}
+
+			rte_vhost_enable_guest_notification(vid, vring_id,
+							    vq->intr_en);
+			rte_wmb();
+		}
+	}
+
+	return ret;
+}
+
 static int
 vring_state_changed(int vid, uint16_t vring, int enable)
 {
@@ -848,6 +892,11 @@ vring_state_changed(int vid, uint16_t vring, int enable)
 	eth_dev = list->eth_dev;
 	/* won't be NULL */
 	state = vring_states[eth_dev->data->port_id];
+
+	if (enable && vring_conf_update(vid, eth_dev, vring))
+		VHOST_LOG(INFO, "Failed to update vring-%d configuration.\n",
+			  (int)vring);
+
 	rte_spinlock_lock(&state->lock);
 	if (state->cur[vring] == enable) {
 		rte_spinlock_unlock(&state->lock);
-- 
2.17.1


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

* Re: [dpdk-dev] [PATCH v2] net/vhost: support queue update
  2020-07-21 16:38 ` [dpdk-dev] [PATCH v2] " Matan Azrad
@ 2020-07-21 17:25   ` Ferruh Yigit
  2020-07-21 17:27   ` Maxime Coquelin
  1 sibling, 0 replies; 4+ messages in thread
From: Ferruh Yigit @ 2020-07-21 17:25 UTC (permalink / raw)
  To: Matan Azrad, Maxime Coquelin
  Cc: dev, chenbo.xia, david.marchand, thomas, Marvin Liu

On 7/21/2020 5:38 PM, Matan Azrad wrote:
> The commit below changed the readiness condition of vhost device to fix
> multi-queues issues showed with QEMU versions.
> 
> Now, the vhost device is ready when the first queue-pair is ready.
> When more queues are being ready, the queue state callback will be
> triggered to notify the vhost manager.
> 
> In case of Rx interrupt configuration, the vhost driver set the
> kickfd queue file descriptor in order to be notified on Rx traffic.
> 
> So, when queue becomes ready, the kickfd may be changed and should be
> updated in the Rx interrupt structure.
> 
> Update kickfd when the queue state callback is invoked.
> Also update event notification when it is enabled by the user.
> 
> Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Suggested-by: Marvin Liu <yong.liu@intel.com>


This patch addresses a ~%20 performance drop in vhost reported with -rc1.

Although it is missing component maintainers ack, I will merge it relying on
offline discussion Maxime, Matan, Marvin & Chenbo involved, to address the
performance issue observed in -rc2.
Hopefully won't but if we observe issues with this patch, there will be time to
address it before -rc3, I believe it is safer/better to merge it for -rc2 than
holding this fix till -rc3, at least it can be verified this way as part of -rc2.

Applied to dpdk-next-net/master, thanks.

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

* Re: [dpdk-dev] [PATCH v2] net/vhost: support queue update
  2020-07-21 16:38 ` [dpdk-dev] [PATCH v2] " Matan Azrad
  2020-07-21 17:25   ` Ferruh Yigit
@ 2020-07-21 17:27   ` Maxime Coquelin
  1 sibling, 0 replies; 4+ messages in thread
From: Maxime Coquelin @ 2020-07-21 17:27 UTC (permalink / raw)
  To: Matan Azrad; +Cc: dev, Ferruh Yigit, chenbo.xia, david.marchand, thomas

Thanks Matan for the fix.

On 7/21/20 6:38 PM, Matan Azrad wrote:
> The commit below changed the readiness condition of vhost device to fix
> multi-queues issues showed with QEMU versions.
> 
> Now, the vhost device is ready when the first queue-pair is ready.
> When more queues are being ready, the queue state callback will be
> triggered to notify the vhost manager.
> 
> In case of Rx interrupt configuration, the vhost driver set the
> kickfd queue file descriptor in order to be notified on Rx traffic.
> 
> So, when queue becomes ready, the kickfd may be changed and should be
> updated in the Rx interrupt structure.
> 
> Update kickfd when the queue state callback is invoked.
> Also update event notification when it is enabled by the user.
> 
> Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 49 +++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 9fbf39f66b..bbf79b2c0e 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -94,6 +94,7 @@ struct vhost_queue {
>  	struct rte_mempool *mb_pool;
>  	uint16_t port;
>  	uint16_t virtqueue_id;
> +	bool intr_en;
>  	struct vhost_stats stats;
>  };
>  
> @@ -546,6 +547,8 @@ 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;
>  }
>  
> @@ -571,6 +574,8 @@ 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;
>  }
>  
> @@ -830,6 +835,45 @@ destroy_device(int vid)
>  	_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_INTR_LSC, NULL);
>  }
>  
> +static int
> +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;
> +
> +	/*
> +	 * The vring kickfd may be changed after the new device notification.
> +	 * Update it when the vring state is updated.
> +	 */
> +	if (rx_idx >= 0 && rx_idx < eth_dev->data->nb_rx_queues &&
> +	    rte_atomic32_read(&internal->dev_attached) &&
> +	    rte_atomic32_read(&internal->started) &&
> +	    dev_conf->intr_conf.rxq) {

I am not sure we need to filter on rxq, we could just call
rte_vhost_enable_guest_notification(). It would be disabled for tx
queues, as it is done in the .new_device callback so harmless and this
function would be simpler.

But I think your code work, so we should get it in -rc2 to get proper
testing:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

> +		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;
> +			}
> +
> +			rte_vhost_enable_guest_notification(vid, vring_id,
> +							    vq->intr_en);
> +			rte_wmb();
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static int
>  vring_state_changed(int vid, uint16_t vring, int enable)
>  {
> @@ -848,6 +892,11 @@ vring_state_changed(int vid, uint16_t vring, int enable)
>  	eth_dev = list->eth_dev;
>  	/* won't be NULL */
>  	state = vring_states[eth_dev->data->port_id];
> +
> +	if (enable && vring_conf_update(vid, eth_dev, vring))
> +		VHOST_LOG(INFO, "Failed to update vring-%d configuration.\n",
> +			  (int)vring);
> +
>  	rte_spinlock_lock(&state->lock);
>  	if (state->cur[vring] == enable) {
>  		rte_spinlock_unlock(&state->lock);
> 


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

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

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-19 16:18 [dpdk-dev] [PATCH v1] net/vhost: support queue update Matan Azrad
2020-07-21 16:38 ` [dpdk-dev] [PATCH v2] " Matan Azrad
2020-07-21 17:25   ` Ferruh Yigit
2020-07-21 17:27   ` Maxime Coquelin

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