On Fri, Aug 5, 2022, 05:59 Paolo Bonzini <[email protected]> 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
>
