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

2015-06-30 Thread 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 v3 1/2] vhost: vhost unix domain socket cleanup

2015-06-29 Thread 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  intel.com>
> Signed-off-by: Peng 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

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

2015-06-29 Thread 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 
Signed-off-by: Peng Sun 
---
 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

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

2015-06-19 Thread 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 
Signed-off-by: Peng Sun 
---
 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);
+