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

["Xie, Huawei" <huawei.xie@intel.com>]

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