diff mbox

[v5,17/31] vhost-user: keep vhost_net after a disconnection

Message ID 57960A9E.1070904@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ilya Maximets July 25, 2016, 12:48 p.m. UTC 
On 21.07.2016 11:57, Marc-André Lureau wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 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 <marcandre.lureau@redhat.com>
> ---
>  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.

Comments

Marc-André Lureau July 25, 2016, 1:09 p.m. UTC | #1 
Hi

----- Original Message -----
> On 21.07.2016 11:57, Marc-André Lureau wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > 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 <marcandre.lureau@redhat.com>
> > ---
> >  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.
>
diff mbox

Patch

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);
         }
     }