From patchwork Mon Jul 25 12:48:30 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 652249 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3ryh2l5Csrz9sC3 for ; Mon, 25 Jul 2016 22:51:27 +1000 (AEST) Received: from localhost ([::1]:60443 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfLx-0005MY-A2 for incoming@patchwork.ozlabs.org; Mon, 25 Jul 2016 08:51:25 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57804) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfJJ-0003B5-C5 for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:48:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRfJE-00067y-Hv for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:48:41 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:54265) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfJE-00067j-8g for qemu-devel@nongnu.org; Mon, 25 Jul 2016 08:48:36 -0400 Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OAV00H5OFKW9B60@mailout4.w1.samsung.com> for qemu-devel@nongnu.org; Mon, 25 Jul 2016 13:48:33 +0100 (BST) Received: from eusync4.samsung.com ( [203.254.199.214]) by eucpsbgm1.samsung.com (EUCPMTA) with SMTP id 25.62.05254.0AA06975; Mon, 25 Jul 2016 13:48:32 +0100 (BST) Received: from [106.109.129.180] by eusync4.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0OAV006AOFKUJ910@eusync4.samsung.com>; Mon, 25 Jul 2016 13:48:32 +0100 (BST) Date: Mon, 25 Jul 2016 15:48:30 +0300 From: Ilya Maximets In-reply-to: <20160721085750.30008-18-marcandre.lureau@redhat.com> To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , qemu-devel@nongnu.org Message-id: <57960A9E.1070904@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: QUOTED-PRINTABLE X-AuditID: cbfec7f4-f796c6d000001486-c0-57960aa0cd08 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrBLMWRmVeSWpSXmKPExsVy+t/xa7oLuKaFG1yYomYx7fNtdosr7T/Z LZ6sX8NusaCtndXi/69XrBbHej6xWhzv3cFiMXm2lMXiA4eZLa5PuMDqwOUx5fdGVo/G5xIe 804Gejy5tpnJ4/2+q2wefVtWMQawRXHZpKTmZJalFunbJXBl7L7zi7Xgu0lF5wydBsbfal2M nBwSAiYSCzvuMkPYYhIX7q1nA7GFBJYySjR05YLYvAKCEj8m32PpYuTgYBaQlzhyKRvCVJeY MgWogguo+gWjxJZXlxlB4sICHhLT7jOBdIoIxEo0zVvABDHRUWLDme/sIPXMArsYJX6vXcwC kmAT0JE4tfoII8QqLYmu6S1gNouAqsSM/b1gNaICERKztv8AG8Qp4CRxc+IF5gmMArOQXDcL 4bpZCNctYGRexSiaWppcUJyUnmuoV5yYW1yal66XnJ+7iRES/l92MC4+ZnWIUYCDUYmHV4Fp argQa2JZcWXuIUYJDmYlEd75nNPChXhTEiurUovy44tKc1KLDzFKc7AoifPO3fU+REggPbEk NTs1tSC1CCbLxMEp1cCYL5LGIjl/63uGsk2F2WmzNVgnenw8ty/z1BQvzqtyJzz9/pQI5nlw tS9+d1/a5W9wZKrAjF3q518bTw7bN3/HDckFbiwzeZ+J2CexcLrf6ryZvtT687tvxVs0eo1X /no9K/ywru2uleZF7evN9FRVv07/OrdLKFd0VuvzLbf+m33uiWiYzSegxFKckWioxVxUnAgA 6PtfTnsCAAA= References: <20160721085750.30008-18-marcandre.lureau@redhat.com> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 210.118.77.14 Subject: Re: [Qemu-devel] [v5, 17/31] vhost-user: keep vhost_net after a disconnection X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: yuanhan.liu@linux.intel.com, victork@redhat.com, Heetae Ahn , Dyasly Sergey , mst@redhat.com, jonshin@cisco.com, mukawa@igel.co.jp Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" On 21.07.2016 11:57, Marc-André Lureau wrote: > From: Marc-André Lureau > > Many code paths assume get_vhost_net() returns non-null. > > Keep VhostUserState.vhost_net after a successful vhost_net_init(), > instead of freeing it in vhost_net_cleanup(). > > VhostUserState.vhost_net is thus freed before after being recreated or > on final vhost_user_cleanup() and there is no need to save the acked > features. > > Signed-off-by: Marc-André Lureau > --- > hw/net/vhost_net.c | 1 - > net/tap.c | 1 + > net/vhost-user.c | 36 ++++++++++++++++-------------------- > 3 files changed, 17 insertions(+), 21 deletions(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index c11f69c..7b76591 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -378,7 +378,6 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs, > void vhost_net_cleanup(struct vhost_net *net) > { > vhost_dev_cleanup(&net->dev); > - g_free(net); > } > > int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr) > diff --git a/net/tap.c b/net/tap.c > index 40a8c74..6abb962 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -312,6 +312,7 @@ static void tap_cleanup(NetClientState *nc) > > if (s->vhost_net) { > vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > s->vhost_net = NULL; > } > > diff --git a/net/vhost-user.c b/net/vhost-user.c > index 2af212b..60fab9a 100644 > --- a/net/vhost-user.c > +++ b/net/vhost-user.c > @@ -23,7 +23,6 @@ typedef struct VhostUserState { > CharDriverState *chr; > VHostNetState *vhost_net; > guint watch; > - uint64_t acked_features; > } VhostUserState; > > typedef struct VhostUserChardevProps { > @@ -42,12 +41,7 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc) > { > VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc); > assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER); > - return s->acked_features; > -} > - > -static int vhost_user_running(VhostUserState *s) > -{ > - return (s->vhost_net) ? 1 : 0; > + return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0; > } > > static void vhost_user_stop(int queues, NetClientState *ncs[]) > @@ -59,15 +53,9 @@ static void vhost_user_stop(int queues, NetClientState *ncs[]) > assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER); > > s = DO_UPCAST(VhostUserState, nc, ncs[i]); > - if (!vhost_user_running(s)) { > - continue; > - } > > if (s->vhost_net) { > - /* save acked features */ > - s->acked_features = vhost_net_get_acked_features(s->vhost_net); > vhost_net_cleanup(s->vhost_net); > - s->vhost_net = NULL; > } > } > } > @@ -75,6 +63,7 @@ static void vhost_user_stop(int queues, NetClientState *ncs[]) > static int vhost_user_start(int queues, NetClientState *ncs[]) > { > VhostNetOptions options; > + struct vhost_net *net = NULL; > VhostUserState *s; > int max_queues; > int i; > @@ -85,33 +74,39 @@ static int vhost_user_start(int queues, NetClientState *ncs[]) > assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER); > > s = DO_UPCAST(VhostUserState, nc, ncs[i]); > - if (vhost_user_running(s)) { > - continue; > - } > > options.net_backend = ncs[i]; > options.opaque = s->chr; > options.busyloop_timeout = 0; > - s->vhost_net = vhost_net_init(&options); > - if (!s->vhost_net) { > + net = vhost_net_init(&options); > + if (!net) { > error_report("failed to init vhost_net for queue %d", i); > goto err; > } > > if (i == 0) { > - max_queues = vhost_net_get_max_queues(s->vhost_net); > + max_queues = vhost_net_get_max_queues(net); > if (queues > max_queues) { > error_report("you are asking more queues than supported: %d", > max_queues); > goto err; > } > } > + > + if (s->vhost_net) { > + vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > + } > + s->vhost_net = net; > } > > return 0; > > err: > - vhost_user_stop(i + 1, ncs); > + if (net) { > + vhost_net_cleanup(net); > + } > + vhost_user_stop(i, ncs); > return -1; > } > > @@ -150,6 +145,7 @@ static void vhost_user_cleanup(NetClientState *nc) > > if (s->vhost_net) { > vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > s->vhost_net = NULL; > } > if (s->chr) { > In patch number 7 of this patch set was introduced 'memset()' inside 'vhost_dev_cleanup()' which clears 'acked_features' field of 'vhost_dev' structure. This patch uses already zeroed value of this field for features restore after reconnection. You should not remove 'acked_features' from 'struct VhostUserState' or avoid cleaning of this field in 'vhost_dev'. I'm suggesting following fixup (mainly, just a partial revert): ---------------------------------------------------------------------- ---------------------------------------------------------------------- Best regards, Ilya Maximets. diff --git a/net/vhost-user.c b/net/vhost-user.c index 60fab9a..3100a5e 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -23,6 +23,7 @@ typedef struct VhostUserState { CharDriverState *chr; VHostNetState *vhost_net; guint watch; + uint64_t acked_features; } VhostUserState; typedef struct VhostUserChardevProps { @@ -41,7 +42,7 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc) { VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc); assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER); - return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0; + return s->acked_features; } static void vhost_user_stop(int queues, NetClientState *ncs[]) @@ -55,6 +56,11 @@ static void vhost_user_stop(int queues, NetClientState *ncs[]) s = DO_UPCAST(VhostUserState, nc, ncs[i]); if (s->vhost_net) { + /* save acked features */ + uint64_t features = vhost_net_get_acked_features(s->vhost_net); + if (features) { + s->acked_features = features; + } vhost_net_cleanup(s->vhost_net); } }