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.ben...@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.ben...@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 > > >