| Message ID | 20230425172716.1033562-17-stefanha@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block: remove aio_disable_external() API | expand |
Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben: > virtio_queue_aio_detach_host_notifier() does two things: > 1. It removes the fd handler from the event loop. > 2. It processes the virtqueue one last time. > > The first step can be peformed by any thread and without taking the > AioContext lock. > > The second step may need the AioContext lock (depending on the device > implementation) and runs in the thread where request processing takes > place. virtio-blk and virtio-scsi therefore call > virtio_queue_aio_detach_host_notifier() from a BH that is scheduled in > AioContext > > Scheduling a BH is undesirable for .drained_begin() functions. The next > patch will introduce a .drained_begin() function that needs to call > virtio_queue_aio_detach_host_notifier(). Why is it undesirable? In my mental model, .drained_begin() is still free to start as many asynchronous things as it likes. The only important thing to take care of is that .drained_poll() returns true as long as the BH (or other asynchronous operation) is still pending. Of course, your way of doing things still seems to result in simpler code because you don't have to deal with a BH at all if you only really want the first part and not the second. > Move the virtqueue processing out to the callers of > virtio_queue_aio_detach_host_notifier() so that the function can be > called from any thread. This is in preparation for the next patch. Did you forget to remove it in virtio_queue_aio_detach_host_notifier()? If it's unchanged, I don't think the AioContext requirement is lifted. > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > hw/block/dataplane/virtio-blk.c | 2 ++ > hw/scsi/virtio-scsi-dataplane.c | 9 +++++++++ > 2 files changed, 11 insertions(+) > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c > index b28d81737e..bd7cc6e76b 100644 > --- a/hw/block/dataplane/virtio-blk.c > +++ b/hw/block/dataplane/virtio-blk.c > @@ -286,8 +286,10 @@ static void virtio_blk_data_plane_stop_bh(void *opaque) > > for (i = 0; i < s->conf->num_queues; i++) { > VirtQueue *vq = virtio_get_queue(s->vdev, i); > + EventNotifier *host_notifier = virtio_queue_get_host_notifier(vq); > > virtio_queue_aio_detach_host_notifier(vq, s->ctx); > + virtio_queue_host_notifier_read(host_notifier); > } > } The existing code in virtio_queue_aio_detach_host_notifier() has a comment before the read: /* Test and clear notifier before after disabling event, * in case poll callback didn't have time to run. */ Do we want to keep it around in the new places? (And also fix the "before after", I suppose, or replace it with a similar, but better comment that explains why we're reading here.) Kevin
On Thu, May 04, 2023 at 11:00:35PM +0200, Kevin Wolf wrote: > Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben: > > virtio_queue_aio_detach_host_notifier() does two things: > > 1. It removes the fd handler from the event loop. > > 2. It processes the virtqueue one last time. > > > > The first step can be peformed by any thread and without taking the > > AioContext lock. > > > > The second step may need the AioContext lock (depending on the device > > implementation) and runs in the thread where request processing takes > > place. virtio-blk and virtio-scsi therefore call > > virtio_queue_aio_detach_host_notifier() from a BH that is scheduled in > > AioContext > > > > Scheduling a BH is undesirable for .drained_begin() functions. The next > > patch will introduce a .drained_begin() function that needs to call > > virtio_queue_aio_detach_host_notifier(). > > Why is it undesirable? In my mental model, .drained_begin() is still > free to start as many asynchronous things as it likes. The only > important thing to take care of is that .drained_poll() returns true as > long as the BH (or other asynchronous operation) is still pending. > > Of course, your way of doing things still seems to result in simpler > code because you don't have to deal with a BH at all if you only really > want the first part and not the second. I have clarified this in the commit description. We can't wait synchronously, but we could wait asynchronously as you described. It's simpler to split the function instead of implementing async wait using .drained_poll(). > > > Move the virtqueue processing out to the callers of > > virtio_queue_aio_detach_host_notifier() so that the function can be > > called from any thread. This is in preparation for the next patch. > > Did you forget to remove it in virtio_queue_aio_detach_host_notifier()? > If it's unchanged, I don't think the AioContext requirement is lifted. Yes! Thank you :) > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > hw/block/dataplane/virtio-blk.c | 2 ++ > > hw/scsi/virtio-scsi-dataplane.c | 9 +++++++++ > > 2 files changed, 11 insertions(+) > > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c > > index b28d81737e..bd7cc6e76b 100644 > > --- a/hw/block/dataplane/virtio-blk.c > > +++ b/hw/block/dataplane/virtio-blk.c > > @@ -286,8 +286,10 @@ static void virtio_blk_data_plane_stop_bh(void *opaque) > > > > for (i = 0; i < s->conf->num_queues; i++) { > > VirtQueue *vq = virtio_get_queue(s->vdev, i); > > + EventNotifier *host_notifier = virtio_queue_get_host_notifier(vq); > > > > virtio_queue_aio_detach_host_notifier(vq, s->ctx); > > + virtio_queue_host_notifier_read(host_notifier); > > } > > } > > The existing code in virtio_queue_aio_detach_host_notifier() has a > comment before the read: > > /* Test and clear notifier before after disabling event, > * in case poll callback didn't have time to run. */ > > Do we want to keep it around in the new places? (And also fix the > "before after", I suppose, or replace it with a similar, but better > comment that explains why we're reading here.) I will add the comment. Stefan
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index b28d81737e..bd7cc6e76b 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -286,8 +286,10 @@ static void virtio_blk_data_plane_stop_bh(void *opaque) for (i = 0; i < s->conf->num_queues; i++) { VirtQueue *vq = virtio_get_queue(s->vdev, i); + EventNotifier *host_notifier = virtio_queue_get_host_notifier(vq); virtio_queue_aio_detach_host_notifier(vq, s->ctx); + virtio_queue_host_notifier_read(host_notifier); } } diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index 20bb91766e..81643445ed 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -71,12 +71,21 @@ static void virtio_scsi_dataplane_stop_bh(void *opaque) { VirtIOSCSI *s = opaque; VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); + EventNotifier *host_notifier; int i; virtio_queue_aio_detach_host_notifier(vs->ctrl_vq, s->ctx); + host_notifier = virtio_queue_get_host_notifier(vs->ctrl_vq); + virtio_queue_host_notifier_read(host_notifier); + virtio_queue_aio_detach_host_notifier(vs->event_vq, s->ctx); + host_notifier = virtio_queue_get_host_notifier(vs->event_vq); + virtio_queue_host_notifier_read(host_notifier); + for (i = 0; i < vs->conf.num_queues; i++) { virtio_queue_aio_detach_host_notifier(vs->cmd_vqs[i], s->ctx); + host_notifier = virtio_queue_get_host_notifier(vs->cmd_vqs[i]); + virtio_queue_host_notifier_read(host_notifier); } }
virtio_queue_aio_detach_host_notifier() does two things: 1. It removes the fd handler from the event loop. 2. It processes the virtqueue one last time. The first step can be peformed by any thread and without taking the AioContext lock. The second step may need the AioContext lock (depending on the device implementation) and runs in the thread where request processing takes place. virtio-blk and virtio-scsi therefore call virtio_queue_aio_detach_host_notifier() from a BH that is scheduled in AioContext Scheduling a BH is undesirable for .drained_begin() functions. The next patch will introduce a .drained_begin() function that needs to call virtio_queue_aio_detach_host_notifier(). Move the virtqueue processing out to the callers of virtio_queue_aio_detach_host_notifier() so that the function can be called from any thread. This is in preparation for the next patch. Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> --- hw/block/dataplane/virtio-blk.c | 2 ++ hw/scsi/virtio-scsi-dataplane.c | 9 +++++++++ 2 files changed, 11 insertions(+)