vhost-user: fix reconnection support for host notifier

We need to destroy the host notifiers when cleaning up
the backend. Otherwise, some resources are not released
after the connection is closed, and it may prevent the
external backend from reopening them (e.g. VFIO files)
during restart.

Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
Cc: qemu-stable@nongnu.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 hw/virtio/vhost-user.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Hi

On Fri, Apr 26, 2019 at 8:32 AM Tiwei Bie <tiwei.bie@intel.com> wrote:
>
> We need to destroy the host notifiers when cleaning up
> the backend. Otherwise, some resources are not released
> after the connection is closed, and it may prevent the
> external backend from reopening them (e.g. VFIO files)
> during restart.
>
> Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> Cc: qemu-stable@nongnu.org
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>  hw/virtio/vhost-user.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 553319c7ac..56656629c0 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1454,10 +1454,24 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
>  static int vhost_user_backend_cleanup(struct vhost_dev *dev)
>  {
>      struct vhost_user *u;
> +    VhostUserState *user;
> +    int i;
>
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>
>      u = dev->opaque;
> +
> +    if (dev->vq_index == 0) {
> +        user = u->user;
> +        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +            if (user->notifier[i].addr) {
> +                object_unparent(OBJECT(&user->notifier[i].mr));
> +                munmap(user->notifier[i].addr, qemu_real_host_page_size);
> +                user->notifier[i].addr = NULL;
> +            }
> +        }
> +    }

Why not call vhost_user_cleanup() ? Alternatively, factor the notifier
code in a seperate vhost_user_notifiers_cleanup() ?

> +
>      if (u->postcopy_notifier.notify) {
>          postcopy_remove_notifier(&u->postcopy_notifier);
>          u->postcopy_notifier.notify = NULL;
> @@ -1881,6 +1895,8 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp)
>          error_setg(errp, "Cannot initialize vhost-user state");
>          return false;
>      }
> +
> +    memset(user, 0, sizeof(*user));

This looks superflous. Is it really needed?

I wish there would be some basic tests for external host notifiers. Is
it too much to ask to add it in vhost-user-test.c ?


>      user->chr = chr;
>      return true;
>  }
> --
> 2.17.1
>
>

Hi,

On Thu, Jun 06, 2019 at 03:30:29PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Apr 26, 2019 at 8:32 AM Tiwei Bie <tiwei.bie@intel.com> wrote:
> >
> > We need to destroy the host notifiers when cleaning up
> > the backend. Otherwise, some resources are not released
> > after the connection is closed, and it may prevent the
> > external backend from reopening them (e.g. VFIO files)
> > during restart.
> >
> > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > Cc: qemu-stable@nongnu.org
> >
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> >  hw/virtio/vhost-user.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 553319c7ac..56656629c0 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1454,10 +1454,24 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
> >  static int vhost_user_backend_cleanup(struct vhost_dev *dev)
> >  {
> >      struct vhost_user *u;
> > +    VhostUserState *user;
> > +    int i;
> >
> >      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> >
> >      u = dev->opaque;
> > +
> > +    if (dev->vq_index == 0) {
> > +        user = u->user;
> > +        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> > +            if (user->notifier[i].addr) {
> > +                object_unparent(OBJECT(&user->notifier[i].mr));
> > +                munmap(user->notifier[i].addr, qemu_real_host_page_size);
> > +                user->notifier[i].addr = NULL;
> > +            }
> > +        }
> > +    }
> 
> Why not call vhost_user_cleanup() ? Alternatively, factor the notifier
> code in a seperate vhost_user_notifiers_cleanup() ?

I like the idea to factor the notifier code in a seperate
vhost_user_notifiers_cleanup(). I can do it. Thanks!

> 
> > +
> >      if (u->postcopy_notifier.notify) {
> >          postcopy_remove_notifier(&u->postcopy_notifier);
> >          u->postcopy_notifier.notify = NULL;
> > @@ -1881,6 +1895,8 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp)
> >          error_setg(errp, "Cannot initialize vhost-user state");
> >          return false;
> >      }
> > +
> > +    memset(user, 0, sizeof(*user));
> 
> This looks superflous. Is it really needed?

I think you are right. We already checked whether user->chr
is zero. The caller should make sure that the VhostUserState
will be zero initialized.

> 
> I wish there would be some basic tests for external host notifiers. Is
> it too much to ask to add it in vhost-user-test.c ?

Sounds good to me. Besides, there are already some basic
external host notifier supports in tests/vhost-user-bridge.c
that can be enabled with -H. I'm not sure whether that already
met what you want. If adding some basic tests in vhost-user-test.c
would help, I'd like to do it.

Thanks for the review!
Tiwei

> 
> 
> >      user->chr = chr;
> >      return true;
> >  }
> > --
> > 2.17.1
> >
> >
> 
> 
> -- 
> Marc-André Lureau