diff mbox series

[v2,2/5] vhost: improve device readiness notifications

Message ID 1593092298-52257-3-git-send-email-matan@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: improve ready state |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Matan Azrad June 25, 2020, 1:38 p.m. UTC 
  Some guest drivers may not configure disabled virtio queues.

In this case, the vhost management never notifies the application and
the vDPA device readiness because it waits to the device to be ready.

The current ready state means that all the virtio queues should be
configured regardless the enablement status.

In order to support this case, this patch changes the ready state:
The device is ready when at least 1 queue pair is configured and
enabled.

So, now, the application and vDPA driver are notifies when the first
queue pair is configured and enabled.

Also the queue notifications will be triggered according to the new
ready definition.

Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 lib/librte_vhost/vhost.h      |  1 +
 lib/librte_vhost/vhost_user.c | 55 +++++++++++++++++++++++++++++--------------
 2 files changed, 38 insertions(+), 18 deletions(-)

Comments

Maxime Coquelin June 26, 2020, 12:10 p.m. UTC | #1 
On 6/25/20 3:38 PM, Matan Azrad wrote:
> Some guest drivers may not configure disabled virtio queues.
> 
> In this case, the vhost management never notifies the application and
> the vDPA device readiness because it waits to the device to be ready.
> 
> The current ready state means that all the virtio queues should be
> configured regardless the enablement status.
> 
> In order to support this case, this patch changes the ready state:
> The device is ready when at least 1 queue pair is configured and
> enabled.
> 
> So, now, the application and vDPA driver are notifies when the first
> queue pair is configured and enabled.
> 
> Also the queue notifications will be triggered according to the new
> ready definition.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  lib/librte_vhost/vhost.h      |  1 +
>  lib/librte_vhost/vhost_user.c | 55 +++++++++++++++++++++++++++++--------------
>  2 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 17f1e9a..8a74f33 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -151,6 +151,7 @@ struct vhost_virtqueue {
>  	int			backend;
>  	int			enabled;
>  	int			access_ok;
> +	int			ready;
>  	rte_spinlock_t		access_lock;
>  
>  	/* Used to notify the guest (trigger interrupt) */
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 8d8050b..b90fc78 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -228,6 +228,21 @@
>  	dev->postcopy_listening = 0;
>  }
>  
> +static void
> +vhost_user_notify_queue_state(struct virtio_net *dev, uint16_t index,
> +			      int enable)
> +{
> +	int did = dev->vdpa_dev_id;
> +	struct rte_vdpa_device *vdpa_dev = rte_vdpa_get_device(did);
> +
> +	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> +		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
> +
> +	if (dev->notify_ops->vring_state_changed)
> +		dev->notify_ops->vring_state_changed(dev->vid,
> +				index, enable);
> +}
> +
>  /*
>   * This function just returns success at the moment unless
>   * the device hasn't been initialised.
> @@ -1306,27 +1321,31 @@
>  
>  	return rings_ok &&
>  	       vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> -	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
> +	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> +	       vq->enabled;
>  }
>  
> +#define VIRTIO_DEV_NUM_VQS_TO_BE_READY 2u

(Thinking out loud) If for some reason it would cause issue with OVS-
DPDK or other application, it should be easy to only apply this new way
of initializing the device based on whether vdpa device is attached or
not.

>  static int
>  virtio_is_ready(struct virtio_net *dev)
>  {
>  	struct vhost_virtqueue *vq;
>  	uint32_t i;
>  
> -	if (dev->nr_vring == 0)
> +	if (dev->nr_vring < VIRTIO_DEV_NUM_VQS_TO_BE_READY)
>  		return 0;
>  
> -	for (i = 0; i < dev->nr_vring; i++) {
> +	for (i = 0; i < VIRTIO_DEV_NUM_VQS_TO_BE_READY; i++) {
>  		vq = dev->virtqueue[i];
>  
>  		if (!vq_is_ready(dev, vq))
>  			return 0;
>  	}
>  
> -	VHOST_LOG_CONFIG(INFO,
> -		"virtio is now ready for processing.\n");
> +	if (!(dev->flags & VIRTIO_DEV_RUNNING))
> +		VHOST_LOG_CONFIG(INFO,
> +			"virtio is now ready for processing.\n");
>  	return 1;
>  }


Patch looks good to me, thanks for working on it.


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

Maxime
Chenbo Xia June 28, 2020, 3:08 a.m. UTC | #2 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Matan Azrad
> Sent: Thursday, June 25, 2020 9:38 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>
> Cc: dev@dpdk.org; Wang, Xiao W <xiao.w.wang@intel.com>
> Subject: [dpdk-dev] [PATCH v2 2/5] vhost: improve device readiness
> notifications
> 
> Some guest drivers may not configure disabled virtio queues.
> 
> In this case, the vhost management never notifies the application and the vDPA
> device readiness because it waits to the device to be ready.
> 
> The current ready state means that all the virtio queues should be configured
> regardless the enablement status.
> 
> In order to support this case, this patch changes the ready state:
> The device is ready when at least 1 queue pair is configured and enabled.
> 
> So, now, the application and vDPA driver are notifies when the first queue pair is
> configured and enabled.
> 
> Also the queue notifications will be triggered according to the new ready
> definition.
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  lib/librte_vhost/vhost.h      |  1 +
>  lib/librte_vhost/vhost_user.c | 55 +++++++++++++++++++++++++++++------------
> --
>  2 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index
> 17f1e9a..8a74f33 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -151,6 +151,7 @@ struct vhost_virtqueue {
>  	int			backend;
>  	int			enabled;
>  	int			access_ok;
> +	int			ready;
>  	rte_spinlock_t		access_lock;
> 
>  	/* Used to notify the guest (trigger interrupt) */ diff --git
> a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
> 8d8050b..b90fc78 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -228,6 +228,21 @@
>  	dev->postcopy_listening = 0;
>  }
> 
> +static void
> +vhost_user_notify_queue_state(struct virtio_net *dev, uint16_t index,
> +			      int enable)
> +{
> +	int did = dev->vdpa_dev_id;
> +	struct rte_vdpa_device *vdpa_dev = rte_vdpa_get_device(did);
> +
> +	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> +		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
> +
> +	if (dev->notify_ops->vring_state_changed)
> +		dev->notify_ops->vring_state_changed(dev->vid,
> +				index, enable);
> +}
> +
>  /*
>   * This function just returns success at the moment unless
>   * the device hasn't been initialised.
> @@ -1306,27 +1321,31 @@
> 
>  	return rings_ok &&
>  	       vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> -	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
> +	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> +	       vq->enabled;
>  }
> 
> +#define VIRTIO_DEV_NUM_VQS_TO_BE_READY 2u
> +
>  static int
>  virtio_is_ready(struct virtio_net *dev)  {
>  	struct vhost_virtqueue *vq;
>  	uint32_t i;
> 
> -	if (dev->nr_vring == 0)
> +	if (dev->nr_vring < VIRTIO_DEV_NUM_VQS_TO_BE_READY)
>  		return 0;
> 
> -	for (i = 0; i < dev->nr_vring; i++) {
> +	for (i = 0; i < VIRTIO_DEV_NUM_VQS_TO_BE_READY; i++) {
>  		vq = dev->virtqueue[i];
> 
>  		if (!vq_is_ready(dev, vq))
>  			return 0;
>  	}
> 
> -	VHOST_LOG_CONFIG(INFO,
> -		"virtio is now ready for processing.\n");
> +	if (!(dev->flags & VIRTIO_DEV_RUNNING))
> +		VHOST_LOG_CONFIG(INFO,
> +			"virtio is now ready for processing.\n");
>  	return 1;
>  }
> 
> @@ -1970,8 +1989,6 @@ static int vhost_user_set_vring_err(struct virtio_net
> **pdev __rte_unused,
>  	struct virtio_net *dev = *pdev;
>  	int enable = (int)msg->payload.state.num;
>  	int index = (int)msg->payload.state.index;
> -	struct rte_vdpa_device *vdpa_dev;
> -	int did = -1;
> 
>  	if (validate_msg_fds(msg, 0) != 0)
>  		return RTE_VHOST_MSG_RESULT_ERR;
> @@ -1980,15 +1997,6 @@ static int vhost_user_set_vring_err(struct virtio_net
> **pdev __rte_unused,
>  		"set queue enable: %d to qp idx: %d\n",
>  		enable, index);
> 
> -	did = dev->vdpa_dev_id;
> -	vdpa_dev = rte_vdpa_get_device(did);
> -	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> -		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
> -
> -	if (dev->notify_ops->vring_state_changed)
> -		dev->notify_ops->vring_state_changed(dev->vid,
> -				index, enable);
> -
>  	/* On disable, rings have to be stopped being processed. */
>  	if (!enable && dev->dequeue_zero_copy)
>  		drain_zmbuf_list(dev->virtqueue[index]);
> @@ -2618,6 +2626,7 @@ typedef int (*vhost_message_handler_t)(struct
> virtio_net **pdev,
>  	int unlock_required = 0;
>  	bool handled;
>  	int request;
> +	uint32_t i;
> 
>  	dev = get_device(vid);
>  	if (dev == NULL)
> @@ -2793,6 +2802,17 @@ typedef int (*vhost_message_handler_t)(struct
> virtio_net **pdev,
>  		return -1;
>  	}
> 
> +	for (i = 0; i < dev->nr_vring; i++) {
> +		struct vhost_virtqueue *vq = dev->virtqueue[i];
> +		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;
> +		}
> +	}
> +
> +
>  	if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) {
>  		dev->flags |= VIRTIO_DEV_READY;
> 
> @@ -2810,8 +2830,7 @@ typedef int (*vhost_message_handler_t)(struct
> virtio_net **pdev,
>  	did = dev->vdpa_dev_id;
>  	vdpa_dev = rte_vdpa_get_device(did);
>  	if (vdpa_dev && virtio_is_ready(dev) &&
> -			!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) &&
> -			msg.request.master == VHOST_USER_SET_VRING_CALL)
> {
> +	    !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
>  		if (vdpa_dev->ops->dev_conf)
>  			vdpa_dev->ops->dev_conf(dev->vid);
>  		dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;
> --
> 1.8.3.1

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

Patch

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 17f1e9a..8a74f33 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -151,6 +151,7 @@  struct vhost_virtqueue {
 	int			backend;
 	int			enabled;
 	int			access_ok;
+	int			ready;
 	rte_spinlock_t		access_lock;
 
 	/* Used to notify the guest (trigger interrupt) */
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 8d8050b..b90fc78 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -228,6 +228,21 @@ 
 	dev->postcopy_listening = 0;
 }
 
+static void
+vhost_user_notify_queue_state(struct virtio_net *dev, uint16_t index,
+			      int enable)
+{
+	int did = dev->vdpa_dev_id;
+	struct rte_vdpa_device *vdpa_dev = rte_vdpa_get_device(did);
+
+	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
+		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
+
+	if (dev->notify_ops->vring_state_changed)
+		dev->notify_ops->vring_state_changed(dev->vid,
+				index, enable);
+}
+
 /*
  * This function just returns success at the moment unless
  * the device hasn't been initialised.
@@ -1306,27 +1321,31 @@ 
 
 	return rings_ok &&
 	       vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
-	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
+	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD &&
+	       vq->enabled;
 }
 
+#define VIRTIO_DEV_NUM_VQS_TO_BE_READY 2u
+
 static int
 virtio_is_ready(struct virtio_net *dev)
 {
 	struct vhost_virtqueue *vq;
 	uint32_t i;
 
-	if (dev->nr_vring == 0)
+	if (dev->nr_vring < VIRTIO_DEV_NUM_VQS_TO_BE_READY)
 		return 0;
 
-	for (i = 0; i < dev->nr_vring; i++) {
+	for (i = 0; i < VIRTIO_DEV_NUM_VQS_TO_BE_READY; i++) {
 		vq = dev->virtqueue[i];
 
 		if (!vq_is_ready(dev, vq))
 			return 0;
 	}
 
-	VHOST_LOG_CONFIG(INFO,
-		"virtio is now ready for processing.\n");
+	if (!(dev->flags & VIRTIO_DEV_RUNNING))
+		VHOST_LOG_CONFIG(INFO,
+			"virtio is now ready for processing.\n");
 	return 1;
 }
 
@@ -1970,8 +1989,6 @@  static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 	struct virtio_net *dev = *pdev;
 	int enable = (int)msg->payload.state.num;
 	int index = (int)msg->payload.state.index;
-	struct rte_vdpa_device *vdpa_dev;
-	int did = -1;
 
 	if (validate_msg_fds(msg, 0) != 0)
 		return RTE_VHOST_MSG_RESULT_ERR;
@@ -1980,15 +1997,6 @@  static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 		"set queue enable: %d to qp idx: %d\n",
 		enable, index);
 
-	did = dev->vdpa_dev_id;
-	vdpa_dev = rte_vdpa_get_device(did);
-	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
-		vdpa_dev->ops->set_vring_state(dev->vid, index, enable);
-
-	if (dev->notify_ops->vring_state_changed)
-		dev->notify_ops->vring_state_changed(dev->vid,
-				index, enable);
-
 	/* On disable, rings have to be stopped being processed. */
 	if (!enable && dev->dequeue_zero_copy)
 		drain_zmbuf_list(dev->virtqueue[index]);
@@ -2618,6 +2626,7 @@  typedef int (*vhost_message_handler_t)(struct virtio_net **pdev,
 	int unlock_required = 0;
 	bool handled;
 	int request;
+	uint32_t i;
 
 	dev = get_device(vid);
 	if (dev == NULL)
@@ -2793,6 +2802,17 @@  typedef int (*vhost_message_handler_t)(struct virtio_net **pdev,
 		return -1;
 	}
 
+	for (i = 0; i < dev->nr_vring; i++) {
+		struct vhost_virtqueue *vq = dev->virtqueue[i];
+		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;
+		}
+	}
+
+
 	if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) {
 		dev->flags |= VIRTIO_DEV_READY;
 
@@ -2810,8 +2830,7 @@  typedef int (*vhost_message_handler_t)(struct virtio_net **pdev,
 	did = dev->vdpa_dev_id;
 	vdpa_dev = rte_vdpa_get_device(did);
 	if (vdpa_dev && virtio_is_ready(dev) &&
-			!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) &&
-			msg.request.master == VHOST_USER_SET_VRING_CALL) {
+	    !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
 		if (vdpa_dev->ops->dev_conf)
 			vdpa_dev->ops->dev_conf(dev->vid);
 		dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;