| Message ID | 1459516794-23629-9-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 01, 2016 at 03:19:53PM +0200, Paolo Bonzini wrote: > Eliminating the reentrancy is actually a nice thing that we can do > with the API that Michael proposed, so let's make it first class. > This also hides the complex assign/set_handler conventions from > callers of virtio_queue_aio_set_host_notifier_handler, which in > fact was always called with assign=true. > > Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/block/dataplane/virtio-blk.c | 7 +++---- > hw/scsi/virtio-scsi-dataplane.c | 12 ++++-------- > hw/virtio/virtio.c | 19 ++++--------------- > include/hw/virtio/virtio.h | 6 ++---- > 4 files changed, 13 insertions(+), 31 deletions(-) > > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c > index fd06726..74c6d37 100644 > --- a/hw/block/dataplane/virtio-blk.c > +++ b/hw/block/dataplane/virtio-blk.c > @@ -237,8 +237,8 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) > > /* Get this show started by hooking up our callbacks */ > aio_context_acquire(s->ctx); > - virtio_set_queue_aio(s->vq, virtio_blk_data_plane_handle_output); > - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true); > + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, > + virtio_blk_data_plane_handle_output); > aio_context_release(s->ctx); > return; > > @@ -273,8 +273,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) > aio_context_acquire(s->ctx); > > /* Stop notifications for new requests from guest */ > - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, false); > - virtio_set_queue_aio(s->vq, NULL); > + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, NULL); > > /* Drain and switch bs back to the QEMU main loop */ > blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context()); > diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c > index a497a2c..5494dcc 100644 > --- a/hw/scsi/virtio-scsi-dataplane.c > +++ b/hw/scsi/virtio-scsi-dataplane.c > @@ -81,8 +81,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n, > return rc; > } > > - virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true); > - virtio_set_queue_aio(vq, fn); > + virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, fn); > return 0; > } > > @@ -99,13 +98,10 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s) > VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); > int i; > > - virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, true, false); > - virtio_set_queue_aio(vs->ctrl_vq, NULL); > - virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, true, false); > - virtio_set_queue_aio(vs->event_vq, NULL); > + virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL); > + virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL); > for (i = 0; i < vs->conf.num_queues; i++) { > - virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, true, false); > - virtio_set_queue_aio(vs->cmd_vqs[i], NULL); > + virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL); > } > } > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index eb04ac0..7fcfc24f 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1159,14 +1159,6 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, > 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) { > @@ -1809,19 +1801,16 @@ static void virtio_queue_host_notifier_aio_read(EventNotifier *n) > } > > void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, > - bool assign, bool set_handler) > + void (*handle_output)(VirtIODevice *, > + VirtQueue *)) > { > - if (assign && set_handler) { > + vq->handle_aio_output = handle_output; > + if (handle_output) { > aio_set_event_notifier(ctx, &vq->host_notifier, true, > 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_aio_read(&vq->host_notifier); > - } > } > > static void virtio_queue_host_notifier_read(EventNotifier *n) This means that caller is now responsible for invoking the handler after it sets handle_output = NULL. I think it's cleaner to invoke it internally. > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index fa3f93b..6a37065 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -142,9 +142,6 @@ 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); > @@ -254,7 +251,8 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq); > void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, > bool set_handler); > void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, > - bool assign, bool set_handler); > + void (*fn)(VirtIODevice *, > + VirtQueue *)); > void virtio_irq(VirtQueue *vq); > VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector); > VirtQueue *virtio_vector_next_queue(VirtQueue *vq); > -- > 1.8.3.1 >
On 03/04/2016 11:06, Michael S. Tsirkin wrote: > On Fri, Apr 01, 2016 at 03:19:53PM +0200, Paolo Bonzini wrote: >> Eliminating the reentrancy is actually a nice thing that we can do >> with the API that Michael proposed, so let's make it first class. >> This also hides the complex assign/set_handler conventions from >> callers of virtio_queue_aio_set_host_notifier_handler, which in >> fact was always called with assign=true. >> >> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/block/dataplane/virtio-blk.c | 7 +++---- >> hw/scsi/virtio-scsi-dataplane.c | 12 ++++-------- >> hw/virtio/virtio.c | 19 ++++--------------- >> include/hw/virtio/virtio.h | 6 ++---- >> 4 files changed, 13 insertions(+), 31 deletions(-) >> >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c >> index fd06726..74c6d37 100644 >> --- a/hw/block/dataplane/virtio-blk.c >> +++ b/hw/block/dataplane/virtio-blk.c >> @@ -237,8 +237,8 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) >> >> /* Get this show started by hooking up our callbacks */ >> aio_context_acquire(s->ctx); >> - virtio_set_queue_aio(s->vq, virtio_blk_data_plane_handle_output); >> - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true); >> + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, >> + virtio_blk_data_plane_handle_output); >> aio_context_release(s->ctx); >> return; >> >> @@ -273,8 +273,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) >> aio_context_acquire(s->ctx); >> >> /* Stop notifications for new requests from guest */ >> - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, false); >> - virtio_set_queue_aio(s->vq, NULL); >> + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, NULL); >> >> /* Drain and switch bs back to the QEMU main loop */ >> blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context()); >> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c >> index a497a2c..5494dcc 100644 >> --- a/hw/scsi/virtio-scsi-dataplane.c >> +++ b/hw/scsi/virtio-scsi-dataplane.c >> @@ -81,8 +81,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n, >> return rc; >> } >> >> - virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true); >> - virtio_set_queue_aio(vq, fn); >> + virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, fn); >> return 0; >> } >> >> @@ -99,13 +98,10 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s) >> VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); >> int i; >> >> - virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, true, false); >> - virtio_set_queue_aio(vs->ctrl_vq, NULL); >> - virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, true, false); >> - virtio_set_queue_aio(vs->event_vq, NULL); >> + virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL); >> + virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL); >> for (i = 0; i < vs->conf.num_queues; i++) { >> - virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, true, false); >> - virtio_set_queue_aio(vs->cmd_vqs[i], NULL); >> + virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL); >> } >> } >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index eb04ac0..7fcfc24f 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -1159,14 +1159,6 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, >> 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) { >> @@ -1809,19 +1801,16 @@ static void virtio_queue_host_notifier_aio_read(EventNotifier *n) >> } >> >> void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, >> - bool assign, bool set_handler) >> + void (*handle_output)(VirtIODevice *, >> + VirtQueue *)) >> { >> - if (assign && set_handler) { >> + vq->handle_aio_output = handle_output; >> + if (handle_output) { >> aio_set_event_notifier(ctx, &vq->host_notifier, true, >> 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_aio_read(&vq->host_notifier); >> - } >> } >> >> static void virtio_queue_host_notifier_read(EventNotifier *n) > > This means that caller is now responsible for invoking the > handler after it sets handle_output = NULL. > I think it's cleaner to invoke it internally. No, the caller is not responsible for that. Ultimately, it's the virtio core that will be responsible for setting up the handler again in the main I/O thread, and that's how it will be called if it matters. This will happen when the API is cleaned up further by Cornelia. Note that this patch doesn't change the semantics, it's patch 1 that changes assign=false to assign=true in the call to virtio_queue_aio_set_host_notifier_handler. Without that change, virtio_queue_host_notifier_aio_read can run the virtqueue handler in the dataplane thread concurrently with the same handler in the main I/O thread. I consider that a fix for a latent/unknown bug, since we are having so many headaches with reentrancy. Of course I'm okay with dropping 9/9 (that's why I put it last), and probably it was not 2.6 material even if it worked. Paolo
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index fd06726..74c6d37 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -237,8 +237,8 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) /* Get this show started by hooking up our callbacks */ aio_context_acquire(s->ctx); - virtio_set_queue_aio(s->vq, virtio_blk_data_plane_handle_output); - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true); + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, + virtio_blk_data_plane_handle_output); aio_context_release(s->ctx); return; @@ -273,8 +273,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) aio_context_acquire(s->ctx); /* Stop notifications for new requests from guest */ - virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, false); - virtio_set_queue_aio(s->vq, NULL); + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, NULL); /* Drain and switch bs back to the QEMU main loop */ blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context()); diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c index a497a2c..5494dcc 100644 --- a/hw/scsi/virtio-scsi-dataplane.c +++ b/hw/scsi/virtio-scsi-dataplane.c @@ -81,8 +81,7 @@ static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n, return rc; } - virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true); - virtio_set_queue_aio(vq, fn); + virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, fn); return 0; } @@ -99,13 +98,10 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s) VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s); int i; - virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, true, false); - virtio_set_queue_aio(vs->ctrl_vq, NULL); - virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, true, false); - virtio_set_queue_aio(vs->event_vq, NULL); + virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, NULL); + virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, NULL); for (i = 0; i < vs->conf.num_queues; i++) { - virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, true, false); - virtio_set_queue_aio(vs->cmd_vqs[i], NULL); + virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, NULL); } } diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index eb04ac0..7fcfc24f 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1159,14 +1159,6 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, 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) { @@ -1809,19 +1801,16 @@ static void virtio_queue_host_notifier_aio_read(EventNotifier *n) } void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, - bool assign, bool set_handler) + void (*handle_output)(VirtIODevice *, + VirtQueue *)) { - if (assign && set_handler) { + vq->handle_aio_output = handle_output; + if (handle_output) { aio_set_event_notifier(ctx, &vq->host_notifier, true, 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_aio_read(&vq->host_notifier); - } } static void virtio_queue_host_notifier_read(EventNotifier *n) diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index fa3f93b..6a37065 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -142,9 +142,6 @@ 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); @@ -254,7 +251,8 @@ EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq); void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, bool set_handler); void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx, - bool assign, bool set_handler); + void (*fn)(VirtIODevice *, + VirtQueue *)); void virtio_irq(VirtQueue *vq); VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector); VirtQueue *virtio_vector_next_queue(VirtQueue *vq);