On Fri, Aug 5, 2022, 05:59 Paolo Bonzini <pbonz...@redhat.com> wrote:
> On 8/3/22 18:28, Stefan Hajnoczi wrote: > > diff --git a/hw/scsi/virtio-scsi-dataplane.c > b/hw/scsi/virtio-scsi-dataplane.c > > index 8bb6e6acfc..a575c3f0cd 100644 > > --- a/hw/scsi/virtio-scsi-dataplane.c > > +++ b/hw/scsi/virtio-scsi-dataplane.c > > @@ -66,6 +66,21 @@ static int virtio_scsi_set_host_notifier(VirtIOSCSI > *s, VirtQueue *vq, int n) > > return 0; > > } > > > > +/* Context: BH in IOThread */ > > +static void virtio_scsi_dataplane_start_bh(void *opaque) > > +{ > > + VirtIOSCSI *s = opaque; > > + VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); > > + int i; > > + > > + virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx); > > + virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx); > > + > > + for (i = 0; i < vs->conf.num_queues; i++) { > > + virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx); > > + } > > +} > > + > > /* Context: BH in IOThread */ > > static void virtio_scsi_dataplane_stop_bh(void *opaque) > > { > > @@ -136,16 +151,18 @@ int virtio_scsi_dataplane_start(VirtIODevice *vdev) > > > > memory_region_transaction_commit(); > > > > - aio_context_acquire(s->ctx); > > - virtio_queue_aio_attach_host_notifier(vs->ctrl_vq, s->ctx); > > - virtio_queue_aio_attach_host_notifier_no_poll(vs->event_vq, s->ctx); > > - > > - for (i = 0; i < vs->conf.num_queues; i++) { > > - virtio_queue_aio_attach_host_notifier(vs->cmd_vqs[i], s->ctx); > > - } > > - > > s->dataplane_starting = false; > > s->dataplane_started = true; > > + > > + /* > > + * Attach notifiers from within the IOThread. It's possible to > attach > > + * notifiers from our thread directly but this approach has the > advantages > > + * that virtio_scsi_dataplane_start_bh() is symmetric with > > + * virtio_scsi_dataplane_stop_bh() and the s->dataplane_started > assignment > > + * above doesn't require explicit synchronization. > > + */ > > + aio_context_acquire(s->ctx); > > + aio_wait_bh_oneshot(s->ctx, virtio_scsi_dataplane_start_bh, s); > > aio_context_release(s->ctx); > > Taking the AioContext lock for code that is running in another thread > seems wrong. But really there is no need to take the lock: I think it > wanted to protect against the handler running before > s->dataplane_starting/started were set, but it's not needed now because > the iothread is busy running the bottom half. > The lock must be taken solely because aio_wait_bh_oneshot() requires that the caller holds it. Emanuele is working on removing the lock from AIO_WAIT_WHILE() but for now it's necessary to hold the lock. > Also, please do the same in virtio-blk as well. > Okay. There are a few other differences between virtio-scsi and virtio-blk dataplane code that I'll investigate. Maybe they can share common start/stop functions. Stefan >