Re: [Qemu-devel] [PATCH] vhost-user: fix reconnection support for host notifier

[Marc-André Lureau]

Hi

On Fri, Apr 26, 2019 at 8:32 AM Tiwei Bie <address@hidden> 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: address@hidden
>
> Signed-off-by: Tiwei Bie <address@hidden>
> ---
>  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
>
>


-- 
Marc-André Lureau