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

[Alex Bennée]

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