[dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost unix domain socket

[dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost unix domain socket

[Huawei Xie]

rte_vhost_driver_unregister will remove the listenfd from event list, and then close it.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Signed-off-by: Peng Sun <peng.a.sun@intel.com>
---
 lib/librte_vhost/rte_virtio_net.h            |  3 ++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
 lib/librte_vhost/vhost_user/vhost-net-user.c | 70 +++++++++++++++++++++++-----
 lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
 4 files changed, 71 insertions(+), 13 deletions(-)

diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
index 5d38185..5630fbc 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct virtio_net *dev, uint16_t queue_i
 /* Register vhost driver. dev_name could be different for multiple instance support. */
 int rte_vhost_driver_register(const char *dev_name);
 
+/* Unregister vhost driver. This is only meaningful to vhost user. */
+int rte_vhost_driver_unregister(const char *dev_name);
+
 /* Register callbacks. */
 int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const);
 /* Start vhost driver session blocking loop. */
diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
index 6b68abf..1ae7c49 100644
--- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
+++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
@@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)
 }
 
 /**
+ * An empty function for unregister
+ */
+int
+rte_vhost_driver_unregister(const char *dev_name __rte_unused)
+{
+	return 0;
+}
+
+/**
  * The CUSE session is launched allowing the application to receive open,
  * release and ioctl calls.
  */
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index 31f1215..dff46ee 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -66,6 +66,8 @@ struct connfd_ctx {
 struct _vhost_server {
 	struct vhost_server *server[MAX_VHOST_SERVER];
 	struct fdset fdset;
+	int vserver_cnt;
+	pthread_mutex_t server_mutex;
 };
 
 static struct _vhost_server g_vhost_server = {
@@ -74,10 +76,10 @@ static struct _vhost_server g_vhost_server = {
 		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
 		.num = 0
 	},
+	.vserver_cnt = 0,
+	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
 };
 
-static int vserver_idx;
-
 static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_NONE] = "VHOST_USER_NONE",
 	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
@@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int *remove)
 	}
 }
 
-
 /**
  * Creates and initialise the vhost server.
  */
@@ -436,34 +437,79 @@ rte_vhost_driver_register(const char *path)
 {
 	struct vhost_server *vserver;
 
-	if (vserver_idx == 0)
+	pthread_mutex_lock(&g_vhost_server.server_mutex);
+	if (ops == NULL)
 		ops = get_virtio_net_callbacks();
-	if (vserver_idx == MAX_VHOST_SERVER)
+
+	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"error: the number of servers reaches maximum\n");
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
+	}
 
 	vserver = calloc(sizeof(struct vhost_server), 1);
-	if (vserver == NULL)
+	if (vserver == NULL) {
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
-
-	unlink(path);
+	}
 
 	vserver->listenfd = uds_socket(path);
 	if (vserver->listenfd < 0) {
 		free(vserver);
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
 	}
-	vserver->path = path;
+
+	vserver->path = strdup(path);
 
 	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
-		vserver_new_vq_conn, NULL,
-		vserver);
+		vserver_new_vq_conn, NULL, vserver);
 
-	g_vhost_server.server[vserver_idx++] = vserver;
+	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
+	pthread_mutex_unlock(&g_vhost_server.server_mutex);
 
 	return 0;
 }
 
 
+/**
+ * Unregister the specified vhost server
+ */
+int
+rte_vhost_driver_unregister(const char *path)
+{
+	int i;
+	int count;
+
+	pthread_mutex_lock(&g_vhost_server.server_mutex);
+
+	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
+		if (!strcmp(g_vhost_server.server[i]->path, path)) {
+			fdset_del(&g_vhost_server.fdset,
+				g_vhost_server.server[i]->listenfd);
+
+			close(g_vhost_server.server[i]->listenfd);
+			free(g_vhost_server.server[i]->path);
+			free(g_vhost_server.server[i]);
+
+			unlink(path);
+
+			count = --g_vhost_server.vserver_cnt;
+			g_vhost_server.server[i] =
+				g_vhost_server.server[count];
+			g_vhost_server.server[count] =
+				NULL;
+			pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+			return 0;
+		}
+	}
+	pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+	return -1;
+}
+
 int
 rte_vhost_driver_session_start(void)
 {
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h b/lib/librte_vhost/vhost_user/vhost-net-user.h
index 1b6be6c..2e72f3c 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.h
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
@@ -41,7 +41,7 @@
 #include "fd_man.h"
 
 struct vhost_server {
-	const char *path; /**< The path the uds is bind to. */
+	char *path; /**< The path the uds is bind to. */
 	int listenfd;     /**< The listener sockfd. */
 };
 
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost unix domain socket

[Loftus, Ciara]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Tuesday, June 02, 2015 2:50 AM
> To: dev@dpdk.org
> Cc: Sun, Peng A
> Subject: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost
> unix domain socket
> 
> rte_vhost_driver_unregister will remove the listenfd from event list, and
> then close it.
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
> ---
>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>  lib/librte_vhost/vhost_user/vhost-net-user.c | 70
> +++++++++++++++++++++++-----
>  lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
>  4 files changed, 71 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_virtio_net.h
> b/lib/librte_vhost/rte_virtio_net.h
> index 5d38185..5630fbc 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct
> virtio_net *dev, uint16_t queue_i
>  /* Register vhost driver. dev_name could be different for multiple instance
> support. */
>  int rte_vhost_driver_register(const char *dev_name);
> 
> +/* Unregister vhost driver. This is only meaningful to vhost user. */
> +int rte_vhost_driver_unregister(const char *dev_name);
> +
>  /* Register callbacks. */
>  int rte_vhost_driver_callback_register(struct virtio_net_device_ops const *
> const);
>  /* Start vhost driver session blocking loop. */
> diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> index 6b68abf..1ae7c49 100644
> --- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> +++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> @@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)
>  }
> 
>  /**
> + * An empty function for unregister
> + */
> +int
> +rte_vhost_driver_unregister(const char *dev_name __rte_unused)
> +{
> +	return 0;
> +}
> +
> +/**
>   * The CUSE session is launched allowing the application to receive open,
>   * release and ioctl calls.
>   */
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c
> b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index 31f1215..dff46ee 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -66,6 +66,8 @@ struct connfd_ctx {
>  struct _vhost_server {
>  	struct vhost_server *server[MAX_VHOST_SERVER];
>  	struct fdset fdset;
> +	int vserver_cnt;
> +	pthread_mutex_t server_mutex;
>  };
> 
>  static struct _vhost_server g_vhost_server = {
> @@ -74,10 +76,10 @@ static struct _vhost_server g_vhost_server = {
>  		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
>  		.num = 0
>  	},
> +	.vserver_cnt = 0,
> +	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
>  };
> 
> -static int vserver_idx;
> -
>  static const char *vhost_message_str[VHOST_USER_MAX] = {
>  	[VHOST_USER_NONE] = "VHOST_USER_NONE",
>  	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
> @@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int
> *remove)
>  	}
>  }
> 
> -
>  /**
>   * Creates and initialise the vhost server.
>   */
> @@ -436,34 +437,79 @@ rte_vhost_driver_register(const char *path)
>  {
>  	struct vhost_server *vserver;
> 
> -	if (vserver_idx == 0)
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +	if (ops == NULL)
>  		ops = get_virtio_net_callbacks();
> -	if (vserver_idx == MAX_VHOST_SERVER)
> +
> +	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"error: the number of servers reaches maximum\n");
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
> +	}
> 
>  	vserver = calloc(sizeof(struct vhost_server), 1);
> -	if (vserver == NULL)
> +	if (vserver == NULL) {
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
> -
> -	unlink(path);
> +	}
> 
>  	vserver->listenfd = uds_socket(path);
>  	if (vserver->listenfd < 0) {
>  		free(vserver);
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
>  	}
> -	vserver->path = path;
> +
> +	vserver->path = strdup(path);
> 
>  	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
> -		vserver_new_vq_conn, NULL,
> -		vserver);
> +		vserver_new_vq_conn, NULL, vserver);
> 
> -	g_vhost_server.server[vserver_idx++] = vserver;
> +	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> 
>  	return 0;
>  }
> 
> 
> +/**
> + * Unregister the specified vhost server
> + */
> +int
> +rte_vhost_driver_unregister(const char *path)
> +{
> +	int i;
> +	int count;
> +
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +
> +	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
> +		if (!strcmp(g_vhost_server.server[i]->path, path)) {
> +			fdset_del(&g_vhost_server.fdset,
> +				g_vhost_server.server[i]->listenfd);
> +
> +			close(g_vhost_server.server[i]->listenfd);
> +			free(g_vhost_server.server[i]->path);
> +			free(g_vhost_server.server[i]);
> +
> +			unlink(path);
> +
> +			count = --g_vhost_server.vserver_cnt;
> +			g_vhost_server.server[i] =
> +				g_vhost_server.server[count];
> +			g_vhost_server.server[count] =
> +				NULL;
> +
> 	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +			return 0;
> +		}
> +	}
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +	return -1;
> +}
> +
>  int
>  rte_vhost_driver_session_start(void)
>  {
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h
> b/lib/librte_vhost/vhost_user/vhost-net-user.h
> index 1b6be6c..2e72f3c 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.h
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
> @@ -41,7 +41,7 @@
>  #include "fd_man.h"
> 
>  struct vhost_server {
> -	const char *path; /**< The path the uds is bind to. */
> +	char *path; /**< The path the uds is bind to. */
>  	int listenfd;     /**< The listener sockfd. */
>  };
> 
> --
> 1.8.1.4

Acked-by: Ciara Loftus <ciara.loftus@intel.com>

I have validated this and it works with dpdk vhost-user in OVS.

Thanks,
Ciara

Re: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost unix domain socket

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Tuesday, June 2, 2015 9:50 AM
> To: dev@dpdk.org
> Cc: Sun, Peng A
> Subject: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost
> unix domain socket
> 
> rte_vhost_driver_unregister will remove the listenfd from event list, and
> then close it.
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
> ---
>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
> lib/librte_vhost/vhost_user/vhost-net-user.c | 70
> +++++++++++++++++++++++-----  lib/librte_vhost/vhost_user/vhost-net-
> user.h |  2 +-
>  4 files changed, 71 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_virtio_net.h
> b/lib/librte_vhost/rte_virtio_net.h
> index 5d38185..5630fbc 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct
> virtio_net *dev, uint16_t queue_i
>  /* Register vhost driver. dev_name could be different for multiple instance
> support. */  int rte_vhost_driver_register(const char *dev_name);
> 
> +/* Unregister vhost driver. This is only meaningful to vhost user. */
> +int rte_vhost_driver_unregister(const char *dev_name);
> +
>  /* Register callbacks. */
>  int rte_vhost_driver_callback_register(struct virtio_net_device_ops const *
> const);
>  /* Start vhost driver session blocking loop. */ diff --git
> a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> index 6b68abf..1ae7c49 100644
> --- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> +++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> @@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)  }
> 
>  /**
> + * An empty function for unregister
> + */
> +int
> +rte_vhost_driver_unregister(const char *dev_name __rte_unused) {
> +	return 0;
> +}
> +
> +/**
>   * The CUSE session is launched allowing the application to receive open,
>   * release and ioctl calls.
>   */
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c
> b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index 31f1215..dff46ee 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -66,6 +66,8 @@ struct connfd_ctx {
>  struct _vhost_server {
>  	struct vhost_server *server[MAX_VHOST_SERVER];
>  	struct fdset fdset;
> +	int vserver_cnt;
> +	pthread_mutex_t server_mutex;
>  };
> 
>  static struct _vhost_server g_vhost_server = { @@ -74,10 +76,10 @@ static
> struct _vhost_server g_vhost_server = {
>  		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
>  		.num = 0
>  	},
> +	.vserver_cnt = 0,
> +	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
>  };
> 
> -static int vserver_idx;
> -
>  static const char *vhost_message_str[VHOST_USER_MAX] = {
>  	[VHOST_USER_NONE] = "VHOST_USER_NONE",
>  	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
> @@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int
> *remove)
>  	}
>  }
> 
> -
>  /**
>   * Creates and initialise the vhost server.
>   */
> @@ -436,34 +437,79 @@ rte_vhost_driver_register(const char *path)  {
>  	struct vhost_server *vserver;
> 
> -	if (vserver_idx == 0)
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +	if (ops == NULL)
>  		ops = get_virtio_net_callbacks();
> -	if (vserver_idx == MAX_VHOST_SERVER)
> +
> +	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"error: the number of servers reaches maximum\n");
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
> +	}
> 
>  	vserver = calloc(sizeof(struct vhost_server), 1);
> -	if (vserver == NULL)
> +	if (vserver == NULL) {
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
> -
> -	unlink(path);
> +	}
> 
>  	vserver->listenfd = uds_socket(path);
>  	if (vserver->listenfd < 0) {
>  		free(vserver);
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
>  	}
> -	vserver->path = path;
> +
> +	vserver->path = strdup(path);

Do we need check if there is existing server which has same path  with the new one?

> 
>  	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
> -		vserver_new_vq_conn, NULL,
> -		vserver);
> +		vserver_new_vq_conn, NULL, vserver);
> 
> -	g_vhost_server.server[vserver_idx++] = vserver;
> +	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> 
>  	return 0;
>  }
> 
> 
> +/**
> + * Unregister the specified vhost server  */ int
> +rte_vhost_driver_unregister(const char *path) {
> +	int i;
> +	int count;
> +
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +
> +	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
> +		if (!strcmp(g_vhost_server.server[i]->path, path)) {
> +			fdset_del(&g_vhost_server.fdset,
> +				g_vhost_server.server[i]->listenfd);
> +
> +			close(g_vhost_server.server[i]->listenfd);

Besides listenfd, do we need check other fd's status, all fds should be closed before driver unregistered. 

> +			free(g_vhost_server.server[i]->path);
> +			free(g_vhost_server.server[i]);
> +
> +			unlink(path);
> +
> +			count = --g_vhost_server.vserver_cnt;
> +			g_vhost_server.server[i] =
> +				g_vhost_server.server[count];

it is better to remove the unnecessary new line for easy to read, so does the next 2 lines

> +			g_vhost_server.server[count] =
> +				NULL;
> +
> 	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +			return 0;
> +		}
> +	}
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +	return -1;
> +}

Do we have test case cover this new function?

> +
>  int
>  rte_vhost_driver_session_start(void)
>  {
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h
> b/lib/librte_vhost/vhost_user/vhost-net-user.h
> index 1b6be6c..2e72f3c 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.h
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
> @@ -41,7 +41,7 @@
>  #include "fd_man.h"
> 
>  struct vhost_server {
> -	const char *path; /**< The path the uds is bind to. */
> +	char *path; /**< The path the uds is bind to. */
>  	int listenfd;     /**< The listener sockfd. */
>  };
> 
> --
> 1.8.1.4

Re: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost unix domain socket

[Xie, Huawei]

On 6/3/2015 8:55 PM, Ouyang, Changchun wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
>> Sent: Tuesday, June 2, 2015 9:50 AM
>> To: dev@dpdk.org
>> Cc: Sun, Peng A
>> Subject: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost
>> unix domain socket
>>
>> rte_vhost_driver_unregister will remove the listenfd from event list, and
>> then close it.
>>
>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
>> ---
>>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>> lib/librte_vhost/vhost_user/vhost-net-user.c | 70
>> +++++++++++++++++++++++-----  lib/librte_vhost/vhost_user/vhost-net-
>> user.h |  2 +-
>>  4 files changed, 71 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/librte_vhost/rte_virtio_net.h
>> b/lib/librte_vhost/rte_virtio_net.h
>> index 5d38185..5630fbc 100644
>> --- a/lib/librte_vhost/rte_virtio_net.h
>> +++ b/lib/librte_vhost/rte_virtio_net.h
>> @@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct
>> virtio_net *dev, uint16_t queue_i
>>  /* Register vhost driver. dev_name could be different for multiple instance
>> support. */  int rte_vhost_driver_register(const char *dev_name);
>>
>> +/* Unregister vhost driver. This is only meaningful to vhost user. */
>> +int rte_vhost_driver_unregister(const char *dev_name);
>> +
>>  /* Register callbacks. */
>>  int rte_vhost_driver_callback_register(struct virtio_net_device_ops const *
>> const);
>>  /* Start vhost driver session blocking loop. */ diff --git
>> a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
>> b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
>> index 6b68abf..1ae7c49 100644
>> --- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
>> +++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
>> @@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)  }
>>
>>  /**
>> + * An empty function for unregister
>> + */
>> +int
>> +rte_vhost_driver_unregister(const char *dev_name __rte_unused) {
>> +	return 0;
>> +}
>> +
>> +/**
>>   * The CUSE session is launched allowing the application to receive open,
>>   * release and ioctl calls.
>>   */
>> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c
>> b/lib/librte_vhost/vhost_user/vhost-net-user.c
>> index 31f1215..dff46ee 100644
>> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
>> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
>> @@ -66,6 +66,8 @@ struct connfd_ctx {
>>  struct _vhost_server {
>>  	struct vhost_server *server[MAX_VHOST_SERVER];
>>  	struct fdset fdset;
>> +	int vserver_cnt;
>> +	pthread_mutex_t server_mutex;
>>  };
>>
>>  static struct _vhost_server g_vhost_server = { @@ -74,10 +76,10 @@ static
>> struct _vhost_server g_vhost_server = {
>>  		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
>>  		.num = 0
>>  	},
>> +	.vserver_cnt = 0,
>> +	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
>>  };
>>
>> -static int vserver_idx;
>> -
>>  static const char *vhost_message_str[VHOST_USER_MAX] = {
>>  	[VHOST_USER_NONE] = "VHOST_USER_NONE",
>>  	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
>> @@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int
>> *remove)
>>  	}
>>  }
>>
>> -
>>  /**
>>   * Creates and initialise the vhost server.
>>   */
>> @@ -436,34 +437,79 @@ rte_vhost_driver_register(const char *path)  {
>>  	struct vhost_server *vserver;
>>
>> -	if (vserver_idx == 0)
>> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
>> +	if (ops == NULL)
>>  		ops = get_virtio_net_callbacks();
>> -	if (vserver_idx == MAX_VHOST_SERVER)
>> +
>> +	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
>> +		RTE_LOG(ERR, VHOST_CONFIG,
>> +			"error: the number of servers reaches maximum\n");
>> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>>  		return -1;
>> +	}
>>
>>  	vserver = calloc(sizeof(struct vhost_server), 1);
>> -	if (vserver == NULL)
>> +	if (vserver == NULL) {
>> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>>  		return -1;
>> -
>> -	unlink(path);
>> +	}
>>
>>  	vserver->listenfd = uds_socket(path);
>>  	if (vserver->listenfd < 0) {
>>  		free(vserver);
>> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>>  		return -1;
>>  	}
>> -	vserver->path = path;
>> +
>> +	vserver->path = strdup(path);
> Do we need check if there is existing server which has same path  with the new one?

This is considered. If there is existing server, binding will fail.
>
>>  	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
>> -		vserver_new_vq_conn, NULL,
>> -		vserver);
>> +		vserver_new_vq_conn, NULL, vserver);
>>
>> -	g_vhost_server.server[vserver_idx++] = vserver;
>> +	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
>> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
>>
>>  	return 0;
>>  }
>>
>>
>> +/**
>> + * Unregister the specified vhost server  */ int
>> +rte_vhost_driver_unregister(const char *path) {
>> +	int i;
>> +	int count;
>> +
>> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
>> +
>> +	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
>> +		if (!strcmp(g_vhost_server.server[i]->path, path)) {
>> +			fdset_del(&g_vhost_server.fdset,
>> +				g_vhost_server.server[i]->listenfd);
>> +
>> +			close(g_vhost_server.server[i]->listenfd);
> Besides listenfd, do we need check other fd's status, all fds should be closed before driver unregistered. 
No, we don't need. Why do we check other fds? Here we are to unregister 
the listenfd.
>
>> +			free(g_vhost_server.server[i]->path);
>> +			free(g_vhost_server.server[i]);
>> +
>> +			unlink(path);
>> +
>> +			count = --g_vhost_server.vserver_cnt;
>> +			g_vhost_server.server[i] =
>> +				g_vhost_server.server[count];
> it is better to remove the unnecessary new line for easy to read, so does the next 2 lines
Thanks.
>> +			g_vhost_server.server[count] =
>> +				NULL;
>> +
>> 	pthread_mutex_unlock(&g_vhost_server.server_mutex);
>> +
>> +			return 0;
>> +		}
>> +	}
>> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
>> +
>> +	return -1;
>> +}
> Do we have test case cover this new function?
>
>> +
>>  int
>>  rte_vhost_driver_session_start(void)
>>  {
>> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h
>> b/lib/librte_vhost/vhost_user/vhost-net-user.h
>> index 1b6be6c..2e72f3c 100644
>> --- a/lib/librte_vhost/vhost_user/vhost-net-user.h
>> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
>> @@ -41,7 +41,7 @@
>>  #include "fd_man.h"
>>
>>  struct vhost_server {
>> -	const char *path; /**< The path the uds is bind to. */
>> +	char *path; /**< The path the uds is bind to. */
>>  	int listenfd;     /**< The listener sockfd. */
>>  };
>>
>> --
>> 1.8.1.4
>
>

[dpdk-dev] [PATCH v2] vhost: provide vhost API to unregister vhost unix domain socket

[Huawei Xie]

rte_vhost_driver_unregister will remove the listenfd from event list, and then close it.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Signed-off-by: Peng Sun <peng.a.sun@intel.com>
---
 lib/librte_vhost/rte_virtio_net.h            |  3 ++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
 lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
 lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
index 5d38185..5630fbc 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct virtio_net *dev, uint16_t queue_i
 /* Register vhost driver. dev_name could be different for multiple instance support. */
 int rte_vhost_driver_register(const char *dev_name);
 
+/* Unregister vhost driver. This is only meaningful to vhost user. */
+int rte_vhost_driver_unregister(const char *dev_name);
+
 /* Register callbacks. */
 int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const);
 /* Start vhost driver session blocking loop. */
diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
index 6b68abf..1ae7c49 100644
--- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
+++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
@@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)
 }
 
 /**
+ * An empty function for unregister
+ */
+int
+rte_vhost_driver_unregister(const char *dev_name __rte_unused)
+{
+	return 0;
+}
+
+/**
  * The CUSE session is launched allowing the application to receive open,
  * release and ioctl calls.
  */
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index 31f1215..87a4711 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -66,6 +66,8 @@ struct connfd_ctx {
 struct _vhost_server {
 	struct vhost_server *server[MAX_VHOST_SERVER];
 	struct fdset fdset;
+	int vserver_cnt;
+	pthread_mutex_t server_mutex;
 };
 
 static struct _vhost_server g_vhost_server = {
@@ -74,10 +76,10 @@ static struct _vhost_server g_vhost_server = {
 		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
 		.num = 0
 	},
+	.vserver_cnt = 0,
+	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
 };
 
-static int vserver_idx;
-
 static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_NONE] = "VHOST_USER_NONE",
 	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
@@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int *remove)
 	}
 }
 
-
 /**
  * Creates and initialise the vhost server.
  */
@@ -436,34 +437,77 @@ rte_vhost_driver_register(const char *path)
 {
 	struct vhost_server *vserver;
 
-	if (vserver_idx == 0)
+	pthread_mutex_lock(&g_vhost_server.server_mutex);
+	if (ops == NULL)
 		ops = get_virtio_net_callbacks();
-	if (vserver_idx == MAX_VHOST_SERVER)
+
+	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"error: the number of servers reaches maximum\n");
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
+	}
 
 	vserver = calloc(sizeof(struct vhost_server), 1);
-	if (vserver == NULL)
+	if (vserver == NULL) {
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
-
-	unlink(path);
+	}
 
 	vserver->listenfd = uds_socket(path);
 	if (vserver->listenfd < 0) {
 		free(vserver);
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
 	}
-	vserver->path = path;
+
+	vserver->path = strdup(path);
 
 	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
-		vserver_new_vq_conn, NULL,
-		vserver);
+		vserver_new_vq_conn, NULL, vserver);
 
-	g_vhost_server.server[vserver_idx++] = vserver;
+	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
+	pthread_mutex_unlock(&g_vhost_server.server_mutex);
 
 	return 0;
 }
 
 
+/**
+ * Unregister the specified vhost server
+ */
+int
+rte_vhost_driver_unregister(const char *path)
+{
+	int i;
+	int count;
+
+	pthread_mutex_lock(&g_vhost_server.server_mutex);
+
+	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
+		if (!strcmp(g_vhost_server.server[i]->path, path)) {
+			fdset_del(&g_vhost_server.fdset,
+				g_vhost_server.server[i]->listenfd);
+
+			close(g_vhost_server.server[i]->listenfd);
+			free(g_vhost_server.server[i]->path);
+			free(g_vhost_server.server[i]);
+
+			unlink(path);
+
+			count = --g_vhost_server.vserver_cnt;
+			g_vhost_server.server[i] = g_vhost_server.server[count];
+			g_vhost_server.server[count] = NULL;
+			pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+			return 0;
+		}
+	}
+	pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+	return -1;
+}
+
 int
 rte_vhost_driver_session_start(void)
 {
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h b/lib/librte_vhost/vhost_user/vhost-net-user.h
index 1b6be6c..2e72f3c 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.h
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
@@ -41,7 +41,7 @@
 #include "fd_man.h"
 
 struct vhost_server {
-	const char *path; /**< The path the uds is bind to. */
+	char *path; /**< The path the uds is bind to. */
 	int listenfd;     /**< The listener sockfd. */
 };
 
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v2] vhost: provide vhost API to unregister vhost unix domain socket

[Loftus, Ciara]

> -----Original Message-----
> From: Xie, Huawei
> Sent: Friday, June 05, 2015 4:26 AM
> To: dev@dpdk.org
> Cc: Loftus, Ciara; Xie, Huawei; Sun, Peng A
> Subject: [PATCH v2] vhost: provide vhost API to unregister vhost unix domain
> socket
> 
> rte_vhost_driver_unregister will remove the listenfd from event list, and
> then close it.
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
> ---
>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>  lib/librte_vhost/vhost_user/vhost-net-user.c | 68
> +++++++++++++++++++++++-----
>  lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
>  4 files changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_virtio_net.h
> b/lib/librte_vhost/rte_virtio_net.h
> index 5d38185..5630fbc 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct
> virtio_net *dev, uint16_t queue_i
>  /* Register vhost driver. dev_name could be different for multiple instance
> support. */
>  int rte_vhost_driver_register(const char *dev_name);
> 
> +/* Unregister vhost driver. This is only meaningful to vhost user. */
> +int rte_vhost_driver_unregister(const char *dev_name);
> +
>  /* Register callbacks. */
>  int rte_vhost_driver_callback_register(struct virtio_net_device_ops const *
> const);
>  /* Start vhost driver session blocking loop. */
> diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> index 6b68abf..1ae7c49 100644
> --- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> +++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> @@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)
>  }
> 
>  /**
> + * An empty function for unregister
> + */
> +int
> +rte_vhost_driver_unregister(const char *dev_name __rte_unused)
> +{
> +	return 0;
> +}
> +
> +/**
>   * The CUSE session is launched allowing the application to receive open,
>   * release and ioctl calls.
>   */
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c
> b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index 31f1215..87a4711 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -66,6 +66,8 @@ struct connfd_ctx {
>  struct _vhost_server {
>  	struct vhost_server *server[MAX_VHOST_SERVER];
>  	struct fdset fdset;
> +	int vserver_cnt;
> +	pthread_mutex_t server_mutex;
>  };
> 
>  static struct _vhost_server g_vhost_server = {
> @@ -74,10 +76,10 @@ static struct _vhost_server g_vhost_server = {
>  		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
>  		.num = 0
>  	},
> +	.vserver_cnt = 0,
> +	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
>  };
> 
> -static int vserver_idx;
> -
>  static const char *vhost_message_str[VHOST_USER_MAX] = {
>  	[VHOST_USER_NONE] = "VHOST_USER_NONE",
>  	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
> @@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int
> *remove)
>  	}
>  }
> 
> -
>  /**
>   * Creates and initialise the vhost server.
>   */
> @@ -436,34 +437,77 @@ rte_vhost_driver_register(const char *path)
>  {
>  	struct vhost_server *vserver;
> 
> -	if (vserver_idx == 0)
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +	if (ops == NULL)
>  		ops = get_virtio_net_callbacks();
> -	if (vserver_idx == MAX_VHOST_SERVER)
> +
> +	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"error: the number of servers reaches maximum\n");
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
> +	}
> 
>  	vserver = calloc(sizeof(struct vhost_server), 1);
> -	if (vserver == NULL)
> +	if (vserver == NULL) {
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
> -
> -	unlink(path);
> +	}
> 
>  	vserver->listenfd = uds_socket(path);
>  	if (vserver->listenfd < 0) {
>  		free(vserver);
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
>  	}
> -	vserver->path = path;
> +
> +	vserver->path = strdup(path);
> 
>  	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
> -		vserver_new_vq_conn, NULL,
> -		vserver);
> +		vserver_new_vq_conn, NULL, vserver);
> 
> -	g_vhost_server.server[vserver_idx++] = vserver;
> +	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> 
>  	return 0;
>  }
> 
> 
> +/**
> + * Unregister the specified vhost server
> + */
> +int
> +rte_vhost_driver_unregister(const char *path)
> +{
> +	int i;
> +	int count;
> +
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +
> +	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
> +		if (!strcmp(g_vhost_server.server[i]->path, path)) {
> +			fdset_del(&g_vhost_server.fdset,
> +				g_vhost_server.server[i]->listenfd);
> +
> +			close(g_vhost_server.server[i]->listenfd);
> +			free(g_vhost_server.server[i]->path);
> +			free(g_vhost_server.server[i]);
> +
> +			unlink(path);
> +
> +			count = --g_vhost_server.vserver_cnt;
> +			g_vhost_server.server[i] =
> g_vhost_server.server[count];
> +			g_vhost_server.server[count] = NULL;
> +
> 	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +			return 0;
> +		}
> +	}
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +	return -1;
> +}
> +
>  int
>  rte_vhost_driver_session_start(void)
>  {
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h
> b/lib/librte_vhost/vhost_user/vhost-net-user.h
> index 1b6be6c..2e72f3c 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.h
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
> @@ -41,7 +41,7 @@
>  #include "fd_man.h"
> 
>  struct vhost_server {
> -	const char *path; /**< The path the uds is bind to. */
> +	char *path; /**< The path the uds is bind to. */
>  	int listenfd;     /**< The listener sockfd. */
>  };
> 
> --
> 1.8.1.4

Acked-by: Ciara Loftus <ciara.loftus@intel.com>

Re: [dpdk-dev] [PATCH v2] vhost: provide vhost API to unregister vhost unix domain socket

[Xie, Huawei]

On 6/5/2015 5:04 PM, Loftus, Ciara wrote:
>
>> -----Original Message-----
>> From: Xie, Huawei
>> Sent: Friday, June 05, 2015 4:26 AM
>> To: dev@dpdk.org
>> Cc: Loftus, Ciara; Xie, Huawei; Sun, Peng A
>> Subject: [PATCH v2] vhost: provide vhost API to unregister vhost unix domain
>> socket
>>
>> rte_vhost_driver_unregister will remove the listenfd from event list, and
>> then close it.
>>
>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
>> ---
>>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>>  lib/librte_vhost/vhost_user/vhost-net-user.c | 68
>> +++++++++++++++++++++++-----
>>  lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
>>  4 files changed, 69 insertions(+), 13 deletions(-)
>>
>>
> Acked-by: Ciara Loftus <ciara.loftus@intel.com>
>
>
>
Thomas:
Comments to this patch?
This patch will remove the socket file and associated listen fd.
In future, I would also look at whether there is opportunity to attach a
id to each vhost user net interface from QEMU.
Currently DPDK OVS creates a socket file  for each virtio device and use
the file path as the id for the port.

/huawei

Re: [dpdk-dev] [PATCH v2] vhost: provide vhost API to unregister vhost unix domain socket

[Thomas F Herbert]

On 6/8/15 11:38 AM, Xie, Huawei wrote:
> On 6/5/2015 5:04 PM, Loftus, Ciara wrote:
>>
>>> -----Original Message-----
>>> From: Xie, Huawei
>>> Sent: Friday, June 05, 2015 4:26 AM
>>> To: dev@dpdk.org
>>> Cc: Loftus, Ciara; Xie, Huawei; Sun, Peng A
>>> Subject: [PATCH v2] vhost: provide vhost API to unregister vhost unix domain
>>> socket
>>>
>>> rte_vhost_driver_unregister will remove the listenfd from event list, and
>>> then close it.
>>>
>>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>>> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
>>> ---
>>>   lib/librte_vhost/rte_virtio_net.h            |  3 ++
>>>   lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>>>   lib/librte_vhost/vhost_user/vhost-net-user.c | 68
>>> +++++++++++++++++++++++-----
>>>   lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
>>>   4 files changed, 69 insertions(+), 13 deletions(-)
>>>
>>>
>> Acked-by: Ciara Loftus <ciara.loftus@intel.com>
>>
>>
>>
> Thomas:
> Comments to this patch?
After reading the patch, it looks straight forward. I want to compile 
and run OVS/DPDK with vhost-user patch linked with DPDK with this patch 
applied first. I will respond when that is complete.
> This patch will remove the socket file and associated listen fd.
> In future, I would also look at whether there is opportunity to attach a
> id to each vhost user net interface from QEMU.
> Currently DPDK OVS creates a socket file  for each virtio device and use
> the file path as the id for the port.
>
> /huawei
>
>

Re: [dpdk-dev] [PATCH v2] vhost: provide vhost API to unregister vhost unix domain socket

[Xie, Huawei]

On 6/5/2015 5:04 PM, Loftus, Ciara wrote:
>
>> -----Original Message-----
>> From: Xie, Huawei
>> Sent: Friday, June 05, 2015 4:26 AM
>> To: dev@dpdk.org
>> Cc: Loftus, Ciara; Xie, Huawei; Sun, Peng A
>> Subject: [PATCH v2] vhost: provide vhost API to unregister vhost unix domain
>> socket
>>
>> rte_vhost_driver_unregister will remove the listenfd from event list, and
>> then close it.
>>
>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
>> ---
>>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>>  lib/librte_vhost/vhost_user/vhost-net-user.c | 68
>> +++++++++++++++++++++++-----
>>  lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
>>  4 files changed, 69 insertions(+), 13 deletions(-)
>>
[...]
> Acked-by: Ciara Loftus <ciara.loftus@intel.com>
Thomas:
Comments to  this patch?

Re: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost unix domain socket

[Tetsuya Mukawa]

On 2015/06/02 10:50, Huawei Xie wrote:
> rte_vhost_driver_unregister will remove the listenfd from event list, and then close it.
>
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
> ---
>  
> +/**
> + * Unregister the specified vhost server
> + */
> +int
> +rte_vhost_driver_unregister(const char *path)
> +{
> +	int i;
> +	int count;
> +
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +
> +	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
> +		if (!strcmp(g_vhost_server.server[i]->path, path)) {
> +			fdset_del(&g_vhost_server.fdset,
> +				g_vhost_server.server[i]->listenfd);
> +
> +			close(g_vhost_server.server[i]->listenfd);
> +			free(g_vhost_server.server[i]->path);
> +			free(g_vhost_server.server[i]);
> +
> +			unlink(path);
> +
> +			count = --g_vhost_server.vserver_cnt;
> +			g_vhost_server.server[i] =
> +				g_vhost_server.server[count];
> +			g_vhost_server.server[count] =
> +				NULL;
> +			pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +			return 0;
> +		}
> +	}
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +	return -1;
> +}
> +
>

Hi Xie,

It seems vserver_cnt is incremented when socket is registered, and
decremented when unregistered.
And this value is used for index value of g_vhost_server.server[ ], when
a new socket is registered.
So I have a question about below case.

Step1. socket0 is registered.
Step2: scoekt1 is registered.
Step3. socket0 is unregistered.
Step4. socket2 is registered.

After above steps, are socket1 and socket2 still registered?

Thanks,
Tetsuya

Re: [dpdk-dev] [PATCH v2] vhost: provide vhost API to unregister vhost unix domain socket

[Panu Matilainen]

On 06/08/2015 06:38 PM, Xie, Huawei wrote:
> On 6/5/2015 5:04 PM, Loftus, Ciara wrote:
>>
>>> -----Original Message-----
>>> From: Xie, Huawei
>>> Sent: Friday, June 05, 2015 4:26 AM
>>> To: dev@dpdk.org
>>> Cc: Loftus, Ciara; Xie, Huawei; Sun, Peng A
>>> Subject: [PATCH v2] vhost: provide vhost API to unregister vhost unix domain
>>> socket
>>>
>>> rte_vhost_driver_unregister will remove the listenfd from event list, and
>>> then close it.
>>>
>>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>>> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
>>> ---
>>>   lib/librte_vhost/rte_virtio_net.h            |  3 ++
>>>   lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>>>   lib/librte_vhost/vhost_user/vhost-net-user.c | 68
>>> +++++++++++++++++++++++-----
>>>   lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
>>>   4 files changed, 69 insertions(+), 13 deletions(-)
>>>

You need to update the symbol version map when adding new public APIs. 
Something like:

--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -13,3 +13,11 @@ DPDK_2.0 {

         local: *;
  };
+
+DPDK_2.1 {
+       global:
+
+       rte_vhost_driver_unregister;
+
+       local: *;
+};

	- Panu -

Re: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost unix domain socket

[Xie, Huawei]

On 6/17/2015 12:17 PM, Tetsuya Mukawa wrote:
> On 2015/06/02 10:50, Huawei Xie wrote:
>> rte_vhost_driver_unregister will remove the listenfd from event list, and then close it.
>>
>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
>> ---
>>  
>> +/**
>> + * Unregister the specified vhost server
>> + */
>> +int
>> +rte_vhost_driver_unregister(const char *path)
>> +{
>> +	int i;
>> +	int count;
>> +
>> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
>> +
>> +	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
>> +		if (!strcmp(g_vhost_server.server[i]->path, path)) {
>> +			fdset_del(&g_vhost_server.fdset,
>> +				g_vhost_server.server[i]->listenfd);
>> +
>> +			close(g_vhost_server.server[i]->listenfd);
>> +			free(g_vhost_server.server[i]->path);
>> +			free(g_vhost_server.server[i]);
>> +
>> +			unlink(path);
>> +
>> +			count = --g_vhost_server.vserver_cnt;
>> +			g_vhost_server.server[i] =
>> +				g_vhost_server.server[count];
>> +			g_vhost_server.server[count] =
>> +				NULL;
>> +			pthread_mutex_unlock(&g_vhost_server.server_mutex);
>> +
>> +			return 0;
>> +		}
>> +	}
>> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
>> +
>> +	return -1;
>> +}
>> +
>>
> Hi Xie,
>
> It seems vserver_cnt is incremented when socket is registered, and
> decremented when unregistered.
> And this value is used for index value of g_vhost_server.server[ ], when
> a new socket is registered.
When we unregister a server at index x,  we will move the server at the
tail of the array to the location x.
> So I have a question about below case.
>
> Step1. socket0 is registered.
> Step2: scoekt1 is registered.
> Step3. socket0 is unregistered.
When socket0 is unregistered, socket1  will be moved to location at index 0.
> Step4. socket2 is registered.
socket2 is registered at index 1.
>
> After above steps, are socket1 and socket2 still registered?
>
> Thanks,
> Tetsuya
>
>
What is your concern here?

Re: [dpdk-dev] [PATCH v2] vhost: provide vhost API to unregister vhost unix domain socket

[Thomas Monjalon]

2015-06-05 11:26, Huawei Xie:
> rte_vhost_driver_unregister will remove the listenfd from event list, and then close it.
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
> ---
>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>  lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
>  lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-

A function is added to the API without update of the .map file?

Could it be used in a test or an example? It would prevent to break
it or forget to add it in the .map.

Re: [dpdk-dev] [PATCH] vhost: provide vhost API to unregister vhost unix domain socket

[Tetsuya Mukawa]

On 2015/06/17 20:05, Xie, Huawei wrote:
> On 6/17/2015 12:17 PM, Tetsuya Mukawa wrote:
>> On 2015/06/02 10:50, Huawei Xie wrote:
>>> rte_vhost_driver_unregister will remove the listenfd from event list, and then close it.
>>>
>>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>>> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
>>> ---
>>>  
>>> +/**
>>> + * Unregister the specified vhost server
>>> + */
>>> +int
>>> +rte_vhost_driver_unregister(const char *path)
>>> +{
>>> +	int i;
>>> +	int count;
>>> +
>>> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
>>> +
>>> +	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
>>> +		if (!strcmp(g_vhost_server.server[i]->path, path)) {
>>> +			fdset_del(&g_vhost_server.fdset,
>>> +				g_vhost_server.server[i]->listenfd);
>>> +
>>> +			close(g_vhost_server.server[i]->listenfd);
>>> +			free(g_vhost_server.server[i]->path);
>>> +			free(g_vhost_server.server[i]);
>>> +
>>> +			unlink(path);
>>> +
>>> +			count = --g_vhost_server.vserver_cnt;
>>> +			g_vhost_server.server[i] =
>>> +				g_vhost_server.server[count];
>>> +			g_vhost_server.server[count] =
>>> +				NULL;
>>> +			pthread_mutex_unlock(&g_vhost_server.server_mutex);
>>> +
>>> +			return 0;
>>> +		}
>>> +	}
>>> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
>>> +
>>> +	return -1;
>>> +}
>>> +
>>>
>> Hi Xie,
>>
>> It seems vserver_cnt is incremented when socket is registered, and
>> decremented when unregistered.
>> And this value is used for index value of g_vhost_server.server[ ], when
>> a new socket is registered.
> When we unregister a server at index x,  we will move the server at the
> tail of the array to the location x.
>> So I have a question about below case.
>>
>> Step1. socket0 is registered.
>> Step2: scoekt1 is registered.
>> Step3. socket0 is unregistered.
> When socket0 is unregistered, socket1  will be moved to location at index 0.

Thanks for explanation, I overlooked this behavior.
Now I don't have any concerns.

Thanks,
Tetsuya

Re: [dpdk-dev] [PATCH v2] vhost: provide vhost API to unregister vhost unix domain socket

[Xie, Huawei]

On 6/18/2015 5:02 AM, Thomas Monjalon wrote:
> 2015-06-05 11:26, Huawei Xie:
>> rte_vhost_driver_unregister will remove the listenfd from event list, and then close it.
>>
>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>> Signed-off-by: Peng Sun <peng.a.sun@intel.com>
>> ---
>>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>>  lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
>>  lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
> A function is added to the API without update of the .map file?
Get the comment from Panu. Would submit v3 patch.

>
> Could it be used in a test or an example? It would prevent to break
> it or forget to add it in the .map.
>
Now in vhost example, we only register one socket and never unregister it.
Would evaluate calling this API in vhost example in future.
>
>

[dpdk-dev] [PATCH v3 0/2] vhost: vhost unix domain socket cleanup

[Huawei Xie]

vhost user could register multiple unix domain socket server, and use the path to identify the virtio device connecting to it.
rte_vhost_driver_unregister will clean up the unix domain socket for the specified path.

Huawei Xie (2):
  vhost socket cleanup
  update version map file for rte_vhost_driver_unregister API

 lib/librte_vhost/rte_vhost_version.map       |  8 ++++
 lib/librte_vhost/rte_virtio_net.h            |  3 ++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
 lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
 lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
 5 files changed, 77 insertions(+), 13 deletions(-)

-- 
1.8.1.4

[dpdk-dev] [PATCH v3 1/2] vhost: vhost unix domain socket cleanup

[Huawei Xie]

rte_vhost_driver_unregister API will remove the listenfd for the specified path from event processing list, and then close it.

v2 changes:
-minor code style fix: remove unnecessary new line

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Signed-off-by: Peng Sun <peng.a.sun@intel.com>
---
 lib/librte_vhost/rte_virtio_net.h            |  3 ++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
 lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
 lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
index 5d38185..5630fbc 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct virtio_net *dev, uint16_t queue_i
 /* Register vhost driver. dev_name could be different for multiple instance support. */
 int rte_vhost_driver_register(const char *dev_name);
 
+/* Unregister vhost driver. This is only meaningful to vhost user. */
+int rte_vhost_driver_unregister(const char *dev_name);
+
 /* Register callbacks. */
 int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const);
 /* Start vhost driver session blocking loop. */
diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
index 6b68abf..1ae7c49 100644
--- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
+++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
@@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)
 }
 
 /**
+ * An empty function for unregister
+ */
+int
+rte_vhost_driver_unregister(const char *dev_name __rte_unused)
+{
+	return 0;
+}
+
+/**
  * The CUSE session is launched allowing the application to receive open,
  * release and ioctl calls.
  */
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index 31f1215..87a4711 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -66,6 +66,8 @@ struct connfd_ctx {
 struct _vhost_server {
 	struct vhost_server *server[MAX_VHOST_SERVER];
 	struct fdset fdset;
+	int vserver_cnt;
+	pthread_mutex_t server_mutex;
 };
 
 static struct _vhost_server g_vhost_server = {
@@ -74,10 +76,10 @@ static struct _vhost_server g_vhost_server = {
 		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
 		.num = 0
 	},
+	.vserver_cnt = 0,
+	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
 };
 
-static int vserver_idx;
-
 static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_NONE] = "VHOST_USER_NONE",
 	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
@@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int *remove)
 	}
 }
 
-
 /**
  * Creates and initialise the vhost server.
  */
@@ -436,34 +437,77 @@ rte_vhost_driver_register(const char *path)
 {
 	struct vhost_server *vserver;
 
-	if (vserver_idx == 0)
+	pthread_mutex_lock(&g_vhost_server.server_mutex);
+	if (ops == NULL)
 		ops = get_virtio_net_callbacks();
-	if (vserver_idx == MAX_VHOST_SERVER)
+
+	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"error: the number of servers reaches maximum\n");
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
+	}
 
 	vserver = calloc(sizeof(struct vhost_server), 1);
-	if (vserver == NULL)
+	if (vserver == NULL) {
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
-
-	unlink(path);
+	}
 
 	vserver->listenfd = uds_socket(path);
 	if (vserver->listenfd < 0) {
 		free(vserver);
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
 	}
-	vserver->path = path;
+
+	vserver->path = strdup(path);
 
 	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
-		vserver_new_vq_conn, NULL,
-		vserver);
+		vserver_new_vq_conn, NULL, vserver);
 
-	g_vhost_server.server[vserver_idx++] = vserver;
+	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
+	pthread_mutex_unlock(&g_vhost_server.server_mutex);
 
 	return 0;
 }
 
 
+/**
+ * Unregister the specified vhost server
+ */
+int
+rte_vhost_driver_unregister(const char *path)
+{
+	int i;
+	int count;
+
+	pthread_mutex_lock(&g_vhost_server.server_mutex);
+
+	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
+		if (!strcmp(g_vhost_server.server[i]->path, path)) {
+			fdset_del(&g_vhost_server.fdset,
+				g_vhost_server.server[i]->listenfd);
+
+			close(g_vhost_server.server[i]->listenfd);
+			free(g_vhost_server.server[i]->path);
+			free(g_vhost_server.server[i]);
+
+			unlink(path);
+
+			count = --g_vhost_server.vserver_cnt;
+			g_vhost_server.server[i] = g_vhost_server.server[count];
+			g_vhost_server.server[count] = NULL;
+			pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+			return 0;
+		}
+	}
+	pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+	return -1;
+}
+
 int
 rte_vhost_driver_session_start(void)
 {
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h b/lib/librte_vhost/vhost_user/vhost-net-user.h
index 1b6be6c..2e72f3c 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.h
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
@@ -41,7 +41,7 @@
 #include "fd_man.h"
 
 struct vhost_server {
-	const char *path; /**< The path the uds is bind to. */
+	char *path; /**< The path the uds is bind to. */
 	int listenfd;     /**< The listener sockfd. */
 };
 
-- 
1.8.1.4

[dpdk-dev] [PATCH v3 2/2] vhost: version map file update

[Huawei Xie]

update version map file for rte_vhost_driver_unregister API

v3 changes:
update version map file

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/rte_vhost_version.map | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 163dde0..fb6bb9e 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -13,3 +13,11 @@ DPDK_2.0 {
 
 	local: *;
 };
+
+DPDK_2.1 {
+	global:
+
+	rte_vhost_driver_unregister;
+
+	local: *;
+} DPDK_2.0;
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v3 1/2] vhost: vhost unix domain socket cleanup

[Xie, Huawei]

On 6/19/2015 1:40 AM, Huawei Xie wrote:

rte_vhost_driver_unregister API will remove the listenfd for the specified path from event processing list, and then close it.

v2 changes:
-minor code style fix: remove unnecessary new line

Signed-off-by: Huawei Xie <huawei.xie@intel.com><mailto:huawei.xie@intel.com>
Signed-off-by: Peng Sun <peng.a.sun@intel.com><mailto:peng.a.sun@intel.com>
---
 lib/librte_vhost/rte_virtio_net.h            |  3 ++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
 lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
 lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
index 5d38185..5630fbc 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct virtio_net *dev, uint16_t queue_i
 /* Register vhost driver. dev_name could be different for multiple instance support. */
 int rte_vhost_driver_register(const char *dev_name);

+/* Unregister vhost driver. This is only meaningful to vhost user. */
+int rte_vhost_driver_unregister(const char *dev_name);
+
 /* Register callbacks. */
 int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const);
 /* Start vhost driver session blocking loop. */
diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
index 6b68abf..1ae7c49 100644
--- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
+++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
@@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)
 }

 /**
+ * An empty function for unregister
+ */
+int
+rte_vhost_driver_unregister(const char *dev_name __rte_unused)
+{
+       return 0;
+}
+
+/**
  * The CUSE session is launched allowing the application to receive open,
  * release and ioctl calls.
  */
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index 31f1215..87a4711 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -66,6 +66,8 @@ struct connfd_ctx {
 struct _vhost_server {
        struct vhost_server *server[MAX_VHOST_SERVER];
        struct fdset fdset;
+       int vserver_cnt;
+       pthread_mutex_t server_mutex;
 };

 static struct _vhost_server g_vhost_server = {
@@ -74,10 +76,10 @@ static struct _vhost_server g_vhost_server = {
                .fd_mutex = PTHREAD_MUTEX_INITIALIZER,
                .num = 0
        },
+       .vserver_cnt = 0,
+       .server_mutex = PTHREAD_MUTEX_INITIALIZER,
 };

-static int vserver_idx;
-
 static const char *vhost_message_str[VHOST_USER_MAX] = {
        [VHOST_USER_NONE] = "VHOST_USER_NONE",
        [VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
@@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int *remove)
        }
 }

-
 /**
  * Creates and initialise the vhost server.
  */
@@ -436,34 +437,77 @@ rte_vhost_driver_register(const char *path)
 {
        struct vhost_server *vserver;

-       if (vserver_idx == 0)
+       pthread_mutex_lock(&g_vhost_server.server_mutex);
+       if (ops == NULL)
                ops = get_virtio_net_callbacks();
-       if (vserver_idx == MAX_VHOST_SERVER)
+
+       if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
+               RTE_LOG(ERR, VHOST_CONFIG,
+                       "error: the number of servers reaches maximum\n");
+               pthread_mutex_unlock(&g_vhost_server.server_mutex);
                return -1;
+       }

        vserver = calloc(sizeof(struct vhost_server), 1);
-       if (vserver == NULL)
+       if (vserver == NULL) {
+               pthread_mutex_unlock(&g_vhost_server.server_mutex);
                return -1;
-
-       unlink(path);
+       }

        vserver->listenfd = uds_socket(path);
        if (vserver->listenfd < 0) {
                free(vserver);
+               pthread_mutex_unlock(&g_vhost_server.server_mutex);
                return -1;
        }
-       vserver->path = path;
+
+       vserver->path = strdup(path);

        fdset_add(&g_vhost_server.fdset, vserver->listenfd,
-               vserver_new_vq_conn, NULL,
-               vserver);
+               vserver_new_vq_conn, NULL, vserver);


In fd_man.c, in the event handler for connection fd,  the fd could be closed when receives no data.
Before the following code snippet, as it isn't protected, there is chance we register the listenfd with the value of the just closed fd.
so the following fdset_del could wrongly remove the new listenfd.
would use fdset_del_slot to delete entry at fixed slot.

 if (remove1 || remove2)
                                fdset_del(pfdset, fd);


another thing is when select is blocked, rte_vhost_driver_unregister/register  could remove/refill entries of some listenfd(s!!!).
There is potential unwanted call on new listenfds, it is not a issue. would add comment to emphasize that.






-       g_vhost_server.server[vserver_idx++] = vserver;
+       g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
+       pthread_mutex_unlock(&g_vhost_server.server_mutex);

        return 0;
 }


+/**
+ * Unregister the specified vhost server
+ */
+int
+rte_vhost_driver_unregister(const char *path)
+{
+       int i;
+       int count;
+
+       pthread_mutex_lock(&g_vhost_server.server_mutex);
+
+       for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
+               if (!strcmp(g_vhost_server.server[i]->path, path)) {
+                       fdset_del(&g_vhost_server.fdset,
+                               g_vhost_server.server[i]->listenfd);
+
+                       close(g_vhost_server.server[i]->listenfd);
+                       free(g_vhost_server.server[i]->path);
+                       free(g_vhost_server.server[i]);
+
+                       unlink(path);
+
+                       count = --g_vhost_server.vserver_cnt;
+                       g_vhost_server.server[i] = g_vhost_server.server[count];
+                       g_vhost_server.server[count] = NULL;
+                       pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+                       return 0;
+               }
+       }
+       pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+       return -1;
+}
+
 int
 rte_vhost_driver_session_start(void)
 {
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h b/lib/librte_vhost/vhost_user/vhost-net-user.h
index 1b6be6c..2e72f3c 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.h
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
@@ -41,7 +41,7 @@
 #include "fd_man.h"

 struct vhost_server {
-       const char *path; /**< The path the uds is bind to. */
+       char *path; /**< The path the uds is bind to. */
        int listenfd;     /**< The listener sockfd. */
 };

Re: [dpdk-dev] [PATCH v3 1/2] vhost: vhost unix domain socket cleanup

[Thomas Monjalon]

Huawei,
I don't understand this reply. You forgot quoting, you didn't remove useless lines,
and you seem to reply to yourself.
Should this patch be applied?

2015-06-29 18:28, Xie, Huawei:
> On 6/19/2015 1:40 AM, Huawei Xie wrote:
> 
> rte_vhost_driver_unregister API will remove the listenfd for the specified path from event processing list, and then close it.
> 
> v2 changes:
> -minor code style fix: remove unnecessary new line
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com><mailto:huawei.xie@intel.com>
> Signed-off-by: Peng Sun <peng.a.sun@intel.com><mailto:peng.a.sun@intel.com>
> ---
>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
>  lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
>  lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
>  4 files changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
> index 5d38185..5630fbc 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct virtio_net *dev, uint16_t queue_i
>  /* Register vhost driver. dev_name could be different for multiple instance support. */
>  int rte_vhost_driver_register(const char *dev_name);
> 
> +/* Unregister vhost driver. This is only meaningful to vhost user. */
> +int rte_vhost_driver_unregister(const char *dev_name);
> +
>  /* Register callbacks. */
>  int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const);
>  /* Start vhost driver session blocking loop. */
> diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> index 6b68abf..1ae7c49 100644
> --- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> +++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> @@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)
>  }
> 
>  /**
> + * An empty function for unregister
> + */
> +int
> +rte_vhost_driver_unregister(const char *dev_name __rte_unused)
> +{
> +       return 0;
> +}
> +
> +/**
>   * The CUSE session is launched allowing the application to receive open,
>   * release and ioctl calls.
>   */
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index 31f1215..87a4711 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -66,6 +66,8 @@ struct connfd_ctx {
>  struct _vhost_server {
>         struct vhost_server *server[MAX_VHOST_SERVER];
>         struct fdset fdset;
> +       int vserver_cnt;
> +       pthread_mutex_t server_mutex;
>  };
> 
>  static struct _vhost_server g_vhost_server = {
> @@ -74,10 +76,10 @@ static struct _vhost_server g_vhost_server = {
>                 .fd_mutex = PTHREAD_MUTEX_INITIALIZER,
>                 .num = 0
>         },
> +       .vserver_cnt = 0,
> +       .server_mutex = PTHREAD_MUTEX_INITIALIZER,
>  };
> 
> -static int vserver_idx;
> -
>  static const char *vhost_message_str[VHOST_USER_MAX] = {
>         [VHOST_USER_NONE] = "VHOST_USER_NONE",
>         [VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
> @@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int *remove)
>         }
>  }
> 
> -
>  /**
>   * Creates and initialise the vhost server.
>   */
> @@ -436,34 +437,77 @@ rte_vhost_driver_register(const char *path)
>  {
>         struct vhost_server *vserver;
> 
> -       if (vserver_idx == 0)
> +       pthread_mutex_lock(&g_vhost_server.server_mutex);
> +       if (ops == NULL)
>                 ops = get_virtio_net_callbacks();
> -       if (vserver_idx == MAX_VHOST_SERVER)
> +
> +       if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
> +               RTE_LOG(ERR, VHOST_CONFIG,
> +                       "error: the number of servers reaches maximum\n");
> +               pthread_mutex_unlock(&g_vhost_server.server_mutex);
>                 return -1;
> +       }
> 
>         vserver = calloc(sizeof(struct vhost_server), 1);
> -       if (vserver == NULL)
> +       if (vserver == NULL) {
> +               pthread_mutex_unlock(&g_vhost_server.server_mutex);
>                 return -1;
> -
> -       unlink(path);
> +       }
> 
>         vserver->listenfd = uds_socket(path);
>         if (vserver->listenfd < 0) {
>                 free(vserver);
> +               pthread_mutex_unlock(&g_vhost_server.server_mutex);
>                 return -1;
>         }
> -       vserver->path = path;
> +
> +       vserver->path = strdup(path);
> 
>         fdset_add(&g_vhost_server.fdset, vserver->listenfd,
> -               vserver_new_vq_conn, NULL,
> -               vserver);
> +               vserver_new_vq_conn, NULL, vserver);
> 
> 
> In fd_man.c, in the event handler for connection fd,  the fd could be closed when receives no data.
> Before the following code snippet, as it isn't protected, there is chance we register the listenfd with the value of the just closed fd.
> so the following fdset_del could wrongly remove the new listenfd.
> would use fdset_del_slot to delete entry at fixed slot.
> 
>  if (remove1 || remove2)
>                                 fdset_del(pfdset, fd);
> 
> 
> another thing is when select is blocked, rte_vhost_driver_unregister/register  could remove/refill entries of some listenfd(s!!!).
> There is potential unwanted call on new listenfds, it is not a issue. would add comment to emphasize that.
> 
> 
> 
> 
> 
> 
> -       g_vhost_server.server[vserver_idx++] = vserver;
> +       g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
> +       pthread_mutex_unlock(&g_vhost_server.server_mutex);
> 
>         return 0;
>  }
> 
> 
> +/**
> + * Unregister the specified vhost server
> + */
> +int
> +rte_vhost_driver_unregister(const char *path)
> +{
> +       int i;
> +       int count;
> +
> +       pthread_mutex_lock(&g_vhost_server.server_mutex);
> +
> +       for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
> +               if (!strcmp(g_vhost_server.server[i]->path, path)) {
> +                       fdset_del(&g_vhost_server.fdset,
> +                               g_vhost_server.server[i]->listenfd);
> +
> +                       close(g_vhost_server.server[i]->listenfd);
> +                       free(g_vhost_server.server[i]->path);
> +                       free(g_vhost_server.server[i]);
> +
> +                       unlink(path);
> +
> +                       count = --g_vhost_server.vserver_cnt;
> +                       g_vhost_server.server[i] = g_vhost_server.server[count];
> +                       g_vhost_server.server[count] = NULL;
> +                       pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +                       return 0;
> +               }
> +       }
> +       pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +       return -1;
> +}
> +
>  int
>  rte_vhost_driver_session_start(void)
>  {
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h b/lib/librte_vhost/vhost_user/vhost-net-user.h
> index 1b6be6c..2e72f3c 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.h
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
> @@ -41,7 +41,7 @@
>  #include "fd_man.h"
> 
>  struct vhost_server {
> -       const char *path; /**< The path the uds is bind to. */
> +       char *path; /**< The path the uds is bind to. */
>         int listenfd;     /**< The listener sockfd. */
>  };
> 
> 
> 

Re: [dpdk-dev] [PATCH v3 1/2] vhost: vhost unix domain socket cleanup

[Xie, Huawei]

On 6/30/2015 5:04 AM, Thomas Monjalon wrote:
> Huawei,
> I don't understand this reply. You forgot quoting, you didn't remove useless lines,
> and you seem to reply to yourself.
> Should this patch be applied?
>
Thomas:
Oh, here i remove useless lines.  I am sending a new patch to fix a
potential issue.

[dpdk-dev] [PATCH v4 0/4] vhost: vhost unix domain socket cleanup

[Huawei Xie]

vhost user could register multiple unix domain socket server, and use the path
to identify the virtio device connecting to it. rte_vhost_driver_unregister
will clean up the unix domain socket for the specified path.

v2 changes:
-minor code style fix, remove unnecessary new line

v3 changes:
update version map file

v4 changes:
-add comment for potential unwanted callback on listenfds
-call fdset_del_slot to remove connection fd 

Huawei Xie (4):
  fdset_del_slot
  vhost socket cleanup
  version map file update
  add comment for potential unwanted call on listenfds

 lib/librte_vhost/rte_vhost_version.map       |  8 ++++
 lib/librte_vhost/rte_virtio_net.h            |  3 ++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
 lib/librte_vhost/vhost_user/fd_man.c         | 34 +++++++++++++-
 lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
 lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
 6 files changed, 110 insertions(+), 14 deletions(-)

-- 
1.8.1.4

[dpdk-dev] [PATCH v4 1/4] vhost: call fdset_del_slot to remove connection fd

[Huawei Xie]

In the event handler of connection fd, the connection fd could be possibly
closed. The event dispatch loop would then try to remove the fd from fdset.
Between these two actions, another thread might register a new listenfd
reusing the val of just closed fd, so we couldn't call fdset_del which would
wrongly clean up the new listenfd. A new function fdset_del_slot is provided
to cleanup the fd at the specified location.

v4 changes:
- call fdset_del_slot to remove connection fd

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/vhost_user/fd_man.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
index 831c9c1..bd30f8d 100644
--- a/lib/librte_vhost/vhost_user/fd_man.c
+++ b/lib/librte_vhost/vhost_user/fd_man.c
@@ -188,6 +188,24 @@ fdset_del(struct fdset *pfdset, int fd)
 }
 
 /**
+ *  Unregister the fd at the specified slot from the fdset.
+ */
+static void
+fdset_del_slot(struct fdset *pfdset, int index)
+{
+	if (pfdset == NULL || index < 0 || index >= MAX_FDS)
+		return;
+
+	pthread_mutex_lock(&pfdset->fd_mutex);
+
+	pfdset->fd[index].fd = -1;
+	pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
+	pfdset->num--;
+
+	pthread_mutex_unlock(&pfdset->fd_mutex);
+}
+
+/**
  * This functions runs in infinite blocking loop until there is no fd in
  * pfdset. It calls corresponding r/w handler if there is event on the fd.
  *
@@ -248,8 +266,15 @@ fdset_event_dispatch(struct fdset *pfdset)
 			 * We don't allow fdset_del to be called in callback
 			 * directly.
 			 */
+			/*
+			 * When we are to clean up the fd from fdset,
+			 * because the fd is closed in the cb,
+			 * the old fd val could be reused by when creates new
+			 * listen fd in another thread, we couldn't call
+			 * fd_set_del.
+			 */
 			if (remove1 || remove2)
-				fdset_del(pfdset, fd);
+				fdset_del_slot(pfdset, i);
 		}
 	}
 }
-- 
1.8.1.4

[dpdk-dev] [PATCH v4 2/4] vhost: vhost unix domain socket cleanup

[Huawei Xie]

rte_vhost_driver_unregister API will remove the listenfd from event list,
and then close it.

v2 changes:
-minor code style fix, remove unnecessary new line

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Signed-off-by: Peng Sun <peng.a.sun@intel.com>
---
 lib/librte_vhost/rte_virtio_net.h            |  3 ++
 lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
 lib/librte_vhost/vhost_user/vhost-net-user.c | 68 +++++++++++++++++++++++-----
 lib/librte_vhost/vhost_user/vhost-net-user.h |  2 +-
 4 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
index 5d38185..5630fbc 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct virtio_net *dev, uint16_t queue_i
 /* Register vhost driver. dev_name could be different for multiple instance support. */
 int rte_vhost_driver_register(const char *dev_name);
 
+/* Unregister vhost driver. This is only meaningful to vhost user. */
+int rte_vhost_driver_unregister(const char *dev_name);
+
 /* Register callbacks. */
 int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const);
 /* Start vhost driver session blocking loop. */
diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
index 6b68abf..1ae7c49 100644
--- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
+++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
@@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)
 }
 
 /**
+ * An empty function for unregister
+ */
+int
+rte_vhost_driver_unregister(const char *dev_name __rte_unused)
+{
+	return 0;
+}
+
+/**
  * The CUSE session is launched allowing the application to receive open,
  * release and ioctl calls.
  */
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index 31f1215..87a4711 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -66,6 +66,8 @@ struct connfd_ctx {
 struct _vhost_server {
 	struct vhost_server *server[MAX_VHOST_SERVER];
 	struct fdset fdset;
+	int vserver_cnt;
+	pthread_mutex_t server_mutex;
 };
 
 static struct _vhost_server g_vhost_server = {
@@ -74,10 +76,10 @@ static struct _vhost_server g_vhost_server = {
 		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
 		.num = 0
 	},
+	.vserver_cnt = 0,
+	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
 };
 
-static int vserver_idx;
-
 static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_NONE] = "VHOST_USER_NONE",
 	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
@@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int *remove)
 	}
 }
 
-
 /**
  * Creates and initialise the vhost server.
  */
@@ -436,34 +437,77 @@ rte_vhost_driver_register(const char *path)
 {
 	struct vhost_server *vserver;
 
-	if (vserver_idx == 0)
+	pthread_mutex_lock(&g_vhost_server.server_mutex);
+	if (ops == NULL)
 		ops = get_virtio_net_callbacks();
-	if (vserver_idx == MAX_VHOST_SERVER)
+
+	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"error: the number of servers reaches maximum\n");
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
+	}
 
 	vserver = calloc(sizeof(struct vhost_server), 1);
-	if (vserver == NULL)
+	if (vserver == NULL) {
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
-
-	unlink(path);
+	}
 
 	vserver->listenfd = uds_socket(path);
 	if (vserver->listenfd < 0) {
 		free(vserver);
+		pthread_mutex_unlock(&g_vhost_server.server_mutex);
 		return -1;
 	}
-	vserver->path = path;
+
+	vserver->path = strdup(path);
 
 	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
-		vserver_new_vq_conn, NULL,
-		vserver);
+		vserver_new_vq_conn, NULL, vserver);
 
-	g_vhost_server.server[vserver_idx++] = vserver;
+	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
+	pthread_mutex_unlock(&g_vhost_server.server_mutex);
 
 	return 0;
 }
 
 
+/**
+ * Unregister the specified vhost server
+ */
+int
+rte_vhost_driver_unregister(const char *path)
+{
+	int i;
+	int count;
+
+	pthread_mutex_lock(&g_vhost_server.server_mutex);
+
+	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
+		if (!strcmp(g_vhost_server.server[i]->path, path)) {
+			fdset_del(&g_vhost_server.fdset,
+				g_vhost_server.server[i]->listenfd);
+
+			close(g_vhost_server.server[i]->listenfd);
+			free(g_vhost_server.server[i]->path);
+			free(g_vhost_server.server[i]);
+
+			unlink(path);
+
+			count = --g_vhost_server.vserver_cnt;
+			g_vhost_server.server[i] = g_vhost_server.server[count];
+			g_vhost_server.server[count] = NULL;
+			pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+			return 0;
+		}
+	}
+	pthread_mutex_unlock(&g_vhost_server.server_mutex);
+
+	return -1;
+}
+
 int
 rte_vhost_driver_session_start(void)
 {
diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h b/lib/librte_vhost/vhost_user/vhost-net-user.h
index 1b6be6c..2e72f3c 100644
--- a/lib/librte_vhost/vhost_user/vhost-net-user.h
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
@@ -41,7 +41,7 @@
 #include "fd_man.h"
 
 struct vhost_server {
-	const char *path; /**< The path the uds is bind to. */
+	char *path; /**< The path the uds is bind to. */
 	int listenfd;     /**< The listener sockfd. */
 };
 
-- 
1.8.1.4

[dpdk-dev] [PATCH v4 3/4] vhost: version map file update

[Huawei Xie]

update version map file for rte_vhost_driver_unregister API

v3 changes:
update version map file

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/rte_vhost_version.map | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 163dde0..fb6bb9e 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -13,3 +13,11 @@ DPDK_2.0 {
 
 	local: *;
 };
+
+DPDK_2.1 {
+	global:
+
+	rte_vhost_driver_unregister;
+
+	local: *;
+} DPDK_2.0;
-- 
1.8.1.4

[dpdk-dev] [PATCH v4 4/4] vhost: add comment for potential unwanted callback on listenfds

[Huawei Xie]

add comment for potential unwanted callback on listenfds

v4 changes:
add comment for potential unwanted callback on listenfds

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/vhost_user/fd_man.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
index bd30f8d..d68b270 100644
--- a/lib/librte_vhost/vhost_user/fd_man.c
+++ b/lib/librte_vhost/vhost_user/fd_man.c
@@ -242,6 +242,13 @@ fdset_event_dispatch(struct fdset *pfdset)
 
 		pthread_mutex_unlock(&pfdset->fd_mutex);
 
+		/*
+		 * When select is blocked, other threads might unregister
+		 * listenfds from and register new listenfds into fdset.
+		 * When select returns, the entries for listenfds in the fdset
+		 * might have been updated. It is ok if there is unwanted call
+		 * for new listenfds.
+		 */
 		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
 		if (ret <= 0)
 			continue;
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v4 0/4] vhost: vhost unix domain socket cleanup

[Thomas Monjalon]

2015-06-30 17:20, Huawei Xie:
> vhost user could register multiple unix domain socket server, and use the path
> to identify the virtio device connecting to it. rte_vhost_driver_unregister
> will clean up the unix domain socket for the specified path.
> 
> v2 changes:
> -minor code style fix, remove unnecessary new line
> 
> v3 changes:
> update version map file
> 
> v4 changes:
> -add comment for potential unwanted callback on listenfds
> -call fdset_del_slot to remove connection fd 
> 
> Huawei Xie (4):
>   fdset_del_slot
>   vhost socket cleanup
>   version map file update
>   add comment for potential unwanted call on listenfds

Applied, thanks

Re: [dpdk-dev] [PATCH v4 1/4] vhost: call fdset_del_slot to remove connection fd

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Tuesday, June 30, 2015 5:21 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 1/4] vhost: call fdset_del_slot to remove
> connection fd
> 
> In the event handler of connection fd, the connection fd could be possibly
> closed. The event dispatch loop would then try to remove the fd from fdset.
> Between these two actions, another thread might register a new listenfd
> reusing the val of just closed fd, so we couldn't call fdset_del which would
> wrongly clean up the new listenfd. A new function fdset_del_slot is provided
> to cleanup the fd at the specified location.
> 
> v4 changes:
> - call fdset_del_slot to remove connection fd
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Acked-by: Changchun Ouyang <changchun.ouyang@intel.com>

> ---
>  lib/librte_vhost/vhost_user/fd_man.c | 27
> ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost_user/fd_man.c
> b/lib/librte_vhost/vhost_user/fd_man.c
> index 831c9c1..bd30f8d 100644
> --- a/lib/librte_vhost/vhost_user/fd_man.c
> +++ b/lib/librte_vhost/vhost_user/fd_man.c
> @@ -188,6 +188,24 @@ fdset_del(struct fdset *pfdset, int fd)  }
> 
>  /**
> + *  Unregister the fd at the specified slot from the fdset.
> + */
> +static void
> +fdset_del_slot(struct fdset *pfdset, int index) {
> +	if (pfdset == NULL || index < 0 || index >= MAX_FDS)
> +		return;
> +
> +	pthread_mutex_lock(&pfdset->fd_mutex);
> +
> +	pfdset->fd[index].fd = -1;
> +	pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
> +	pfdset->num--;
> +
> +	pthread_mutex_unlock(&pfdset->fd_mutex);
> +}
> +
> +/**
>   * This functions runs in infinite blocking loop until there is no fd in
>   * pfdset. It calls corresponding r/w handler if there is event on the fd.
>   *
> @@ -248,8 +266,15 @@ fdset_event_dispatch(struct fdset *pfdset)
>  			 * We don't allow fdset_del to be called in callback
>  			 * directly.
>  			 */
> +			/*
> +			 * When we are to clean up the fd from fdset,
> +			 * because the fd is closed in the cb,
> +			 * the old fd val could be reused by when creates
> new
> +			 * listen fd in another thread, we couldn't call
> +			 * fd_set_del.
> +			 */
>  			if (remove1 || remove2)
> -				fdset_del(pfdset, fd);
> +				fdset_del_slot(pfdset, i);
>  		}
>  	}
>  }
> --
> 1.8.1.4

Re: [dpdk-dev] [PATCH v4 2/4] vhost: vhost unix domain socket cleanup

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Tuesday, June 30, 2015 5:21 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 2/4] vhost: vhost unix domain socket cleanup
> 
> rte_vhost_driver_unregister API will remove the listenfd from event list, and
> then close it.
> 
> v2 changes:
> -minor code style fix, remove unnecessary new line
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Signed-off-by: Peng Sun <peng.a.sun@intel.com>

Acked-by: Changchun Ouyang <changchun.ouyang@intel.com>

> ---
>  lib/librte_vhost/rte_virtio_net.h            |  3 ++
>  lib/librte_vhost/vhost_cuse/vhost-net-cdev.c |  9 ++++
> lib/librte_vhost/vhost_user/vhost-net-user.c | 68
> +++++++++++++++++++++++-----  lib/librte_vhost/vhost_user/vhost-net-
> user.h |  2 +-
>  4 files changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_virtio_net.h
> b/lib/librte_vhost/rte_virtio_net.h
> index 5d38185..5630fbc 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -188,6 +188,9 @@ int rte_vhost_enable_guest_notification(struct
> virtio_net *dev, uint16_t queue_i
>  /* Register vhost driver. dev_name could be different for multiple instance
> support. */  int rte_vhost_driver_register(const char *dev_name);
> 
> +/* Unregister vhost driver. This is only meaningful to vhost user. */
> +int rte_vhost_driver_unregister(const char *dev_name);
> +
>  /* Register callbacks. */
>  int rte_vhost_driver_callback_register(struct virtio_net_device_ops const *
> const);
>  /* Start vhost driver session blocking loop. */ diff --git
> a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> index 6b68abf..1ae7c49 100644
> --- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> +++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
> @@ -405,6 +405,15 @@ rte_vhost_driver_register(const char *dev_name)  }
> 
>  /**
> + * An empty function for unregister
> + */
> +int
> +rte_vhost_driver_unregister(const char *dev_name __rte_unused) {
> +	return 0;
> +}
> +
> +/**
>   * The CUSE session is launched allowing the application to receive open,
>   * release and ioctl calls.
>   */
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c
> b/lib/librte_vhost/vhost_user/vhost-net-user.c
> index 31f1215..87a4711 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.c
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
> @@ -66,6 +66,8 @@ struct connfd_ctx {
>  struct _vhost_server {
>  	struct vhost_server *server[MAX_VHOST_SERVER];
>  	struct fdset fdset;
> +	int vserver_cnt;
> +	pthread_mutex_t server_mutex;
>  };
> 
>  static struct _vhost_server g_vhost_server = { @@ -74,10 +76,10 @@ static
> struct _vhost_server g_vhost_server = {
>  		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
>  		.num = 0
>  	},
> +	.vserver_cnt = 0,
> +	.server_mutex = PTHREAD_MUTEX_INITIALIZER,
>  };
> 
> -static int vserver_idx;
> -
>  static const char *vhost_message_str[VHOST_USER_MAX] = {
>  	[VHOST_USER_NONE] = "VHOST_USER_NONE",
>  	[VHOST_USER_GET_FEATURES] = "VHOST_USER_GET_FEATURES",
> @@ -427,7 +429,6 @@ vserver_message_handler(int connfd, void *dat, int
> *remove)
>  	}
>  }
> 
> -
>  /**
>   * Creates and initialise the vhost server.
>   */
> @@ -436,34 +437,77 @@ rte_vhost_driver_register(const char *path)  {
>  	struct vhost_server *vserver;
> 
> -	if (vserver_idx == 0)
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +	if (ops == NULL)
>  		ops = get_virtio_net_callbacks();
> -	if (vserver_idx == MAX_VHOST_SERVER)
> +
> +	if (g_vhost_server.vserver_cnt == MAX_VHOST_SERVER) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"error: the number of servers reaches maximum\n");
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
> +	}
> 
>  	vserver = calloc(sizeof(struct vhost_server), 1);
> -	if (vserver == NULL)
> +	if (vserver == NULL) {
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
> -
> -	unlink(path);
> +	}
> 
>  	vserver->listenfd = uds_socket(path);
>  	if (vserver->listenfd < 0) {
>  		free(vserver);
> +		pthread_mutex_unlock(&g_vhost_server.server_mutex);
>  		return -1;
>  	}
> -	vserver->path = path;
> +
> +	vserver->path = strdup(path);
> 
>  	fdset_add(&g_vhost_server.fdset, vserver->listenfd,
> -		vserver_new_vq_conn, NULL,
> -		vserver);
> +		vserver_new_vq_conn, NULL, vserver);
> 
> -	g_vhost_server.server[vserver_idx++] = vserver;
> +	g_vhost_server.server[g_vhost_server.vserver_cnt++] = vserver;
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> 
>  	return 0;
>  }
> 
> 
> +/**
> + * Unregister the specified vhost server  */ int
> +rte_vhost_driver_unregister(const char *path) {
> +	int i;
> +	int count;
> +
> +	pthread_mutex_lock(&g_vhost_server.server_mutex);
> +
> +	for (i = 0; i < g_vhost_server.vserver_cnt; i++) {
> +		if (!strcmp(g_vhost_server.server[i]->path, path)) {
> +			fdset_del(&g_vhost_server.fdset,
> +				g_vhost_server.server[i]->listenfd);
> +
> +			close(g_vhost_server.server[i]->listenfd);
> +			free(g_vhost_server.server[i]->path);
> +			free(g_vhost_server.server[i]);
> +
> +			unlink(path);
> +
> +			count = --g_vhost_server.vserver_cnt;
> +			g_vhost_server.server[i] =
> g_vhost_server.server[count];
> +			g_vhost_server.server[count] = NULL;
> +
> 	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +			return 0;
> +		}
> +	}
> +	pthread_mutex_unlock(&g_vhost_server.server_mutex);
> +
> +	return -1;
> +}
> +
>  int
>  rte_vhost_driver_session_start(void)
>  {
> diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h
> b/lib/librte_vhost/vhost_user/vhost-net-user.h
> index 1b6be6c..2e72f3c 100644
> --- a/lib/librte_vhost/vhost_user/vhost-net-user.h
> +++ b/lib/librte_vhost/vhost_user/vhost-net-user.h
> @@ -41,7 +41,7 @@
>  #include "fd_man.h"
> 
>  struct vhost_server {
> -	const char *path; /**< The path the uds is bind to. */
> +	char *path; /**< The path the uds is bind to. */
>  	int listenfd;     /**< The listener sockfd. */
>  };
> 
> --
> 1.8.1.4

Re: [dpdk-dev] [PATCH v4 3/4] vhost: version map file update

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Tuesday, June 30, 2015 5:21 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 3/4] vhost: version map file update
> 
> update version map file for rte_vhost_driver_unregister API
> 
> v3 changes:
> update version map file
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Acked-by: Changchun Ouyang <changchun.ouyang@intel.com>

> ---
>  lib/librte_vhost/rte_vhost_version.map | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/librte_vhost/rte_vhost_version.map
> b/lib/librte_vhost/rte_vhost_version.map
> index 163dde0..fb6bb9e 100644
> --- a/lib/librte_vhost/rte_vhost_version.map
> +++ b/lib/librte_vhost/rte_vhost_version.map
> @@ -13,3 +13,11 @@ DPDK_2.0 {
> 
>  	local: *;
>  };
> +
> +DPDK_2.1 {
> +	global:
> +
> +	rte_vhost_driver_unregister;
> +
> +	local: *;
> +} DPDK_2.0;
> --
> 1.8.1.4

Re: [dpdk-dev] [PATCH v4 4/4] vhost: add comment for potential unwanted callback on listenfds

[Ouyang, Changchun]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Tuesday, June 30, 2015 5:21 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v4 4/4] vhost: add comment for potential
> unwanted callback on listenfds
> 
> add comment for potential unwanted callback on listenfds
> 
> v4 changes:
> add comment for potential unwanted callback on listenfds
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>

Acked-by: Changchun Ouyang <changchun.ouyang@intel.com>

> ---
>  lib/librte_vhost/vhost_user/fd_man.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost_user/fd_man.c
> b/lib/librte_vhost/vhost_user/fd_man.c
> index bd30f8d..d68b270 100644
> --- a/lib/librte_vhost/vhost_user/fd_man.c
> +++ b/lib/librte_vhost/vhost_user/fd_man.c
> @@ -242,6 +242,13 @@ fdset_event_dispatch(struct fdset *pfdset)
> 
>  		pthread_mutex_unlock(&pfdset->fd_mutex);
> 
> +		/*
> +		 * When select is blocked, other threads might unregister
> +		 * listenfds from and register new listenfds into fdset.
> +		 * When select returns, the entries for listenfds in the fdset
> +		 * might have been updated. It is ok if there is unwanted call
> +		 * for new listenfds.
> +		 */
>  		ret = select(maxfds + 1, &rfds, &wfds, NULL, &tv);
>  		if (ret <= 0)
>  			continue;
> --
> 1.8.1.4