diff mbox series

[3/4] vhost: call vDPA callback at the end of vring enable handler

Message ID 1567740051-367172-3-git-send-email-andy.pei@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series [1/4] vhost: introduce new API to get the active vring number |

Checks

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

Commit Message

Pei, Andy Sept. 6, 2019, 3:20 a.m. UTC 
  vDPA's set_vring_state callback would need to know the virtqueues'
enable status to configure the hardware.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
Signed-off-by: Andy Pei <andy.pei@intel.com>
---
 lib/librte_vhost/rte_vdpa.h   | 4 ++--
 lib/librte_vhost/vhost_user.c | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Xiao Wang Sept. 12, 2019, 7:41 a.m. UTC | #1 
Hi Andy,

> -----Original Message-----
> From: Pei, Andy
> Sent: Friday, September 6, 2019 11:21 AM
> To: dev@dpdk.org
> Cc: Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Bie, Tiwei <tiwei.bie@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>
> Subject: [PATCH 3/4] vhost: call vDPA callback at the end of vring enable
> handler
> 
> vDPA's set_vring_state callback would need to know the virtqueues'
> enable status to configure the hardware.
> 
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
>  lib/librte_vhost/rte_vdpa.h   | 4 ++--
>  lib/librte_vhost/vhost_user.c | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
> index 9a3deb3..19cf897 100644
> --- a/lib/librte_vhost/rte_vdpa.h
> +++ b/lib/librte_vhost/rte_vdpa.h
> @@ -54,8 +54,8 @@ struct rte_vdpa_dev_ops {
>  	int (*dev_conf)(int vid);
>  	int (*dev_close)(int vid);
> 
> -	/** Enable/disable this vring */
> -	int (*set_vring_state)(int vid, int vring, int state);
> +	/** Enable/disable vring queue pairs */
> +	int (*set_vring_state)(int vid);

If rte_vhost_get_active_vring_num() always needs be called inside set_vring_state callback,
inorder to get the active queue pair number, then why not pass the active queue pair number
directly as a parameter to set_vring_state() ? This could help to avoid exposing new API.

BRs,
Xiao

> 
>  	/** Set features when changed */
>  	int (*set_features)(int vid);
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 0b72648..21028cc 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1346,8 +1346,6 @@ static int vhost_user_set_vring_err(struct virtio_net
> **pdev __rte_unused,
> 
>  	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,
> @@ -1359,6 +1357,9 @@ static int vhost_user_set_vring_err(struct virtio_net
> **pdev __rte_unused,
> 
>  	dev->virtqueue[index]->enabled = enable;
> 
> +	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> +		vdpa_dev->ops->set_vring_state(dev->vid);
> +
>  	return RTE_VHOST_MSG_RESULT_OK;
>  }
> 
> --
> 1.8.3.1
Pei, Andy Sept. 17, 2019, 7:29 a.m. UTC | #2 
Yes, I will do this way in v2.

-----Original Message-----
From: Wang, Xiao W 
Sent: Thursday, September 12, 2019 3:42 PM
To: Pei, Andy <andy.pei@intel.com>; dev@dpdk.org
Cc: Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>; Liang, Cunming <cunming.liang@intel.com>
Subject: RE: [PATCH 3/4] vhost: call vDPA callback at the end of vring enable handler

Hi Andy,

> -----Original Message-----
> From: Pei, Andy
> Sent: Friday, September 6, 2019 11:21 AM
> To: dev@dpdk.org
> Cc: Xu, Rosen <rosen.xu@intel.com>; Ye, Xiaolong 
> <xiaolong.ye@intel.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Xiao 
> W <xiao.w.wang@intel.com>
> Subject: [PATCH 3/4] vhost: call vDPA callback at the end of vring 
> enable handler
> 
> vDPA's set_vring_state callback would need to know the virtqueues'
> enable status to configure the hardware.
> 
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
>  lib/librte_vhost/rte_vdpa.h   | 4 ++--
>  lib/librte_vhost/vhost_user.c | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h 
> index 9a3deb3..19cf897 100644
> --- a/lib/librte_vhost/rte_vdpa.h
> +++ b/lib/librte_vhost/rte_vdpa.h
> @@ -54,8 +54,8 @@ struct rte_vdpa_dev_ops {
>  	int (*dev_conf)(int vid);
>  	int (*dev_close)(int vid);
> 
> -	/** Enable/disable this vring */
> -	int (*set_vring_state)(int vid, int vring, int state);
> +	/** Enable/disable vring queue pairs */
> +	int (*set_vring_state)(int vid);

If rte_vhost_get_active_vring_num() always needs be called inside set_vring_state callback, inorder to get the active queue pair number, then why not pass the active queue pair number directly as a parameter to set_vring_state() ? This could help to avoid exposing new API.

BRs,
Xiao

> 
>  	/** Set features when changed */
>  	int (*set_features)(int vid);
> diff --git a/lib/librte_vhost/vhost_user.c 
> b/lib/librte_vhost/vhost_user.c index 0b72648..21028cc 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1346,8 +1346,6 @@ static int vhost_user_set_vring_err(struct 
> virtio_net **pdev __rte_unused,
> 
>  	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,
> @@ -1359,6 +1357,9 @@ static int vhost_user_set_vring_err(struct 
> virtio_net **pdev __rte_unused,
> 
>  	dev->virtqueue[index]->enabled = enable;
> 
> +	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
> +		vdpa_dev->ops->set_vring_state(dev->vid);
> +
>  	return RTE_VHOST_MSG_RESULT_OK;
>  }
> 
> --
> 1.8.3.1
diff mbox series

Patch

diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index 9a3deb3..19cf897 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -54,8 +54,8 @@  struct rte_vdpa_dev_ops {
 	int (*dev_conf)(int vid);
 	int (*dev_close)(int vid);
 
-	/** Enable/disable this vring */
-	int (*set_vring_state)(int vid, int vring, int state);
+	/** Enable/disable vring queue pairs */
+	int (*set_vring_state)(int vid);
 
 	/** Set features when changed */
 	int (*set_features)(int vid);
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 0b72648..21028cc 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1346,8 +1346,6 @@  static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 
 	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,
@@ -1359,6 +1357,9 @@  static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 
 	dev->virtqueue[index]->enabled = enable;
 
+	if (vdpa_dev && vdpa_dev->ops->set_vring_state)
+		vdpa_dev->ops->set_vring_state(dev->vid);
+
 	return RTE_VHOST_MSG_RESULT_OK;
 }