From mboxrd@z Thu Jan 1 00:00:00 1970 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) Date: Mon, 25 Jul 2016 15:48:30 +0300 From: Ilya Maximets In-reply-to: <20160721085750.30008-18-marcandre.lureau@redhat.com> Message-id: <57960A9E.1070904@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=utf-8 Content-transfer-encoding: QUOTED-PRINTABLE References: <20160721085750.30008-18-marcandre.lureau@redhat.com> Subject: Re: [Qemu-devel] [v5, 17/31] vhost-user: keep vhost_net after a disconnection List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , qemu-devel@nongnu.org Cc: yuanhan.liu@linux.intel.com, victork@redhat.com, mst@redhat.com, jonshin@cisco.com, mukawa@igel.co.jp, Dyasly Sergey , Heetae Ahn On 21.07.2016 11:57, Marc-Andr=C3=A9 Lureau wrote: > From: Marc-Andr=C3=A9 Lureau >=20 > Many code paths assume get_vhost_net() returns non-null. >=20 > Keep VhostUserState.vhost_net after a successful vhost_net_init(), > instead of freeing it in vhost_net_cleanup(). >=20 > VhostUserState.vhost_net is thus freed before after being recreated= or > on final vhost_user_cleanup() and there is no need to save the acke= d > features. >=20 > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > hw/net/vhost_net.c | 1 - > net/tap.c | 1 + > net/vhost-user.c | 36 ++++++++++++++++-------------------- > 3 files changed, 17 insertions(+), 21 deletions(-) >=20 > 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, NetClien= tState *ncs, > void vhost_net_cleanup(struct vhost_net *net) > { > vhost_dev_cleanup(&net->dev); > - g_free(net); > } > =20 > int vhost_net_notify_migration_done(struct vhost_net *net, char* m= ac_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) > =20 > if (s->vhost_net) { > vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > s->vhost_net =3D NULL; > } > =20 > 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; > =20 > typedef struct VhostUserChardevProps { > @@ -42,12 +41,7 @@ uint64_t vhost_user_get_acked_features(NetClient= State *nc) > { > VhostUserState *s =3D DO_UPCAST(VhostUserState, nc, nc); > assert(nc->info->type =3D=3D 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_ne= t) : 0; > } > =20 > static void vhost_user_stop(int queues, NetClientState *ncs[]) > @@ -59,15 +53,9 @@ static void vhost_user_stop(int queues, NetClien= tState *ncs[]) > assert(ncs[i]->info->type =3D=3D NET_CLIENT_DRIVER_VHOST_U= SER); > =20 > s =3D DO_UPCAST(VhostUserState, nc, ncs[i]); > - if (!vhost_user_running(s)) { > - continue; > - } > =20 > if (s->vhost_net) { > - /* save acked features */ > - s->acked_features =3D vhost_net_get_acked_features(s->= vhost_net); > vhost_net_cleanup(s->vhost_net); > - s->vhost_net =3D NULL; > } > } > } > @@ -75,6 +63,7 @@ static void vhost_user_stop(int queues, NetClient= State *ncs[]) > static int vhost_user_start(int queues, NetClientState *ncs[]) > { > VhostNetOptions options; > + struct vhost_net *net =3D NULL; > VhostUserState *s; > int max_queues; > int i; > @@ -85,33 +74,39 @@ static int vhost_user_start(int queues, NetClie= ntState *ncs[]) > assert(ncs[i]->info->type =3D=3D NET_CLIENT_DRIVER_VHOST_U= SER); > =20 > s =3D DO_UPCAST(VhostUserState, nc, ncs[i]); > - if (vhost_user_running(s)) { > - continue; > - } > =20 > options.net_backend =3D ncs[i]; > options.opaque =3D s->chr; > options.busyloop_timeout =3D 0; > - s->vhost_net =3D vhost_net_init(&options); > - if (!s->vhost_net) { > + net =3D vhost_net_init(&options); > + if (!net) { > error_report("failed to init vhost_net for queue %d", = i); > goto err; > } > =20 > if (i =3D=3D 0) { > - max_queues =3D vhost_net_get_max_queues(s->vhost_net); > + max_queues =3D vhost_net_get_max_queues(net); > if (queues > max_queues) { > error_report("you are asking more queues than supp= orted: %d", > max_queues); > goto err; > } > } > + > + if (s->vhost_net) { > + vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > + } > + s->vhost_net =3D net; > } > =20 > return 0; > =20 > err: > - vhost_user_stop(i + 1, ncs); > + if (net) { > + vhost_net_cleanup(net); > + } > + vhost_user_stop(i, ncs); > return -1; > } > =20 > @@ -150,6 +145,7 @@ static void vhost_user_cleanup(NetClientState *= nc) > =20 > if (s->vhost_net) { > vhost_net_cleanup(s->vhost_net); > + g_free(s->vhost_net); > s->vhost_net =3D NULL; > } > if (s->chr) { >=20 In patch number 7 of this patch set was introduced 'memset()' inside 'vhost_dev_cleanup()' which clears 'acked_features' field of 'vhost_d= ev' structure. This patch uses already zeroed value of this field for features restore after reconnection. You should not remove 'acked_features' from 'struct VhostUserState' o= r avoid cleaning of this field in 'vhost_dev'. I'm suggesting following fixup (mainly, just a partial revert): ---------------------------------------------------------------------= - 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; =20 typedef struct VhostUserChardevProps { @@ -41,7 +42,7 @@ uint64_t vhost_user_get_acked_features(NetClientSta= te *nc) { VhostUserState *s =3D DO_UPCAST(VhostUserState, nc, nc); assert(nc->info->type =3D=3D NET_CLIENT_DRIVER_VHOST_USER); - return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net)= : 0; + return s->acked_features; } =20 static void vhost_user_stop(int queues, NetClientState *ncs[]) @@ -55,6 +56,11 @@ static void vhost_user_stop(int queues, NetClientS= tate *ncs[]) s =3D DO_UPCAST(VhostUserState, nc, ncs[i]); =20 if (s->vhost_net) { + /* save acked features */ + uint64_t features =3D vhost_net_get_acked_features(s->vh= ost_net); + if (features) { + s->acked_features =3D features; + } vhost_net_cleanup(s->vhost_net); } } ---------------------------------------------------------------------= - Best regards, Ilya Maximets.