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

Message ID 1433209800-29091-1-git-send-email-huawei.xie@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Huawei Xie June 2, 2015, 1:50 a.m. UTC
  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(-)
  

Comments

Loftus, Ciara June 3, 2015, 9:42 a.m. UTC | #1
> -----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
  
Ouyang Changchun June 3, 2015, 12:55 p.m. UTC | #2
> -----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
  
Huawei Xie June 3, 2015, 7:03 p.m. UTC | #3
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
>
>
  
Tetsuya Mukawa June 17, 2015, 4:17 a.m. UTC | #4
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
  
Huawei Xie June 17, 2015, 11:05 a.m. UTC | #5
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?
  
Tetsuya Mukawa June 18, 2015, 1 a.m. UTC | #6
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
  

Patch

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. */
 };