From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ilya Maximets Subject: Re: [PATCH] vhost: fix driver unregister for client mode Date: Thu, 21 Jul 2016 11:33:13 +0300 Message-ID: <579088C9.2030705@samsung.com> References: <1469003563-27340-1-git-send-email-i.maximets@samsung.com> <20160721082419.GB28708@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, Huawei Xie , Dyasly Sergey , Heetae Ahn , Thomas Monjalon To: Yuanhan Liu Return-path: Received: from mailout4.w1.samsung.com (mailout4.w1.samsung.com [210.118.77.14]) by dpdk.org (Postfix) with ESMTP id 410C93990 for ; Thu, 21 Jul 2016 10:33:17 +0200 (CEST) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OAN00E68P3FDF40@mailout4.w1.samsung.com> for dev@dpdk.org; Thu, 21 Jul 2016 09:33:15 +0100 (BST) In-reply-to: <20160721082419.GB28708@yliu-dev.sh.intel.com> 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" Thanks. Fixed. Best regards, Ilya Maximets. On 21.07.2016 11:24, Yuanhan Liu wrote: > On Wed, Jul 20, 2016 at 11:32:43AM +0300, Ilya Maximets wrote: >> Currently while calling of 'rte_vhost_driver_unregister()' connection >> to QEMU will not be closed. This leads to inability to register driver >> again and reconnect to same virtual machine. >> >> This scenario is reproducible with OVS. While executing of the following >> command vhost port will be re-created (will be executed >> 'rte_vhost_driver_register()' followed by 'rte_vhost_driver_unregister()') >> network will be broken and QEMU possibly will crash: >> >> ovs-vsctl set Interface vhost1 ofport_request=15 >> >> Fix this by closing all established connections on driver unregister and >> removing of pending connections from reconnection list. >> >> Fixes: 64ab701c3d1e ("vhost: add vhost-user client mode") >> Signed-off-by: Ilya Maximets > > Again, thanks for the fix. > >> --- >> >> Patch prepared for master branch because dpdk-next-virtio doesn't contain >> commit acbff5c67ea7 ("vhost: fix crash when exceeding file descriptors"). >> Porting to dpdk-next-virtio/master is trivial and may be performed on >> demand. > > Yeah, my bad, I haven't updated it after rc2, since Thomas no longer > pull request from it. Anyway, you just remind me that I should have > done that. > >> /** >> * Unregister the specified vhost socket >> */ >> @@ -672,20 +700,34 @@ rte_vhost_driver_unregister(const char *path) >> { >> int i; >> int count; >> + struct vhost_user_connection *conn; >> >> pthread_mutex_lock(&vhost_user.mutex); >> >> for (i = 0; i < vhost_user.vsocket_cnt; i++) { >> - if (!strcmp(vhost_user.vsockets[i]->path, path)) { >> - if (vhost_user.vsockets[i]->is_server) { >> - fdset_del(&vhost_user.fdset, >> - vhost_user.vsockets[i]->listenfd); >> - close(vhost_user.vsockets[i]->listenfd); >> + struct vhost_user_socket *vsocket = vhost_user.vsockets[i]; >> + >> + if (!strcmp(vsocket->path, path)) { >> + if (vsocket->is_server) { >> + (void) fdset_del(&vhost_user.fdset, >> + vsocket->listenfd); > > I would think the (void) cast is not neceessary here. > >> + close(vsocket->listenfd); >> unlink(path); >> + } else if (vsocket->reconnect) { >> + vhost_user_remove_reconnect(vsocket); >> + } >> + >> + conn = fdset_del(&vhost_user.fdset, vsocket->connfd); >> + if (conn) { >> + RTE_LOG(INFO, VHOST_CONFIG, "free connfd = %d" >> + "for device '%s'\n", vsocket->connfd, path); > > We should try not to break a log message into several lines, which > hurts "git grep". Here, it could be: > > RTE_LOG(INFO, VHOST_CONFIG, > "free connfd = %d for device '%s'\n", > vsocket->connfd, path); > > Besides the two minor nits, > > Acked-by: Yuanhan Liu > > --yliu > >