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

[Tiwei Bie <tiwei.bie@intel.com>]

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