From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v3 1/2] vhost: vhost unix domain socket cleanup Date: Mon, 29 Jun 2015 23:02:26 +0200 Message-ID: <4036087.qsYtaox4HE@xps13> References: <1433474786-704-1-git-send-email-huawei.xie@intel.com> <1434649260-26317-2-git-send-email-huawei.xie@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org To: "Xie, Huawei" Return-path: Received: from mail-wi0-f172.google.com (mail-wi0-f172.google.com [209.85.212.172]) by dpdk.org (Postfix) with ESMTP id 1B0D7C44E for ; Mon, 29 Jun 2015 23:03:37 +0200 (CEST) Received: by wiar9 with SMTP id r9so16232135wia.1 for ; Mon, 29 Jun 2015 14:03:37 -0700 (PDT) In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > 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 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. */ > }; > > >