From mboxrd@z Thu Jan 1 00:00:00 1970 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) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=kGSFSHz6jnqQYX8/RP4tDP10/kj0ndHtRuFD6gzYKV0=; b=KOXOdeSWBspjUTfyoHY5S+Mh/dId6M0jJn8851kaFBaQUXu3jIJ8qNlstYqpgaVUi0 ezcpQvMtCSS4j+0EjG1qDTSZmZjG36/s+RWx4I4u734U89XkfD+08h0kUGhgmnOtI9pG n9A3pOIdWSymStlvSykIV2CXqG12rZGrDe+VQDmY4eESJqwGk4GdBC4irIEk7IkDrAYG x5GBskVr5sLaL91bIsO3Bd6wn8RMEnmp1UwgFEmVRWclepR4YLyKZCS110T+tsY3AFVv 5RTS0uBVsd+VKsjIrf29arcVJFcq7fywM2mk9DvlSd1/tP41jID4/jweZCu1kpRuL5hl 09Mw== X-Gm-Message-State: ALoCoQl69dNenn9zOfdQm6XEx9l7v9YRSwJBche8xYjk3tQ2Wi2fo5rSeMgXO/WmAIx40FVkN7z8 X-Received: by 10.194.78.110 with SMTP id a14mr34136363wjx.87.1435611816893; Mon, 29 Jun 2015 14:03:36 -0700 (PDT) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by mx.google.com with ESMTPSA id d3sm13828923wic.1.2015.06.29.14.03.34 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 29 Jun 2015 14:03:35 -0700 (PDT) From: Thomas Monjalon To: "Xie, Huawei" Date: Mon, 29 Jun 2015 23:02:26 +0200 Message-ID: <4036087.qsYtaox4HE@xps13> Organization: 6WIND User-Agent: KMail/4.14.8 (Linux/4.0.4-2-ARCH; KDE/4.14.8; x86_64; ; ) In-Reply-To: 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-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v3 1/2] vhost: vhost unix domain socket cleanup X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Jun 2015 21:03:37 -0000 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. */ > }; > > >