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

[Raphael Norwitz]

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