[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v5, 17/31] vhost-user: keep vhost_net after a disconne
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [v5, 17/31] vhost-user: keep vhost_net after a disconnection |
Date: |
Mon, 25 Jul 2016 09:09:59 -0400 (EDT) |
Hi
----- Original Message -----
> On 21.07.2016 11:57, Marc-André Lureau wrote:
> > From: Marc-André Lureau <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> > 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):
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;
>
> 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);
> }
> }
> ----------------------------------------------------------------------
>
> Best regards, Ilya Maximets.
>
- Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init(), (continued)
- [Qemu-devel] [PATCH v5 10/31] vhost: do not assert() on vhost_ops failure, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 11/31] vhost: add missing VHOST_OPS_DEBUG, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 12/31] vhost: use error_report() instead of fprintf(stderr, ...), marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 13/31] qemu-char: fix qemu_chr_fe_set_msgfds() crash when disconnected, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 15/31] vhost-user: check qemu_chr_fe_set_msgfds() return value, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 14/31] vhost-user: call set_msgfds unconditionally, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 16/31] vhost-user: check vhost_user_{read, write}() return value, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 17/31] vhost-user: keep vhost_net after a disconnection, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 18/31] vhost-user: add get_vhost_net() assertions, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 19/31] Revert "vhost-net: do not crash if backend is not present", marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 20/31] vhost-net: vhost_migration_done is vhost-user specific, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 21/31] vhost: add assert() to check runtime behaviour, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 22/31] char: add chr_wait_connected callback, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 23/31] char: add and use tcp_chr_wait_connected, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 24/31] vhost-user: wait until backend init is completed, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 25/31] tests: plug some leaks in virtio-net-test, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 26/31] tests: fix vhost-user-test leak, marcandre . lureau, 2016/07/21
- [Qemu-devel] [PATCH v5 27/31] tests: add /vhost-user/connect-fail test, marcandre . lureau, 2016/07/21