* [Qemu-devel] [PATCH] vhost-user: fix reconnection support for host notifier
@ 2019-04-26 6:18 Tiwei Bie
2019-04-26 6:18 ` Tiwei Bie
2019-06-06 13:30 ` Marc-André Lureau
0 siblings, 2 replies; 4+ messages in thread
From: Tiwei Bie @ 2019-04-26 6:18 UTC (permalink / raw)
To: mst, qemu-devel; +Cc: cunming.liang, qemu-stable
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;
+ }
+ }
+ }
+
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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Qemu-devel] [PATCH] vhost-user: fix reconnection support for host notifier
2019-04-26 6:18 [Qemu-devel] [PATCH] vhost-user: fix reconnection support for host notifier Tiwei Bie
@ 2019-04-26 6:18 ` Tiwei Bie
2019-06-06 13:30 ` Marc-André Lureau
1 sibling, 0 replies; 4+ messages in thread
From: Tiwei Bie @ 2019-04-26 6:18 UTC (permalink / raw)
To: mst, qemu-devel; +Cc: cunming.liang, qemu-stable
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;
+ }
+ }
+ }
+
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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] vhost-user: fix reconnection support for host notifier
2019-04-26 6:18 [Qemu-devel] [PATCH] vhost-user: fix reconnection support for host notifier Tiwei Bie
2019-04-26 6:18 ` Tiwei Bie
@ 2019-06-06 13:30 ` Marc-André Lureau
2019-06-07 4:21 ` Tiwei Bie
1 sibling, 1 reply; 4+ messages in thread
From: Marc-André Lureau @ 2019-06-06 13:30 UTC (permalink / raw)
To: Tiwei Bie; +Cc: cunming.liang, qemu-stable, QEMU, Michael S. Tsirkin
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
>
>
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] vhost-user: fix reconnection support for host notifier
2019-06-06 13:30 ` Marc-André Lureau
@ 2019-06-07 4:21 ` Tiwei Bie
0 siblings, 0 replies; 4+ messages in thread
From: Tiwei Bie @ 2019-06-07 4:21 UTC (permalink / raw)
To: Marc-André Lureau
Cc: cunming.liang, qemu-stable, QEMU, Michael S. Tsirkin
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-07 4:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-26 6:18 [Qemu-devel] [PATCH] vhost-user: fix reconnection support for host notifier Tiwei Bie
2019-04-26 6:18 ` Tiwei Bie
2019-06-06 13:30 ` Marc-André Lureau
2019-06-07 4:21 ` Tiwei Bie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).