Re: [Qemu-devel] [PATCH 8/9] virtio: merge virtio_queue_aio_set_host_notifier_handler with virtio_queue_set_aio

[Michael S. Tsirkin]

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 <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  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
>