diff mbox series

[v3] vhost: add config change slave msg support

Message ID 20191205053833.29068-1-fengli@smartx.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series [v3] vhost: add config change slave msg support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance fail Performance Testing issues
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot warning Travis build: failed

Commit Message

Li Feng Dec. 5, 2019, 5:38 a.m. UTC 
  This msg is used to notify qemu that should get the config of backend.

For example, vhost-user-blk uses this msg to notify guest os the
compacity of backend has changed.

Signed-off-by: Li Feng <fengli@smartx.com>
---
v3:
* Move the declare to rte_vhost.h
* Add the symbol in rte_vhost_version.map

v2:
* Fix a little log typo.

 lib/librte_vhost/rte_vhost.h           | 12 ++++++++++++
 lib/librte_vhost/rte_vhost_version.map |  1 +
 lib/librte_vhost/vhost_user.c          | 31 +++++++++++++++++++++++++++++++
 lib/librte_vhost/vhost_user.h          |  1 +
 4 files changed, 45 insertions(+)

Comments

Li Feng Dec. 11, 2019, 2:58 a.m. UTC | #1 
ping, any update about this?

Thanks,

Feng Li

Li Feng <fengli@smartx.com> 于2019年12月5日周四 下午1:38写道：
>
> This msg is used to notify qemu that should get the config of backend.
>
> For example, vhost-user-blk uses this msg to notify guest os the
> compacity of backend has changed.
>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v3:
> * Move the declare to rte_vhost.h
> * Add the symbol in rte_vhost_version.map
>
> v2:
> * Fix a little log typo.
>
>  lib/librte_vhost/rte_vhost.h           | 12 ++++++++++++
>  lib/librte_vhost/rte_vhost_version.map |  1 +
>  lib/librte_vhost/vhost_user.c          | 31 +++++++++++++++++++++++++++++++
>  lib/librte_vhost/vhost_user.h          |  1 +
>  4 files changed, 45 insertions(+)
>
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index 7b5dc87c2..fc28da264 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -977,6 +977,18 @@ __rte_experimental
>  int
>  rte_vhost_get_vdpa_device_id(int vid);
>
> +/**
> + * Notify the guest that should get config from backend.
> + *
> + * @param vid
> + *  vhost device ID
> + * @return
> + *  0 on success, < 0 on failure
> + */
> +__rte_experimental
> +int
> +rte_vhost_user_slave_config_change(int vid);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
> index c512377fe..acf013d6d 100644
> --- a/lib/librte_vhost/rte_vhost_version.map
> +++ b/lib/librte_vhost/rte_vhost_version.map
> @@ -65,4 +65,5 @@ EXPERIMENTAL {
>         rte_vhost_clr_inflight_desc_packed;
>         rte_vhost_get_vhost_ring_inflight;
>         rte_vhost_get_vring_base_from_inflight;
> +       rte_vhost_user_slave_config_change;
>  };
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 0cfb8b792..10f2e47d5 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -2840,6 +2840,37 @@ vhost_user_iotlb_miss(struct virtio_net *dev, uint64_t iova, uint8_t perm)
>         return 0;
>  }
>
> +static int
> +vhost_user_slave_config_change(struct virtio_net *dev)
> +{
> +       int ret;
> +       struct VhostUserMsg msg = {
> +               .request.slave = VHOST_USER_SLAVE_CONFIG_CHANGE_MSG,
> +               .flags = VHOST_USER_VERSION,
> +               .size = 0,
> +       };
> +
> +       ret = send_vhost_message(dev->slave_req_fd, &msg);
> +       if (ret < 0) {
> +               RTE_LOG(ERR, VHOST_CONFIG,
> +                               "Failed to send config change (%d)\n",
> +                               ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +int
> +rte_vhost_user_slave_config_change(int vid)
> +{
> +       struct virtio_net *dev;
> +       dev = get_device(vid);
> +       if (!dev)
> +               return -ENODEV;
> +       return vhost_user_slave_config_change(dev);
> +}
> +
>  static int vhost_user_slave_set_vring_host_notifier(struct virtio_net *dev,
>                                                     int index, int fd,
>                                                     uint64_t offset,
> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> index 6563f7315..86c364a93 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -62,6 +62,7 @@ typedef enum VhostUserRequest {
>  typedef enum VhostUserSlaveRequest {
>         VHOST_USER_SLAVE_NONE = 0,
>         VHOST_USER_SLAVE_IOTLB_MSG = 1,
> +       VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
>         VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
>         VHOST_USER_SLAVE_MAX
>  } VhostUserSlaveRequest;
> --
> 2.11.0
>
Tiwei Bie Dec. 16, 2019, 5:06 a.m. UTC | #2 
On Thu, Dec 05, 2019 at 01:38:33PM +0800, Li Feng wrote:
> This msg is used to notify qemu that should get the config of backend.
> 
> For example, vhost-user-blk uses this msg to notify guest os the
> compacity of backend has changed.

capacity?

> 
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v3:
> * Move the declare to rte_vhost.h
> * Add the symbol in rte_vhost_version.map
> 
> v2:
> * Fix a little log typo.
> 
>  lib/librte_vhost/rte_vhost.h           | 12 ++++++++++++
>  lib/librte_vhost/rte_vhost_version.map |  1 +
>  lib/librte_vhost/vhost_user.c          | 31 +++++++++++++++++++++++++++++++
>  lib/librte_vhost/vhost_user.h          |  1 +
>  4 files changed, 45 insertions(+)
> 
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index 7b5dc87c2..fc28da264 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -977,6 +977,18 @@ __rte_experimental
>  int
>  rte_vhost_get_vdpa_device_id(int vid);
>  
> +/**
> + * Notify the guest that should get config from backend.
> + *
> + * @param vid
> + *  vhost device ID
> + * @return
> + *  0 on success, < 0 on failure
> + */
> +__rte_experimental
> +int
> +rte_vhost_user_slave_config_change(int vid);

Normally the prefix in vhost API is rte_vhost_
not rte_vhost_user_

> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
> index c512377fe..acf013d6d 100644
> --- a/lib/librte_vhost/rte_vhost_version.map
> +++ b/lib/librte_vhost/rte_vhost_version.map
> @@ -65,4 +65,5 @@ EXPERIMENTAL {
>  	rte_vhost_clr_inflight_desc_packed;
>  	rte_vhost_get_vhost_ring_inflight;
>  	rte_vhost_get_vring_base_from_inflight;
> +	rte_vhost_user_slave_config_change;
>  };
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 0cfb8b792..10f2e47d5 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -2840,6 +2840,37 @@ vhost_user_iotlb_miss(struct virtio_net *dev, uint64_t iova, uint8_t perm)
>  	return 0;
>  }
>  
> +static int
> +vhost_user_slave_config_change(struct virtio_net *dev)
> +{
> +	int ret;
> +	struct VhostUserMsg msg = {
> +		.request.slave = VHOST_USER_SLAVE_CONFIG_CHANGE_MSG,
> +		.flags = VHOST_USER_VERSION,

Will it be better to also set VHOST_USER_NEED_REPLY?

> +		.size = 0,
> +	};
> +
> +	ret = send_vhost_message(dev->slave_req_fd, &msg);
> +	if (ret < 0) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +				"Failed to send config change (%d)\n",
> +				ret);

Looks better to indent the code by 3 tabs.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int
> +rte_vhost_user_slave_config_change(int vid)
> +{
> +	struct virtio_net *dev;
> +	dev = get_device(vid);
> +	if (!dev)
> +		return -ENODEV;
> +	return vhost_user_slave_config_change(dev);
> +}

I'm wondering will it be better to provide a generic API
to allow external backends to send any slave messages?

> +
>  static int vhost_user_slave_set_vring_host_notifier(struct virtio_net *dev,
>  						    int index, int fd,
>  						    uint64_t offset,
> diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> index 6563f7315..86c364a93 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -62,6 +62,7 @@ typedef enum VhostUserRequest {
>  typedef enum VhostUserSlaveRequest {
>  	VHOST_USER_SLAVE_NONE = 0,
>  	VHOST_USER_SLAVE_IOTLB_MSG = 1,
> +	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
>  	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
>  	VHOST_USER_SLAVE_MAX
>  } VhostUserSlaveRequest;
> -- 
> 2.11.0
> 
> 
> -- 
> The SmartX email address is only for business purpose. Any sent message 
> that is not related to the business is not authorized or permitted by 
> SmartX.
> 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> 
>
Li Feng Dec. 18, 2019, 6:34 a.m. UTC | #3 
Thanks.

Tiwei Bie <tiwei.bie@intel.com> 于2019年12月16日周一 下午1:05写道：
>
> On Thu, Dec 05, 2019 at 01:38:33PM +0800, Li Feng wrote:
> > This msg is used to notify qemu that should get the config of backend.
> >
> > For example, vhost-user-blk uses this msg to notify guest os the
> > compacity of backend has changed.
>
> capacity?
>
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v3:
> > * Move the declare to rte_vhost.h
> > * Add the symbol in rte_vhost_version.map
> >
> > v2:
> > * Fix a little log typo.
> >
> >  lib/librte_vhost/rte_vhost.h           | 12 ++++++++++++
> >  lib/librte_vhost/rte_vhost_version.map |  1 +
> >  lib/librte_vhost/vhost_user.c          | 31 +++++++++++++++++++++++++++++++
> >  lib/librte_vhost/vhost_user.h          |  1 +
> >  4 files changed, 45 insertions(+)
> >
> > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> > index 7b5dc87c2..fc28da264 100644
> > --- a/lib/librte_vhost/rte_vhost.h
> > +++ b/lib/librte_vhost/rte_vhost.h
> > @@ -977,6 +977,18 @@ __rte_experimental
> >  int
> >  rte_vhost_get_vdpa_device_id(int vid);
> >
> > +/**
> > + * Notify the guest that should get config from backend.
> > + *
> > + * @param vid
> > + *  vhost device ID
> > + * @return
> > + *  0 on success, < 0 on failure
> > + */
> > +__rte_experimental
> > +int
> > +rte_vhost_user_slave_config_change(int vid);
>
> Normally the prefix in vhost API is rte_vhost_
> not rte_vhost_user_
>
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
> > index c512377fe..acf013d6d 100644
> > --- a/lib/librte_vhost/rte_vhost_version.map
> > +++ b/lib/librte_vhost/rte_vhost_version.map
> > @@ -65,4 +65,5 @@ EXPERIMENTAL {
> >       rte_vhost_clr_inflight_desc_packed;
> >       rte_vhost_get_vhost_ring_inflight;
> >       rte_vhost_get_vring_base_from_inflight;
> > +     rte_vhost_user_slave_config_change;
> >  };
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 0cfb8b792..10f2e47d5 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -2840,6 +2840,37 @@ vhost_user_iotlb_miss(struct virtio_net *dev, uint64_t iova, uint8_t perm)
> >       return 0;
> >  }
> >
> > +static int
> > +vhost_user_slave_config_change(struct virtio_net *dev)
> > +{
> > +     int ret;
> > +     struct VhostUserMsg msg = {
> > +             .request.slave = VHOST_USER_SLAVE_CONFIG_CHANGE_MSG,
> > +             .flags = VHOST_USER_VERSION,
>
> Will it be better to also set VHOST_USER_NEED_REPLY?
This feature could not be supported because the vhost thread will
block at the read slave fd,
but qemu have sent a get_config message, and wait for the vhost thread
response, the stack like this:

#2  0x0000555555b435d2 in tcp_chr_recv (chr=chr@entry=0x555556b12c80,
buf=buf@entry=0x7fffffffd7c0 "\030", len=len@entry=12) at
chardev/char-socket.c:332
#3  0x0000555555b44611 in tcp_chr_sync_read (chr=0x555556b12c80,
buf=0x7fffffffd7c0 "\030", len=12) at chardev/char-socket.c:543
#4  0x0000555555b3f5e1 in qemu_chr_fe_read_all
(be=be@entry=0x555557e82310, buf=buf@entry=0x7fffffffd7c0 "\030",
len=len@entry=12) at chardev/char-fe.c:72
#5  0x0000555555884b7b in vhost_user_read_header (dev=0x555557e82310,
msg=0x7fffffffd7c0) at
/usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:236
#6  vhost_user_read (msg=msg@entry=0x7fffffffd7c0, dev=0x555557e82398)
at /usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:261
#7  0x00005555558850ff in vhost_user_get_config (dev=0x555557e82398,
config=0x7fffffffda80 "\340(6WUU", config_len=60)
    at /usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:1636
#8  0x0000555555842880 in vhost_user_blk_handle_config_change
(dev=0x555557e82398) at
/usr/src/debug/qemu-4.1.0/hw/block/vhost-user-blk.c:85
#9  0x0000555555886df1 in vhost_user_slave_handle_config_change
(dev=0x555557e82398) at
/usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:912
#10 slave_read (opaque=0x555557e82398) at
/usr/src/debug/qemu-4.1.0/hw/virtio/vhost-user.c:1050

>
> > +             .size = 0,
> > +     };
> > +
> > +     ret = send_vhost_message(dev->slave_req_fd, &msg);
> > +     if (ret < 0) {
> > +             RTE_LOG(ERR, VHOST_CONFIG,
> > +                             "Failed to send config change (%d)\n",
> > +                             ret);
>
> Looks better to indent the code by 3 tabs.
>
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +int
> > +rte_vhost_user_slave_config_change(int vid)
> > +{
> > +     struct virtio_net *dev;
> > +     dev = get_device(vid);
> > +     if (!dev)
> > +             return -ENODEV;
> > +     return vhost_user_slave_config_change(dev);
> > +}
>
> I'm wondering will it be better to provide a generic API
> to allow external backends to send any slave messages?
I have thought about this before, and I don't choose external backend because:
1. Sending slave message is more like an API, and external backend
acts like a hooker that receives messages from master;
If adding a new slave message type, we should also need to add type
definition in the dpdk librte_vhost header.
2. There is only three slave message type;


>
> > +
> >  static int vhost_user_slave_set_vring_host_notifier(struct virtio_net *dev,
> >                                                   int index, int fd,
> >                                                   uint64_t offset,
> > diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> > index 6563f7315..86c364a93 100644
> > --- a/lib/librte_vhost/vhost_user.h
> > +++ b/lib/librte_vhost/vhost_user.h
> > @@ -62,6 +62,7 @@ typedef enum VhostUserRequest {
> >  typedef enum VhostUserSlaveRequest {
> >       VHOST_USER_SLAVE_NONE = 0,
> >       VHOST_USER_SLAVE_IOTLB_MSG = 1,
> > +     VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
> >       VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
> >       VHOST_USER_SLAVE_MAX
> >  } VhostUserSlaveRequest;
> > --
> > 2.11.0
> >
> >
> > --
> > The SmartX email address is only for business purpose. Any sent message
> > that is not related to the business is not authorized or permitted by
> > SmartX.
> > 本邮箱为北京志凌海纳科技有限公司（SmartX）工作邮箱. 如本邮箱发出的邮件与工作无关,该邮件未得到本公司任何的明示或默示的授权.
> >
> >
diff mbox series

Patch

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index 7b5dc87c2..fc28da264 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -977,6 +977,18 @@  __rte_experimental
 int
 rte_vhost_get_vdpa_device_id(int vid);
 
+/**
+ * Notify the guest that should get config from backend.
+ *
+ * @param vid
+ *  vhost device ID
+ * @return
+ *  0 on success, < 0 on failure
+ */
+__rte_experimental
+int
+rte_vhost_user_slave_config_change(int vid);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index c512377fe..acf013d6d 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -65,4 +65,5 @@  EXPERIMENTAL {
 	rte_vhost_clr_inflight_desc_packed;
 	rte_vhost_get_vhost_ring_inflight;
 	rte_vhost_get_vring_base_from_inflight;
+	rte_vhost_user_slave_config_change;
 };
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 0cfb8b792..10f2e47d5 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2840,6 +2840,37 @@  vhost_user_iotlb_miss(struct virtio_net *dev, uint64_t iova, uint8_t perm)
 	return 0;
 }
 
+static int
+vhost_user_slave_config_change(struct virtio_net *dev)
+{
+	int ret;
+	struct VhostUserMsg msg = {
+		.request.slave = VHOST_USER_SLAVE_CONFIG_CHANGE_MSG,
+		.flags = VHOST_USER_VERSION,
+		.size = 0,
+	};
+
+	ret = send_vhost_message(dev->slave_req_fd, &msg);
+	if (ret < 0) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+				"Failed to send config change (%d)\n",
+				ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+int
+rte_vhost_user_slave_config_change(int vid)
+{
+	struct virtio_net *dev;
+	dev = get_device(vid);
+	if (!dev)
+		return -ENODEV;
+	return vhost_user_slave_config_change(dev);
+}
+
 static int vhost_user_slave_set_vring_host_notifier(struct virtio_net *dev,
 						    int index, int fd,
 						    uint64_t offset,
diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
index 6563f7315..86c364a93 100644
--- a/lib/librte_vhost/vhost_user.h
+++ b/lib/librte_vhost/vhost_user.h
@@ -62,6 +62,7 @@  typedef enum VhostUserRequest {
 typedef enum VhostUserSlaveRequest {
 	VHOST_USER_SLAVE_NONE = 0,
 	VHOST_USER_SLAVE_IOTLB_MSG = 1,
+	VHOST_USER_SLAVE_CONFIG_CHANGE_MSG = 2,
 	VHOST_USER_SLAVE_VRING_HOST_NOTIFIER_MSG = 3,
 	VHOST_USER_SLAVE_MAX
 } VhostUserSlaveRequest;