| Message ID | 20221128164105.1191058-8-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix vhost-user issues with CI | expand |
> 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 >>>> >>> >>
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.
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,
..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(-)