Re: [PATCH v3 7/7] hw/virtio: generalise CHR_EVENT_CLOSED handling

["Michael S. Tsirkin" <mst@redhat.com>]

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
> > 
>