From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35510) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfe4-0003A9-1H for qemu-devel@nongnu.org; Mon, 25 Jul 2016 09:10:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bRfdz-0002qr-GP for qemu-devel@nongnu.org; Mon, 25 Jul 2016 09:10:07 -0400 Received: from mx5-phx2.redhat.com ([209.132.183.37]:54118) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bRfdz-0002qZ-7z for qemu-devel@nongnu.org; Mon, 25 Jul 2016 09:10:03 -0400 Date: Mon, 25 Jul 2016 09:09:59 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <388285361.8322739.1469452199408.JavaMail.zimbra@redhat.com> In-Reply-To: <57960A9E.1070904@samsung.com> References: <20160721085750.30008-18-marcandre.lureau@redhat.com> <57960A9E.1070904@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: Ilya Maximets Cc: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , qemu-devel@nongnu.org, yuanhan liu , victork@redhat.com, mst@redhat.com, jonshin@cisco.com, mukawa@igel.co.jp, Dyasly Sergey , Heetae Ahn Hi ----- Original Message ----- > 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 acked > > 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, NetClientSta= te > > *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* mac_a= ddr) > > 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(NetClientStat= e > > *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_net) := 0; > > } > > =20 > > static void vhost_user_stop(int queues, NetClientState *ncs[]) > > @@ -59,15 +53,9 @@ static void vhost_user_stop(int queues, NetClientSta= te > > *ncs[]) > > assert(ncs[i]->info->type =3D=3D NET_CLIENT_DRIVER_VHOST_USER)= ; > > =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, NetClientStat= e > > *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, NetClientSt= ate > > *ncs[]) > > assert(ncs[i]->info->type =3D=3D NET_CLIENT_DRIVER_VHOST_USER)= ; > > =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 supporte= d: > > %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 >=20 > 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. >=20 > You should not remove 'acked_features' from 'struct VhostUserState' or > avoid cleaning of this field in 'vhost_dev'. >=20 > I'm suggesting following fixup (mainly, just a partial revert): Thanks! I'll squash the fix. > ---------------------------------------------------------------------- > 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(NetClientState *= 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, NetClientState > *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->vhost_= net); > + if (features) { > + s->acked_features =3D features; > + } > vhost_net_cleanup(s->vhost_net); > } > } > ---------------------------------------------------------------------- >=20 > Best regards, Ilya Maximets. >=20