| Message ID | 1459937788-31904-5-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 6 Apr 2016 12:16:25 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > From: "Michael S. Tsirkin" <mst@redhat.com> > > In addition to handling IO in vcpu thread and in io thread, blk dataplane > introduces yet another mode: handling it by AioContext. > > Currently, this reuses the same handler as previous modes, > which triggers races as these were not designed to be reentrant. > Add instead a separate handler just for aio; this will make > it possible to disable regular handlers when dataplane is active. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/virtio/virtio.c | 36 ++++++++++++++++++++++++++++++++---- > include/hw/virtio/virtio.h | 3 +++ > 2 files changed, 35 insertions(+), 4 deletions(-) > > +static void virtio_queue_notify_aio_vq(VirtQueue *vq) > +{ > + if (vq->vring.desc && vq->handle_aio_output) { > + VirtIODevice *vdev = vq->vdev; > + > + trace_virtio_queue_notify(vdev, vq - vdev->vq, vq); > + vq->handle_aio_output(vdev, vq); > + } > +} > + So this avoids reentrancy, but might we miss one notify if ->handle_aio_output has already been unset? What am I missing?
On 06/04/2016 13:11, Cornelia Huck wrote: >> > +static void virtio_queue_notify_aio_vq(VirtQueue *vq) >> > +{ >> > + if (vq->vring.desc && vq->handle_aio_output) { >> > + VirtIODevice *vdev = vq->vdev; >> > + >> > + trace_virtio_queue_notify(vdev, vq - vdev->vq, vq); >> > + vq->handle_aio_output(vdev, vq); >> > + } >> > +} >> > + > So this avoids reentrancy, but might we miss one notify if > ->handle_aio_output has already been unset? What am I missing? Calling the notifier just before unset is handled by using "false, false" when unsetting the notifier, and only setting vq->handle_aio_output after the notifier has been unset. Patch 7 makes things clearer. Paolo
On Wed, 6 Apr 2016 14:09:12 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 06/04/2016 13:11, Cornelia Huck wrote: > >> > +static void virtio_queue_notify_aio_vq(VirtQueue *vq) > >> > +{ > >> > + if (vq->vring.desc && vq->handle_aio_output) { > >> > + VirtIODevice *vdev = vq->vdev; > >> > + > >> > + trace_virtio_queue_notify(vdev, vq - vdev->vq, vq); > >> > + vq->handle_aio_output(vdev, vq); > >> > + } > >> > +} > >> > + > > So this avoids reentrancy, but might we miss one notify if > > ->handle_aio_output has already been unset? What am I missing? > > Calling the notifier just before unset is handled by using "false, > false" when unsetting the notifier, and only setting > vq->handle_aio_output after the notifier has been unset. Actually, the code is not quite right until the next two patches have been applied, but I think we can live with that. > Patch 7 makes things clearer. That as well.
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 264d4f6..eb04ac0 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -96,6 +96,7 @@ struct VirtQueue uint16_t vector; void (*handle_output)(VirtIODevice *vdev, VirtQueue *vq); + void (*handle_aio_output)(VirtIODevice *vdev, VirtQueue *vq); VirtIODevice *vdev; EventNotifier guest_notifier; EventNotifier host_notifier; @@ -1088,6 +1089,16 @@ void virtio_queue_set_align(VirtIODevice *vdev, int n, int align) virtio_queue_update_rings(vdev, n); } +static void virtio_queue_notify_aio_vq(VirtQueue *vq) +{ + if (vq->vring.desc && vq->handle_aio_output) { + VirtIODevice *vdev = vq->vdev; + + trace_virtio_queue_notify(vdev, vq - vdev->vq, vq); + vq->handle_aio_output(vdev, vq); + } +} + static void virtio_queue_notify_vq(VirtQueue *vq) { if (vq->vring.desc && vq->handle_output) { @@ -1143,10 +1154,19 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, vdev->vq[i].vring.num_default = queue_size; vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN; vdev->vq[i].handle_output = handle_output; + vdev->vq[i].handle_aio_output = NULL; return &vdev->vq[i]; } +void virtio_set_queue_aio(VirtQueue *vq, + void (*handle_output)(VirtIODevice *, VirtQueue *)) +{ + assert(vq->handle_output); + + vq->handle_aio_output = handle_output; +} + void virtio_del_queue(VirtIODevice *vdev, int n) { if (n < 0 || n >= VIRTIO_QUEUE_MAX) { @@ -1780,11 +1800,11 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq) return &vq->guest_notifier; } -static void virtio_queue_host_notifier_read(EventNotifier *n) +static void virtio_queue_host_notifier_aio_read(EventNotifier *n) { VirtQueue *vq = container_of(n, VirtQueue, host_notifier); if (event_notifier_test_and_clear(n)) { - virtio_queue_notify_vq(vq); + virtio_queue_notify_aio_vq(vq); } } @@ -1793,14 +1813,22 @@ void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, { if (assign && set_handler) { aio_set_event_notifier(ctx, &vq->host_notifier, true, - virtio_queue_host_notifier_read); + virtio_queue_host_notifier_aio_read); } else { aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL); } if (!assign) { /* Test and clear notifier before after disabling event, * in case poll callback didn't have time to run. */ - virtio_queue_host_notifier_read(&vq->host_notifier); + virtio_queue_host_notifier_aio_read(&vq->host_notifier); + } +} + +static void virtio_queue_host_notifier_read(EventNotifier *n) +{ + VirtQueue *vq = container_of(n, VirtQueue, host_notifier); + if (event_notifier_test_and_clear(n)) { + virtio_queue_notify_vq(vq); } } diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 5afb51c..fa3f93b 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -142,6 +142,9 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, void (*handle_output)(VirtIODevice *, VirtQueue *)); +void virtio_set_queue_aio(VirtQueue *vq, + void (*handle_output)(VirtIODevice *, VirtQueue *)); + void virtio_del_queue(VirtIODevice *vdev, int n); void *virtqueue_alloc_element(size_t sz, unsigned out_num, unsigned in_num);