| Message ID | 20190426061815.6384-1-tiwei.bie@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-user: fix reconnection support for host notifier | expand |
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
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; + } + } + } + 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)); user->chr = chr; return true; }
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(+)