..and use for both virtio-user-blk and virtio-user-gpio. This avoids
the circular close by deferring shutdown due to disconnection until a
later point. virtio-user-blk already had this mechanism in place so
generalise it as a vhost-user helper function and use for both blk and
gpio devices.
While we are at it we also fix up vhost-user-gpio to re-establish the
event handler after close down so we can reconnect later.
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
include/hw/virtio/vhost-user.h | 18 +++++++++
hw/block/vhost-user-blk.c | 41 +++-----------------
hw/virtio/vhost-user-gpio.c | 11 +++++-
hw/virtio/vhost-user.c | 71 ++++++++++++++++++++++++++++++++++
4 files changed, 104 insertions(+), 37 deletions(-)
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index c6e693cd3f..191216a74f 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
*/
void vhost_user_cleanup(VhostUserState *user);
+/**
+ * vhost_user_async_close() - cleanup vhost-user post connection drop
+ * @d: DeviceState for the associated device (passed to callback)
+ * @chardev: the CharBackend associated with the connection
+ * @vhost: the common vhost device
+ * @cb: the user callback function to complete the clean-up
+ *
+ * This function is used to handle the shutdown of a vhost-user
+ * connection to a backend. We handle this centrally to make sure we
+ * do all the steps and handle potential races due to VM shutdowns.
+ * Once the connection is disabled we call a backhalf to ensure
+ */
+typedef void (*vu_async_close_fn)(DeviceState *cb);
+
+void vhost_user_async_close(DeviceState *d,
+ CharBackend *chardev, struct vhost_dev *vhost,
+ vu_async_close_fn cb);
+
#endif
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 1177064631..aff4d2b8cb 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
vhost_user_blk_stop(vdev);
vhost_dev_cleanup(&s->dev);
-}
-static void vhost_user_blk_chr_closed_bh(void *opaque)
-{
- DeviceState *dev = opaque;
- VirtIODevice *vdev = VIRTIO_DEVICE(dev);
- VHostUserBlk *s = VHOST_USER_BLK(vdev);
-
- vhost_user_blk_disconnect(dev);
+ /* Re-instate the event handler for new connections */
qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
- NULL, opaque, NULL, true);
+ NULL, dev, NULL, true);
}
static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
@@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
}
break;
case CHR_EVENT_CLOSED:
- if (!runstate_check(RUN_STATE_SHUTDOWN)) {
- /*
- * A close event may happen during a read/write, but vhost
- * code assumes the vhost_dev remains setup, so delay the
- * stop & clear.
- */
- AioContext *ctx = qemu_get_current_aio_context();
-
- qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
- NULL, NULL, false);
- aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
-
- /*
- * Move vhost device to the stopped state. The vhost-user device
- * will be clean up and disconnected in BH. This can be useful in
- * the vhost migration code. If disconnect was caught there is an
- * option for the general vhost code to get the dev state without
- * knowing its type (in this case vhost-user).
- *
- * FIXME: this is sketchy to be reaching into vhost_dev
- * now because we are forcing something that implies we
- * have executed vhost_dev_stop() but that won't happen
- * until vhost_user_blk_stop() gets called from the bh.
- * Really this state check should be tracked locally.
- */
- s->dev.started = false;
- }
+ /* defer close until later to avoid circular close */
+ vhost_user_async_close(dev, &s->chardev, &s->dev,
+ vhost_user_blk_disconnect);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 75e28bcd3b..cd76287766 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
return 0;
}
+static void vu_gpio_event(void *opaque, QEMUChrEvent event);
+
static void vu_gpio_disconnect(DeviceState *dev)
{
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
vu_gpio_stop(vdev);
vhost_dev_cleanup(&gpio->vhost_dev);
+
+ /* Re-instate the event handler for new connections */
+ qemu_chr_fe_set_handlers(&gpio->chardev,
+ NULL, NULL, vu_gpio_event,
+ NULL, dev, NULL, true);
}
static void vu_gpio_event(void *opaque, QEMUChrEvent event)
@@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
}
break;
case CHR_EVENT_CLOSED:
- vu_gpio_disconnect(dev);
+ /* defer close until later to avoid circular close */
+ vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
+ vu_gpio_disconnect);
break;
case CHR_EVENT_BREAK:
case CHR_EVENT_MUX_IN:
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index abe23d4ebe..8f635844af 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -21,6 +21,7 @@
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
#include "qemu/sockets.h"
+#include "sysemu/runstate.h"
#include "sysemu/cryptodev.h"
#include "migration/migration.h"
#include "migration/postcopy-ram.h"
@@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
user->chr = NULL;
}
+
+typedef struct {
+ vu_async_close_fn cb;
+ DeviceState *dev;
+ CharBackend *cd;
+ struct vhost_dev *vhost;
+} VhostAsyncCallback;
+
+static void vhost_user_async_close_bh(void *opaque)
+{
+ VhostAsyncCallback *data = opaque;
+ struct vhost_dev *vhost = data->vhost;
+
+ /*
+ * If the vhost_dev has been cleared in the meantime there is
+ * nothing left to do as some other path has completed the
+ * cleanup.
+ */
+ if (vhost->vdev) {
+ data->cb(data->dev);
+ }
+
+ g_free(data);
+}
+
+/*
+ * We only schedule the work if the machine is running. If suspended
+ * we want to keep all the in-flight data as is for migration
+ * purposes.
+ */
+void vhost_user_async_close(DeviceState *d,
+ CharBackend *chardev, struct vhost_dev *vhost,
+ vu_async_close_fn cb)
+{
+ if (!runstate_check(RUN_STATE_SHUTDOWN)) {
+ /*
+ * A close event may happen during a read/write, but vhost
+ * code assumes the vhost_dev remains setup, so delay the
+ * stop & clear.
+ */
+ AioContext *ctx = qemu_get_current_aio_context();
+ VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
+
+ /* Save data for the callback */
+ data->cb = cb;
+ data->dev = d;
+ data->cd = chardev;
+ data->vhost = vhost;
+
+ /* Disable any further notifications on the chardev */
+ qemu_chr_fe_set_handlers(chardev,
+ NULL, NULL, NULL, NULL, NULL, NULL,
+ false);
+
+ aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
+
+ /*
+ * Move vhost device to the stopped state. The vhost-user device
+ * will be clean up and disconnected in BH. This can be useful in
+ * the vhost migration code. If disconnect was caught there is an
+ * option for the general vhost code to get the dev state without
+ * knowing its type (in this case vhost-user).
+ *
+ * Note if the vhost device is fully cleared by the time we
+ * execute the bottom half we won't continue with the cleanup.
+ */
+ vhost->started = false;
+ }
+}
+
static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
{
if (!virtio_has_feature(dev->protocol_features,
--
2.34.1
> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
> the circular close by deferring shutdown due to disconnection until a
> later point. virtio-user-blk already had this mechanism in place so
The mechanism was originally copied from virtio-net so we should probably fix it there too. AFAICT calling vhost_user_async_close() should work in net_vhost_user_event().
Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.
> generalise it as a vhost-user helper function and use for both blk and
> gpio devices.
>
> While we are at it we also fix up vhost-user-gpio to re-establish the
> event handler after close down so we can reconnect later.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/hw/virtio/vhost-user.h | 18 +++++++++
> hw/block/vhost-user-blk.c | 41 +++-----------------
> hw/virtio/vhost-user-gpio.c | 11 +++++-
> hw/virtio/vhost-user.c | 71 ++++++++++++++++++++++++++++++++++
> 4 files changed, 104 insertions(+), 37 deletions(-)
>
> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> index c6e693cd3f..191216a74f 100644
> --- a/include/hw/virtio/vhost-user.h
> +++ b/include/hw/virtio/vhost-user.h
> @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
> */
> void vhost_user_cleanup(VhostUserState *user);
>
> +/**
> + * vhost_user_async_close() - cleanup vhost-user post connection drop
> + * @d: DeviceState for the associated device (passed to callback)
> + * @chardev: the CharBackend associated with the connection
> + * @vhost: the common vhost device
> + * @cb: the user callback function to complete the clean-up
> + *
> + * This function is used to handle the shutdown of a vhost-user
> + * connection to a backend. We handle this centrally to make sure we
> + * do all the steps and handle potential races due to VM shutdowns.
> + * Once the connection is disabled we call a backhalf to ensure
> + */
> +typedef void (*vu_async_close_fn)(DeviceState *cb);
> +
> +void vhost_user_async_close(DeviceState *d,
> + CharBackend *chardev, struct vhost_dev *vhost,
> + vu_async_close_fn cb);
> +
> #endif
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 1177064631..aff4d2b8cb 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> vhost_user_blk_stop(vdev);
>
> vhost_dev_cleanup(&s->dev);
> -}
>
> -static void vhost_user_blk_chr_closed_bh(void *opaque)
> -{
> - DeviceState *dev = opaque;
> - VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> - VHostUserBlk *s = VHOST_USER_BLK(vdev);
> -
> - vhost_user_blk_disconnect(dev);
> + /* Re-instate the event handler for new connections */
> qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> - NULL, opaque, NULL, true);
> + NULL, dev, NULL, true);
> }
>
> static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> }
> break;
> case CHR_EVENT_CLOSED:
> - if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> - /*
> - * A close event may happen during a read/write, but vhost
> - * code assumes the vhost_dev remains setup, so delay the
> - * stop & clear.
> - */
> - AioContext *ctx = qemu_get_current_aio_context();
> -
> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> - NULL, NULL, false);
> - aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> -
> - /*
> - * Move vhost device to the stopped state. The vhost-user device
> - * will be clean up and disconnected in BH. This can be useful in
> - * the vhost migration code. If disconnect was caught there is an
> - * option for the general vhost code to get the dev state without
> - * knowing its type (in this case vhost-user).
> - *
> - * FIXME: this is sketchy to be reaching into vhost_dev
> - * now because we are forcing something that implies we
> - * have executed vhost_dev_stop() but that won't happen
> - * until vhost_user_blk_stop() gets called from the bh.
> - * Really this state check should be tracked locally.
> - */
> - s->dev.started = false;
> - }
> + /* defer close until later to avoid circular close */
> + vhost_user_async_close(dev, &s->chardev, &s->dev,
> + vhost_user_blk_disconnect);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index 75e28bcd3b..cd76287766 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
> return 0;
> }
>
> +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
> +
> static void vu_gpio_disconnect(DeviceState *dev)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
>
> vu_gpio_stop(vdev);
> vhost_dev_cleanup(&gpio->vhost_dev);
> +
> + /* Re-instate the event handler for new connections */
> + qemu_chr_fe_set_handlers(&gpio->chardev,
> + NULL, NULL, vu_gpio_event,
> + NULL, dev, NULL, true);
> }
>
> static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> }
> break;
> case CHR_EVENT_CLOSED:
> - vu_gpio_disconnect(dev);
> + /* defer close until later to avoid circular close */
> + vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
> + vu_gpio_disconnect);
> break;
> case CHR_EVENT_BREAK:
> case CHR_EVENT_MUX_IN:
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index abe23d4ebe..8f635844af 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -21,6 +21,7 @@
> #include "qemu/error-report.h"
> #include "qemu/main-loop.h"
> #include "qemu/sockets.h"
> +#include "sysemu/runstate.h"
> #include "sysemu/cryptodev.h"
> #include "migration/migration.h"
> #include "migration/postcopy-ram.h"
> @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
> user->chr = NULL;
> }
>
nit: Extra space
> +
> +typedef struct {
> + vu_async_close_fn cb;
> + DeviceState *dev;
> + CharBackend *cd;
> + struct vhost_dev *vhost;
> +} VhostAsyncCallback;
> +
> +static void vhost_user_async_close_bh(void *opaque)
> +{
> + VhostAsyncCallback *data = opaque;
> + struct vhost_dev *vhost = data->vhost;
> +
> + /*
> + * If the vhost_dev has been cleared in the meantime there is
> + * nothing left to do as some other path has completed the
> + * cleanup.
> + */
> + if (vhost->vdev) {
> + data->cb(data->dev);
> + }
> +
> + g_free(data);
> +}
> +
> +/*
> + * We only schedule the work if the machine is running. If suspended
> + * we want to keep all the in-flight data as is for migration
> + * purposes.
> + */
> +void vhost_user_async_close(DeviceState *d,
> + CharBackend *chardev, struct vhost_dev *vhost,
> + vu_async_close_fn cb)
> +{
> + if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> + /*
> + * A close event may happen during a read/write, but vhost
> + * code assumes the vhost_dev remains setup, so delay the
> + * stop & clear.
> + */
> + AioContext *ctx = qemu_get_current_aio_context();
> + VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
> +
> + /* Save data for the callback */
> + data->cb = cb;
> + data->dev = d;
> + data->cd = chardev;
> + data->vhost = vhost;
> +
> + /* Disable any further notifications on the chardev */
> + qemu_chr_fe_set_handlers(chardev,
> + NULL, NULL, NULL, NULL, NULL, NULL,
> + false);
> +
> + aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
> +
> + /*
> + * Move vhost device to the stopped state. The vhost-user device
Not this change’s fault but we should fix up the grammar here i.e. s/clean/cleaned/ and probably should be “disconnected in the BH”…etc.
> + * will be clean up and disconnected in BH. This can be useful in
> + * the vhost migration code. If disconnect was caught there is an
> + * option for the general vhost code to get the dev state without
> + * knowing its type (in this case vhost-user).
> + *
> + * Note if the vhost device is fully cleared by the time we
> + * execute the bottom half we won't continue with the cleanup.
> + */
> + vhost->started = false;
> + }
> +}
> +
> static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
> {
> if (!virtio_has_feature(dev->protocol_features,
> --
> 2.34.1
>
On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
> > On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> > ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
> > the circular close by deferring shutdown due to disconnection until a
> > later point. virtio-user-blk already had this mechanism in place so
>
> The mechanism was originally copied from virtio-net so we should probably fix it there too. AFAICT calling vhost_user_async_close() should work in net_vhost_user_event().
>
> Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.
If you do, separate patch pls and does not have to block this series.
>
> > generalise it as a vhost-user helper function and use for both blk and
> > gpio devices.
> >
> > While we are at it we also fix up vhost-user-gpio to re-establish the
> > event handler after close down so we can reconnect later.
> >
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> > include/hw/virtio/vhost-user.h | 18 +++++++++
> > hw/block/vhost-user-blk.c | 41 +++-----------------
> > hw/virtio/vhost-user-gpio.c | 11 +++++-
> > hw/virtio/vhost-user.c | 71 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 104 insertions(+), 37 deletions(-)
> >
> > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > index c6e693cd3f..191216a74f 100644
> > --- a/include/hw/virtio/vhost-user.h
> > +++ b/include/hw/virtio/vhost-user.h
> > @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
> > */
> > void vhost_user_cleanup(VhostUserState *user);
> >
> > +/**
> > + * vhost_user_async_close() - cleanup vhost-user post connection drop
> > + * @d: DeviceState for the associated device (passed to callback)
> > + * @chardev: the CharBackend associated with the connection
> > + * @vhost: the common vhost device
> > + * @cb: the user callback function to complete the clean-up
> > + *
> > + * This function is used to handle the shutdown of a vhost-user
> > + * connection to a backend. We handle this centrally to make sure we
> > + * do all the steps and handle potential races due to VM shutdowns.
> > + * Once the connection is disabled we call a backhalf to ensure
> > + */
> > +typedef void (*vu_async_close_fn)(DeviceState *cb);
> > +
> > +void vhost_user_async_close(DeviceState *d,
> > + CharBackend *chardev, struct vhost_dev *vhost,
> > + vu_async_close_fn cb);
> > +
> > #endif
> > diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> > index 1177064631..aff4d2b8cb 100644
> > --- a/hw/block/vhost-user-blk.c
> > +++ b/hw/block/vhost-user-blk.c
> > @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> > vhost_user_blk_stop(vdev);
> >
> > vhost_dev_cleanup(&s->dev);
> > -}
> >
> > -static void vhost_user_blk_chr_closed_bh(void *opaque)
> > -{
> > - DeviceState *dev = opaque;
> > - VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > - VHostUserBlk *s = VHOST_USER_BLK(vdev);
> > -
> > - vhost_user_blk_disconnect(dev);
> > + /* Re-instate the event handler for new connections */
> > qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> > - NULL, opaque, NULL, true);
> > + NULL, dev, NULL, true);
> > }
> >
> > static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> > @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> > }
> > break;
> > case CHR_EVENT_CLOSED:
> > - if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> > - /*
> > - * A close event may happen during a read/write, but vhost
> > - * code assumes the vhost_dev remains setup, so delay the
> > - * stop & clear.
> > - */
> > - AioContext *ctx = qemu_get_current_aio_context();
> > -
> > - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> > - NULL, NULL, false);
> > - aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> > -
> > - /*
> > - * Move vhost device to the stopped state. The vhost-user device
> > - * will be clean up and disconnected in BH. This can be useful in
> > - * the vhost migration code. If disconnect was caught there is an
> > - * option for the general vhost code to get the dev state without
> > - * knowing its type (in this case vhost-user).
> > - *
> > - * FIXME: this is sketchy to be reaching into vhost_dev
> > - * now because we are forcing something that implies we
> > - * have executed vhost_dev_stop() but that won't happen
> > - * until vhost_user_blk_stop() gets called from the bh.
> > - * Really this state check should be tracked locally.
> > - */
> > - s->dev.started = false;
> > - }
> > + /* defer close until later to avoid circular close */
> > + vhost_user_async_close(dev, &s->chardev, &s->dev,
> > + vhost_user_blk_disconnect);
> > break;
> > case CHR_EVENT_BREAK:
> > case CHR_EVENT_MUX_IN:
> > diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> > index 75e28bcd3b..cd76287766 100644
> > --- a/hw/virtio/vhost-user-gpio.c
> > +++ b/hw/virtio/vhost-user-gpio.c
> > @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
> > return 0;
> > }
> >
> > +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
> > +
> > static void vu_gpio_disconnect(DeviceState *dev)
> > {
> > VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
> >
> > vu_gpio_stop(vdev);
> > vhost_dev_cleanup(&gpio->vhost_dev);
> > +
> > + /* Re-instate the event handler for new connections */
> > + qemu_chr_fe_set_handlers(&gpio->chardev,
> > + NULL, NULL, vu_gpio_event,
> > + NULL, dev, NULL, true);
> > }
> >
> > static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> > @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> > }
> > break;
> > case CHR_EVENT_CLOSED:
> > - vu_gpio_disconnect(dev);
> > + /* defer close until later to avoid circular close */
> > + vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
> > + vu_gpio_disconnect);
> > break;
> > case CHR_EVENT_BREAK:
> > case CHR_EVENT_MUX_IN:
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index abe23d4ebe..8f635844af 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -21,6 +21,7 @@
> > #include "qemu/error-report.h"
> > #include "qemu/main-loop.h"
> > #include "qemu/sockets.h"
> > +#include "sysemu/runstate.h"
> > #include "sysemu/cryptodev.h"
> > #include "migration/migration.h"
> > #include "migration/postcopy-ram.h"
> > @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
> > user->chr = NULL;
> > }
> >
>
> nit: Extra space
>
> > +
> > +typedef struct {
> > + vu_async_close_fn cb;
> > + DeviceState *dev;
> > + CharBackend *cd;
> > + struct vhost_dev *vhost;
> > +} VhostAsyncCallback;
> > +
> > +static void vhost_user_async_close_bh(void *opaque)
> > +{
> > + VhostAsyncCallback *data = opaque;
> > + struct vhost_dev *vhost = data->vhost;
> > +
> > + /*
> > + * If the vhost_dev has been cleared in the meantime there is
> > + * nothing left to do as some other path has completed the
> > + * cleanup.
> > + */
> > + if (vhost->vdev) {
> > + data->cb(data->dev);
> > + }
> > +
> > + g_free(data);
> > +}
> > +
> > +/*
> > + * We only schedule the work if the machine is running. If suspended
> > + * we want to keep all the in-flight data as is for migration
> > + * purposes.
> > + */
> > +void vhost_user_async_close(DeviceState *d,
> > + CharBackend *chardev, struct vhost_dev *vhost,
> > + vu_async_close_fn cb)
> > +{
> > + if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> > + /*
> > + * A close event may happen during a read/write, but vhost
> > + * code assumes the vhost_dev remains setup, so delay the
> > + * stop & clear.
> > + */
> > + AioContext *ctx = qemu_get_current_aio_context();
> > + VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
> > +
> > + /* Save data for the callback */
> > + data->cb = cb;
> > + data->dev = d;
> > + data->cd = chardev;
> > + data->vhost = vhost;
> > +
> > + /* Disable any further notifications on the chardev */
> > + qemu_chr_fe_set_handlers(chardev,
> > + NULL, NULL, NULL, NULL, NULL, NULL,
> > + false);
> > +
> > + aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
> > +
> > + /*
> > + * Move vhost device to the stopped state. The vhost-user device
>
> Not this change’s fault but we should fix up the grammar here i.e. s/clean/cleaned/ and probably should be “disconnected in the BH”…etc.
>
> > + * will be clean up and disconnected in BH. This can be useful in
> > + * the vhost migration code. If disconnect was caught there is an
> > + * option for the general vhost code to get the dev state without
> > + * knowing its type (in this case vhost-user).
> > + *
> > + * Note if the vhost device is fully cleared by the time we
> > + * execute the bottom half we won't continue with the cleanup.
> > + */
> > + vhost->started = false;
> > + }
> > +}
> > +
> > static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
> > {
> > if (!virtio_has_feature(dev->protocol_features,
> > --
> > 2.34.1
> >
>
> On Nov 29, 2022, at 12:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
>>> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
>>> the circular close by deferring shutdown due to disconnection until a
>>> later point. virtio-user-blk already had this mechanism in place so
>>
>> The mechanism was originally copied from virtio-net so we should probably fix it there too. AFAICT calling vhost_user_async_close() should work in net_vhost_user_event().
>>
>> Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.
>
> If you do, separate patch pls and does not have to block this series.
If the series is urgent my comments can be addressed later.
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
>
>>
>>> generalise it as a vhost-user helper function and use for both blk and
>>> gpio devices.
>>>
>>> While we are at it we also fix up vhost-user-gpio to re-establish the
>>> event handler after close down so we can reconnect later.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>> include/hw/virtio/vhost-user.h | 18 +++++++++
>>> hw/block/vhost-user-blk.c | 41 +++-----------------
>>> hw/virtio/vhost-user-gpio.c | 11 +++++-
>>> hw/virtio/vhost-user.c | 71 ++++++++++++++++++++++++++++++++++
>>> 4 files changed, 104 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
>>> index c6e693cd3f..191216a74f 100644
>>> --- a/include/hw/virtio/vhost-user.h
>>> +++ b/include/hw/virtio/vhost-user.h
>>> @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
>>> */
>>> void vhost_user_cleanup(VhostUserState *user);
>>>
>>> +/**
>>> + * vhost_user_async_close() - cleanup vhost-user post connection drop
>>> + * @d: DeviceState for the associated device (passed to callback)
>>> + * @chardev: the CharBackend associated with the connection
>>> + * @vhost: the common vhost device
>>> + * @cb: the user callback function to complete the clean-up
>>> + *
>>> + * This function is used to handle the shutdown of a vhost-user
>>> + * connection to a backend. We handle this centrally to make sure we
>>> + * do all the steps and handle potential races due to VM shutdowns.
>>> + * Once the connection is disabled we call a backhalf to ensure
>>> + */
>>> +typedef void (*vu_async_close_fn)(DeviceState *cb);
>>> +
>>> +void vhost_user_async_close(DeviceState *d,
>>> + CharBackend *chardev, struct vhost_dev *vhost,
>>> + vu_async_close_fn cb);
>>> +
>>> #endif
>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>> index 1177064631..aff4d2b8cb 100644
>>> --- a/hw/block/vhost-user-blk.c
>>> +++ b/hw/block/vhost-user-blk.c
>>> @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>>> vhost_user_blk_stop(vdev);
>>>
>>> vhost_dev_cleanup(&s->dev);
>>> -}
>>>
>>> -static void vhost_user_blk_chr_closed_bh(void *opaque)
>>> -{
>>> - DeviceState *dev = opaque;
>>> - VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>> - VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>> -
>>> - vhost_user_blk_disconnect(dev);
>>> + /* Re-instate the event handler for new connections */
>>> qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
>>> - NULL, opaque, NULL, true);
>>> + NULL, dev, NULL, true);
>>> }
>>>
>>> static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>>> @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>>> }
>>> break;
>>> case CHR_EVENT_CLOSED:
>>> - if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>>> - /*
>>> - * A close event may happen during a read/write, but vhost
>>> - * code assumes the vhost_dev remains setup, so delay the
>>> - * stop & clear.
>>> - */
>>> - AioContext *ctx = qemu_get_current_aio_context();
>>> -
>>> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
>>> - NULL, NULL, false);
>>> - aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
>>> -
>>> - /*
>>> - * Move vhost device to the stopped state. The vhost-user device
>>> - * will be clean up and disconnected in BH. This can be useful in
>>> - * the vhost migration code. If disconnect was caught there is an
>>> - * option for the general vhost code to get the dev state without
>>> - * knowing its type (in this case vhost-user).
>>> - *
>>> - * FIXME: this is sketchy to be reaching into vhost_dev
>>> - * now because we are forcing something that implies we
>>> - * have executed vhost_dev_stop() but that won't happen
>>> - * until vhost_user_blk_stop() gets called from the bh.
>>> - * Really this state check should be tracked locally.
>>> - */
>>> - s->dev.started = false;
>>> - }
>>> + /* defer close until later to avoid circular close */
>>> + vhost_user_async_close(dev, &s->chardev, &s->dev,
>>> + vhost_user_blk_disconnect);
>>> break;
>>> case CHR_EVENT_BREAK:
>>> case CHR_EVENT_MUX_IN:
>>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>>> index 75e28bcd3b..cd76287766 100644
>>> --- a/hw/virtio/vhost-user-gpio.c
>>> +++ b/hw/virtio/vhost-user-gpio.c
>>> @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
>>> return 0;
>>> }
>>>
>>> +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
>>> +
>>> static void vu_gpio_disconnect(DeviceState *dev)
>>> {
>>> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>> @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
>>>
>>> vu_gpio_stop(vdev);
>>> vhost_dev_cleanup(&gpio->vhost_dev);
>>> +
>>> + /* Re-instate the event handler for new connections */
>>> + qemu_chr_fe_set_handlers(&gpio->chardev,
>>> + NULL, NULL, vu_gpio_event,
>>> + NULL, dev, NULL, true);
>>> }
>>>
>>> static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>>> @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>>> }
>>> break;
>>> case CHR_EVENT_CLOSED:
>>> - vu_gpio_disconnect(dev);
>>> + /* defer close until later to avoid circular close */
>>> + vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
>>> + vu_gpio_disconnect);
>>> break;
>>> case CHR_EVENT_BREAK:
>>> case CHR_EVENT_MUX_IN:
>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>> index abe23d4ebe..8f635844af 100644
>>> --- a/hw/virtio/vhost-user.c
>>> +++ b/hw/virtio/vhost-user.c
>>> @@ -21,6 +21,7 @@
>>> #include "qemu/error-report.h"
>>> #include "qemu/main-loop.h"
>>> #include "qemu/sockets.h"
>>> +#include "sysemu/runstate.h"
>>> #include "sysemu/cryptodev.h"
>>> #include "migration/migration.h"
>>> #include "migration/postcopy-ram.h"
>>> @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
>>> user->chr = NULL;
>>> }
>>>
>>
>> nit: Extra space
>>
>>> +
>>> +typedef struct {
>>> + vu_async_close_fn cb;
>>> + DeviceState *dev;
>>> + CharBackend *cd;
>>> + struct vhost_dev *vhost;
>>> +} VhostAsyncCallback;
>>> +
>>> +static void vhost_user_async_close_bh(void *opaque)
>>> +{
>>> + VhostAsyncCallback *data = opaque;
>>> + struct vhost_dev *vhost = data->vhost;
>>> +
>>> + /*
>>> + * If the vhost_dev has been cleared in the meantime there is
>>> + * nothing left to do as some other path has completed the
>>> + * cleanup.
>>> + */
>>> + if (vhost->vdev) {
>>> + data->cb(data->dev);
>>> + }
>>> +
>>> + g_free(data);
>>> +}
>>> +
>>> +/*
>>> + * We only schedule the work if the machine is running. If suspended
>>> + * we want to keep all the in-flight data as is for migration
>>> + * purposes.
>>> + */
>>> +void vhost_user_async_close(DeviceState *d,
>>> + CharBackend *chardev, struct vhost_dev *vhost,
>>> + vu_async_close_fn cb)
>>> +{
>>> + if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>>> + /*
>>> + * A close event may happen during a read/write, but vhost
>>> + * code assumes the vhost_dev remains setup, so delay the
>>> + * stop & clear.
>>> + */
>>> + AioContext *ctx = qemu_get_current_aio_context();
>>> + VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
>>> +
>>> + /* Save data for the callback */
>>> + data->cb = cb;
>>> + data->dev = d;
>>> + data->cd = chardev;
>>> + data->vhost = vhost;
>>> +
>>> + /* Disable any further notifications on the chardev */
>>> + qemu_chr_fe_set_handlers(chardev,
>>> + NULL, NULL, NULL, NULL, NULL, NULL,
>>> + false);
>>> +
>>> + aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
>>> +
>>> + /*
>>> + * Move vhost device to the stopped state. The vhost-user device
>>
>> Not this change’s fault but we should fix up the grammar here i.e. s/clean/cleaned/ and probably should be “disconnected in the BH”…etc.
>>
>>> + * will be clean up and disconnected in BH. This can be useful in
>>> + * the vhost migration code. If disconnect was caught there is an
>>> + * option for the general vhost code to get the dev state without
>>> + * knowing its type (in this case vhost-user).
>>> + *
>>> + * Note if the vhost device is fully cleared by the time we
>>> + * execute the bottom half we won't continue with the cleanup.
>>> + */
>>> + vhost->started = false;
>>> + }
>>> +}
>>> +
>>> static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
>>> {
>>> if (!virtio_has_feature(dev->protocol_features,
>>> --
>>> 2.34.1
>>>
>>
>
Raphael Norwitz <raphael.norwitz@nutanix.com> writes:
>> On Nov 29, 2022, at 12:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
>>>> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
>>>> the circular close by deferring shutdown due to disconnection until a
>>>> later point. virtio-user-blk already had this mechanism in place so
>>>
>>> The mechanism was originally copied from virtio-net so we should
>>> probably fix it there too. AFAICT calling vhost_user_async_close()
>>> should work in net_vhost_user_event().
>>>
>>> Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.
>>
>> If you do, separate patch pls and does not have to block this series.
>
> If the series is urgent my comments can be addressed later.
On the surface it looks similar but the vhost-net code doesn't deal in
DeviceState opaques and also has invented a s->watch variable for
manually removing gio sources. I'm not sure I'm confident I can
re-factor this code and not break something so close to release.
>
> Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
>
>>
>>>
>>>> generalise it as a vhost-user helper function and use for both blk and
>>>> gpio devices.
>>>>
>>>> While we are at it we also fix up vhost-user-gpio to re-establish the
>>>> event handler after close down so we can reconnect later.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> ---
>>>> include/hw/virtio/vhost-user.h | 18 +++++++++
>>>> hw/block/vhost-user-blk.c | 41 +++-----------------
>>>> hw/virtio/vhost-user-gpio.c | 11 +++++-
>>>> hw/virtio/vhost-user.c | 71 ++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 104 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
>>>> index c6e693cd3f..191216a74f 100644
>>>> --- a/include/hw/virtio/vhost-user.h
>>>> +++ b/include/hw/virtio/vhost-user.h
>>>> @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
>>>> */
>>>> void vhost_user_cleanup(VhostUserState *user);
>>>>
>>>> +/**
>>>> + * vhost_user_async_close() - cleanup vhost-user post connection drop
>>>> + * @d: DeviceState for the associated device (passed to callback)
>>>> + * @chardev: the CharBackend associated with the connection
>>>> + * @vhost: the common vhost device
>>>> + * @cb: the user callback function to complete the clean-up
>>>> + *
>>>> + * This function is used to handle the shutdown of a vhost-user
>>>> + * connection to a backend. We handle this centrally to make sure we
>>>> + * do all the steps and handle potential races due to VM shutdowns.
>>>> + * Once the connection is disabled we call a backhalf to ensure
>>>> + */
>>>> +typedef void (*vu_async_close_fn)(DeviceState *cb);
>>>> +
>>>> +void vhost_user_async_close(DeviceState *d,
>>>> + CharBackend *chardev, struct vhost_dev *vhost,
>>>> + vu_async_close_fn cb);
>>>> +
>>>> #endif
>>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>>>> index 1177064631..aff4d2b8cb 100644
>>>> --- a/hw/block/vhost-user-blk.c
>>>> +++ b/hw/block/vhost-user-blk.c
>>>> @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
>>>> vhost_user_blk_stop(vdev);
>>>>
>>>> vhost_dev_cleanup(&s->dev);
>>>> -}
>>>>
>>>> -static void vhost_user_blk_chr_closed_bh(void *opaque)
>>>> -{
>>>> - DeviceState *dev = opaque;
>>>> - VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>> - VHostUserBlk *s = VHOST_USER_BLK(vdev);
>>>> -
>>>> - vhost_user_blk_disconnect(dev);
>>>> + /* Re-instate the event handler for new connections */
>>>> qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
>>>> - NULL, opaque, NULL, true);
>>>> + NULL, dev, NULL, true);
>>>> }
>>>>
>>>> static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>>>> @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>>>> }
>>>> break;
>>>> case CHR_EVENT_CLOSED:
>>>> - if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>>>> - /*
>>>> - * A close event may happen during a read/write, but vhost
>>>> - * code assumes the vhost_dev remains setup, so delay the
>>>> - * stop & clear.
>>>> - */
>>>> - AioContext *ctx = qemu_get_current_aio_context();
>>>> -
>>>> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
>>>> - NULL, NULL, false);
>>>> - aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
>>>> -
>>>> - /*
>>>> - * Move vhost device to the stopped state. The vhost-user device
>>>> - * will be clean up and disconnected in BH. This can be useful in
>>>> - * the vhost migration code. If disconnect was caught there is an
>>>> - * option for the general vhost code to get the dev state without
>>>> - * knowing its type (in this case vhost-user).
>>>> - *
>>>> - * FIXME: this is sketchy to be reaching into vhost_dev
>>>> - * now because we are forcing something that implies we
>>>> - * have executed vhost_dev_stop() but that won't happen
>>>> - * until vhost_user_blk_stop() gets called from the bh.
>>>> - * Really this state check should be tracked locally.
>>>> - */
>>>> - s->dev.started = false;
>>>> - }
>>>> + /* defer close until later to avoid circular close */
>>>> + vhost_user_async_close(dev, &s->chardev, &s->dev,
>>>> + vhost_user_blk_disconnect);
>>>> break;
>>>> case CHR_EVENT_BREAK:
>>>> case CHR_EVENT_MUX_IN:
>>>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
>>>> index 75e28bcd3b..cd76287766 100644
>>>> --- a/hw/virtio/vhost-user-gpio.c
>>>> +++ b/hw/virtio/vhost-user-gpio.c
>>>> @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
>>>> return 0;
>>>> }
>>>>
>>>> +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
>>>> +
>>>> static void vu_gpio_disconnect(DeviceState *dev)
>>>> {
>>>> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>> @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
>>>>
>>>> vu_gpio_stop(vdev);
>>>> vhost_dev_cleanup(&gpio->vhost_dev);
>>>> +
>>>> + /* Re-instate the event handler for new connections */
>>>> + qemu_chr_fe_set_handlers(&gpio->chardev,
>>>> + NULL, NULL, vu_gpio_event,
>>>> + NULL, dev, NULL, true);
>>>> }
>>>>
>>>> static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>>>> @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
>>>> }
>>>> break;
>>>> case CHR_EVENT_CLOSED:
>>>> - vu_gpio_disconnect(dev);
>>>> + /* defer close until later to avoid circular close */
>>>> + vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
>>>> + vu_gpio_disconnect);
>>>> break;
>>>> case CHR_EVENT_BREAK:
>>>> case CHR_EVENT_MUX_IN:
>>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>>>> index abe23d4ebe..8f635844af 100644
>>>> --- a/hw/virtio/vhost-user.c
>>>> +++ b/hw/virtio/vhost-user.c
>>>> @@ -21,6 +21,7 @@
>>>> #include "qemu/error-report.h"
>>>> #include "qemu/main-loop.h"
>>>> #include "qemu/sockets.h"
>>>> +#include "sysemu/runstate.h"
>>>> #include "sysemu/cryptodev.h"
>>>> #include "migration/migration.h"
>>>> #include "migration/postcopy-ram.h"
>>>> @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
>>>> user->chr = NULL;
>>>> }
>>>>
>>>
>>> nit: Extra space
>>>
>>>> +
>>>> +typedef struct {
>>>> + vu_async_close_fn cb;
>>>> + DeviceState *dev;
>>>> + CharBackend *cd;
>>>> + struct vhost_dev *vhost;
>>>> +} VhostAsyncCallback;
>>>> +
>>>> +static void vhost_user_async_close_bh(void *opaque)
>>>> +{
>>>> + VhostAsyncCallback *data = opaque;
>>>> + struct vhost_dev *vhost = data->vhost;
>>>> +
>>>> + /*
>>>> + * If the vhost_dev has been cleared in the meantime there is
>>>> + * nothing left to do as some other path has completed the
>>>> + * cleanup.
>>>> + */
>>>> + if (vhost->vdev) {
>>>> + data->cb(data->dev);
>>>> + }
>>>> +
>>>> + g_free(data);
>>>> +}
>>>> +
>>>> +/*
>>>> + * We only schedule the work if the machine is running. If suspended
>>>> + * we want to keep all the in-flight data as is for migration
>>>> + * purposes.
>>>> + */
>>>> +void vhost_user_async_close(DeviceState *d,
>>>> + CharBackend *chardev, struct vhost_dev *vhost,
>>>> + vu_async_close_fn cb)
>>>> +{
>>>> + if (!runstate_check(RUN_STATE_SHUTDOWN)) {
>>>> + /*
>>>> + * A close event may happen during a read/write, but vhost
>>>> + * code assumes the vhost_dev remains setup, so delay the
>>>> + * stop & clear.
>>>> + */
>>>> + AioContext *ctx = qemu_get_current_aio_context();
>>>> + VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
>>>> +
>>>> + /* Save data for the callback */
>>>> + data->cb = cb;
>>>> + data->dev = d;
>>>> + data->cd = chardev;
>>>> + data->vhost = vhost;
>>>> +
>>>> + /* Disable any further notifications on the chardev */
>>>> + qemu_chr_fe_set_handlers(chardev,
>>>> + NULL, NULL, NULL, NULL, NULL, NULL,
>>>> + false);
>>>> +
>>>> + aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
>>>> +
>>>> + /*
>>>> + * Move vhost device to the stopped state. The vhost-user device
>>>
>>> Not this change’s fault but we should fix up the grammar here i.e.
>>> s/clean/cleaned/ and probably should be “disconnected in the
>>> BH”…etc.
>>>
>>>> + * will be clean up and disconnected in BH. This can be useful in
>>>> + * the vhost migration code. If disconnect was caught there is an
>>>> + * option for the general vhost code to get the dev state without
>>>> + * knowing its type (in this case vhost-user).
>>>> + *
>>>> + * Note if the vhost device is fully cleared by the time we
>>>> + * execute the bottom half we won't continue with the cleanup.
>>>> + */
>>>> + vhost->started = false;
>>>> + }
>>>> +}
>>>> +
>>>> static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
>>>> {
>>>> if (!virtio_has_feature(dev->protocol_features,
>>>> --
>>>> 2.34.1
>>>>
>>>
>>
--
Alex Bennée
On Wed, Nov 30, 2022 at 10:25:58AM +0000, Alex Bennée wrote:
>
> Raphael Norwitz <raphael.norwitz@nutanix.com> writes:
>
> >> On Nov 29, 2022, at 12:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >> On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote:
> >>>> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>>>
> >>>> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids
> >>>> the circular close by deferring shutdown due to disconnection until a
> >>>> later point. virtio-user-blk already had this mechanism in place so
> >>>
> >>> The mechanism was originally copied from virtio-net so we should
> >>> probably fix it there too. AFAICT calling vhost_user_async_close()
> >>> should work in net_vhost_user_event().
> >>>
> >>> Otherwise the code looks good modulo a few nits. Happy to see the duplicated logic is being generalized.
> >>
> >> If you do, separate patch pls and does not have to block this series.
> >
> > If the series is urgent my comments can be addressed later.
>
> On the surface it looks similar but the vhost-net code doesn't deal in
> DeviceState opaques and also has invented a s->watch variable for
> manually removing gio sources. I'm not sure I'm confident I can
> re-factor this code and not break something so close to release.
OK, that's fair.
> >
> > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> >
> >>
> >>>
> >>>> generalise it as a vhost-user helper function and use for both blk and
> >>>> gpio devices.
> >>>>
> >>>> While we are at it we also fix up vhost-user-gpio to re-establish the
> >>>> event handler after close down so we can reconnect later.
> >>>>
> >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >>>> ---
> >>>> include/hw/virtio/vhost-user.h | 18 +++++++++
> >>>> hw/block/vhost-user-blk.c | 41 +++-----------------
> >>>> hw/virtio/vhost-user-gpio.c | 11 +++++-
> >>>> hw/virtio/vhost-user.c | 71 ++++++++++++++++++++++++++++++++++
> >>>> 4 files changed, 104 insertions(+), 37 deletions(-)
> >>>>
> >>>> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> >>>> index c6e693cd3f..191216a74f 100644
> >>>> --- a/include/hw/virtio/vhost-user.h
> >>>> +++ b/include/hw/virtio/vhost-user.h
> >>>> @@ -68,4 +68,22 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp);
> >>>> */
> >>>> void vhost_user_cleanup(VhostUserState *user);
> >>>>
> >>>> +/**
> >>>> + * vhost_user_async_close() - cleanup vhost-user post connection drop
> >>>> + * @d: DeviceState for the associated device (passed to callback)
> >>>> + * @chardev: the CharBackend associated with the connection
> >>>> + * @vhost: the common vhost device
> >>>> + * @cb: the user callback function to complete the clean-up
> >>>> + *
> >>>> + * This function is used to handle the shutdown of a vhost-user
> >>>> + * connection to a backend. We handle this centrally to make sure we
> >>>> + * do all the steps and handle potential races due to VM shutdowns.
> >>>> + * Once the connection is disabled we call a backhalf to ensure
> >>>> + */
> >>>> +typedef void (*vu_async_close_fn)(DeviceState *cb);
> >>>> +
> >>>> +void vhost_user_async_close(DeviceState *d,
> >>>> + CharBackend *chardev, struct vhost_dev *vhost,
> >>>> + vu_async_close_fn cb);
> >>>> +
> >>>> #endif
> >>>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> >>>> index 1177064631..aff4d2b8cb 100644
> >>>> --- a/hw/block/vhost-user-blk.c
> >>>> +++ b/hw/block/vhost-user-blk.c
> >>>> @@ -369,17 +369,10 @@ static void vhost_user_blk_disconnect(DeviceState *dev)
> >>>> vhost_user_blk_stop(vdev);
> >>>>
> >>>> vhost_dev_cleanup(&s->dev);
> >>>> -}
> >>>>
> >>>> -static void vhost_user_blk_chr_closed_bh(void *opaque)
> >>>> -{
> >>>> - DeviceState *dev = opaque;
> >>>> - VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>> - VHostUserBlk *s = VHOST_USER_BLK(vdev);
> >>>> -
> >>>> - vhost_user_blk_disconnect(dev);
> >>>> + /* Re-instate the event handler for new connections */
> >>>> qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, vhost_user_blk_event,
> >>>> - NULL, opaque, NULL, true);
> >>>> + NULL, dev, NULL, true);
> >>>> }
> >>>>
> >>>> static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> >>>> @@ -398,33 +391,9 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> >>>> }
> >>>> break;
> >>>> case CHR_EVENT_CLOSED:
> >>>> - if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> >>>> - /*
> >>>> - * A close event may happen during a read/write, but vhost
> >>>> - * code assumes the vhost_dev remains setup, so delay the
> >>>> - * stop & clear.
> >>>> - */
> >>>> - AioContext *ctx = qemu_get_current_aio_context();
> >>>> -
> >>>> - qemu_chr_fe_set_handlers(&s->chardev, NULL, NULL, NULL, NULL,
> >>>> - NULL, NULL, false);
> >>>> - aio_bh_schedule_oneshot(ctx, vhost_user_blk_chr_closed_bh, opaque);
> >>>> -
> >>>> - /*
> >>>> - * Move vhost device to the stopped state. The vhost-user device
> >>>> - * will be clean up and disconnected in BH. This can be useful in
> >>>> - * the vhost migration code. If disconnect was caught there is an
> >>>> - * option for the general vhost code to get the dev state without
> >>>> - * knowing its type (in this case vhost-user).
> >>>> - *
> >>>> - * FIXME: this is sketchy to be reaching into vhost_dev
> >>>> - * now because we are forcing something that implies we
> >>>> - * have executed vhost_dev_stop() but that won't happen
> >>>> - * until vhost_user_blk_stop() gets called from the bh.
> >>>> - * Really this state check should be tracked locally.
> >>>> - */
> >>>> - s->dev.started = false;
> >>>> - }
> >>>> + /* defer close until later to avoid circular close */
> >>>> + vhost_user_async_close(dev, &s->chardev, &s->dev,
> >>>> + vhost_user_blk_disconnect);
> >>>> break;
> >>>> case CHR_EVENT_BREAK:
> >>>> case CHR_EVENT_MUX_IN:
> >>>> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> >>>> index 75e28bcd3b..cd76287766 100644
> >>>> --- a/hw/virtio/vhost-user-gpio.c
> >>>> +++ b/hw/virtio/vhost-user-gpio.c
> >>>> @@ -239,6 +239,8 @@ static int vu_gpio_connect(DeviceState *dev, Error **errp)
> >>>> return 0;
> >>>> }
> >>>>
> >>>> +static void vu_gpio_event(void *opaque, QEMUChrEvent event);
> >>>> +
> >>>> static void vu_gpio_disconnect(DeviceState *dev)
> >>>> {
> >>>> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >>>> @@ -251,6 +253,11 @@ static void vu_gpio_disconnect(DeviceState *dev)
> >>>>
> >>>> vu_gpio_stop(vdev);
> >>>> vhost_dev_cleanup(&gpio->vhost_dev);
> >>>> +
> >>>> + /* Re-instate the event handler for new connections */
> >>>> + qemu_chr_fe_set_handlers(&gpio->chardev,
> >>>> + NULL, NULL, vu_gpio_event,
> >>>> + NULL, dev, NULL, true);
> >>>> }
> >>>>
> >>>> static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> >>>> @@ -268,7 +275,9 @@ static void vu_gpio_event(void *opaque, QEMUChrEvent event)
> >>>> }
> >>>> break;
> >>>> case CHR_EVENT_CLOSED:
> >>>> - vu_gpio_disconnect(dev);
> >>>> + /* defer close until later to avoid circular close */
> >>>> + vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
> >>>> + vu_gpio_disconnect);
> >>>> break;
> >>>> case CHR_EVENT_BREAK:
> >>>> case CHR_EVENT_MUX_IN:
> >>>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >>>> index abe23d4ebe..8f635844af 100644
> >>>> --- a/hw/virtio/vhost-user.c
> >>>> +++ b/hw/virtio/vhost-user.c
> >>>> @@ -21,6 +21,7 @@
> >>>> #include "qemu/error-report.h"
> >>>> #include "qemu/main-loop.h"
> >>>> #include "qemu/sockets.h"
> >>>> +#include "sysemu/runstate.h"
> >>>> #include "sysemu/cryptodev.h"
> >>>> #include "migration/migration.h"
> >>>> #include "migration/postcopy-ram.h"
> >>>> @@ -2670,6 +2671,76 @@ void vhost_user_cleanup(VhostUserState *user)
> >>>> user->chr = NULL;
> >>>> }
> >>>>
> >>>
> >>> nit: Extra space
> >>>
> >>>> +
> >>>> +typedef struct {
> >>>> + vu_async_close_fn cb;
> >>>> + DeviceState *dev;
> >>>> + CharBackend *cd;
> >>>> + struct vhost_dev *vhost;
> >>>> +} VhostAsyncCallback;
> >>>> +
> >>>> +static void vhost_user_async_close_bh(void *opaque)
> >>>> +{
> >>>> + VhostAsyncCallback *data = opaque;
> >>>> + struct vhost_dev *vhost = data->vhost;
> >>>> +
> >>>> + /*
> >>>> + * If the vhost_dev has been cleared in the meantime there is
> >>>> + * nothing left to do as some other path has completed the
> >>>> + * cleanup.
> >>>> + */
> >>>> + if (vhost->vdev) {
> >>>> + data->cb(data->dev);
> >>>> + }
> >>>> +
> >>>> + g_free(data);
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * We only schedule the work if the machine is running. If suspended
> >>>> + * we want to keep all the in-flight data as is for migration
> >>>> + * purposes.
> >>>> + */
> >>>> +void vhost_user_async_close(DeviceState *d,
> >>>> + CharBackend *chardev, struct vhost_dev *vhost,
> >>>> + vu_async_close_fn cb)
> >>>> +{
> >>>> + if (!runstate_check(RUN_STATE_SHUTDOWN)) {
> >>>> + /*
> >>>> + * A close event may happen during a read/write, but vhost
> >>>> + * code assumes the vhost_dev remains setup, so delay the
> >>>> + * stop & clear.
> >>>> + */
> >>>> + AioContext *ctx = qemu_get_current_aio_context();
> >>>> + VhostAsyncCallback *data = g_new0(VhostAsyncCallback, 1);
> >>>> +
> >>>> + /* Save data for the callback */
> >>>> + data->cb = cb;
> >>>> + data->dev = d;
> >>>> + data->cd = chardev;
> >>>> + data->vhost = vhost;
> >>>> +
> >>>> + /* Disable any further notifications on the chardev */
> >>>> + qemu_chr_fe_set_handlers(chardev,
> >>>> + NULL, NULL, NULL, NULL, NULL, NULL,
> >>>> + false);
> >>>> +
> >>>> + aio_bh_schedule_oneshot(ctx, vhost_user_async_close_bh, data);
> >>>> +
> >>>> + /*
> >>>> + * Move vhost device to the stopped state. The vhost-user device
> >>>
> >>> Not this change’s fault but we should fix up the grammar here i.e.
> >>> s/clean/cleaned/ and probably should be “disconnected in the
> >>> BH”…etc.
> >>>
> >>>> + * will be clean up and disconnected in BH. This can be useful in
> >>>> + * the vhost migration code. If disconnect was caught there is an
> >>>> + * option for the general vhost code to get the dev state without
> >>>> + * knowing its type (in this case vhost-user).
> >>>> + *
> >>>> + * Note if the vhost device is fully cleared by the time we
> >>>> + * execute the bottom half we won't continue with the cleanup.
> >>>> + */
> >>>> + vhost->started = false;
> >>>> + }
> >>>> +}
> >>>> +
> >>>> static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
> >>>> {
> >>>> if (!virtio_has_feature(dev->protocol_features,
> >>>> --
> >>>> 2.34.1
> >>>>
> >>>
> >>
>
>
> --
> Alex Bennée
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Nov 30, 2022 at 10:25:58AM +0000, Alex Bennée wrote: >> >> Raphael Norwitz <raphael.norwitz@nutanix.com> writes: >> >> >> On Nov 29, 2022, at 12:30 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> >> >> >> On Tue, Nov 29, 2022 at 05:18:58AM +0000, Raphael Norwitz wrote: >> >>>> On Nov 28, 2022, at 11:41 AM, Alex Bennée <alex.bennee@linaro.org> wrote: >> >>>> >> >>>> ..and use for both virtio-user-blk and virtio-user-gpio. This avoids >> >>>> the circular close by deferring shutdown due to disconnection until a >> >>>> later point. virtio-user-blk already had this mechanism in place so >> >>> >> >>> The mechanism was originally copied from virtio-net so we should >> >>> probably fix it there too. AFAICT calling vhost_user_async_close() >> >>> should work in net_vhost_user_event(). >> >>> >> >>> Otherwise the code looks good modulo a few nits. Happy to see >> >>> the duplicated logic is being generalized. >> >> >> >> If you do, separate patch pls and does not have to block this series. >> > >> > If the series is urgent my comments can be addressed later. >> >> On the surface it looks similar but the vhost-net code doesn't deal in >> DeviceState opaques and also has invented a s->watch variable for >> manually removing gio sources. I'm not sure I'm confident I can >> re-factor this code and not break something so close to release. > > OK, that's fair. See 20221130112439.2527228-1-alex.bennee@linaro.org for the v4 series. -- Alex Bennée
© 2016 - 2026 Red Hat, Inc.