Re: [Qemu-devel] [PATCH 11/17] aio: make AioContexts GSources

[Anthony Liguori]

Paolo Bonzini <address@hidden> writes:

> This lets AioContexts be used (optionally) with a glib main loop.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  aio-posix.c |  4 ++++
>  aio-win32.c |  4 ++++
>  async.c     | 65 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  qemu-aio.h  | 23 ++++++++++++++++++++++
>  4 file modificati, 95 inserzioni(+). 1 rimozione(-)
>
> diff --git a/aio-posix.c b/aio-posix.c
> index c848a9f..e29ece9 100644
> --- a/aio-posix.c
> +++ b/aio-posix.c
> @@ -56,6 +56,8 @@ void aio_set_fd_handler(AioContext *ctx,
>      /* Are we deleting the fd handler? */
>      if (!io_read && !io_write) {
>          if (node) {
> +            g_source_remove_poll(&ctx->source, &node->pfd);
> +
>              /* If the lock is held, just mark the node as deleted */
>              if (ctx->walking_handlers) {
>                  node->deleted = 1;
> @@ -75,6 +77,8 @@ void aio_set_fd_handler(AioContext *ctx,
>              node = g_malloc0(sizeof(AioHandler));
>              node->pfd.fd = fd;
>              QLIST_INSERT_HEAD(&ctx->aio_handlers, node, node);
> +
> +            g_source_add_poll(&ctx->source, &node->pfd);
>          }
>          /* Update handler with latest information */
>          node->io_read = io_read;
> diff --git a/aio-win32.c b/aio-win32.c
> index c46dfb2..5057371 100644
> --- a/aio-win32.c
> +++ b/aio-win32.c
> @@ -45,6 +45,8 @@ void aio_set_event_notifier(AioContext *ctx,
>      /* Are we deleting the fd handler? */
>      if (!io_notify) {
>          if (node) {
> +            g_source_remove_poll(&ctx->source, &node->pfd);
> +

Why remove vs. setting events = 0?

add_poll/remove_poll also comes with an event loop notify which I don't
think is strictly necessary here.

>              /* If the lock is held, just mark the node as deleted */
>              if (ctx->walking_handlers) {
>                  node->deleted = 1;
> @@ -66,6 +68,8 @@ void aio_set_event_notifier(AioContext *ctx,
>              node->pfd.fd = (uintptr_t)event_notifier_get_handle(e);
>              node->pfd.events = G_IO_IN;
>              QLIST_INSERT_HEAD(&ctx->aio_handlers, node, node);
> +
> +            g_source_add_poll(&ctx->source, &node->pfd);
>          }
>          /* Update handler with latest information */
>          node->io_notify = io_notify;
> diff --git a/async.c b/async.c
> index 513bdd7..ed2bd3f 100644
> --- a/async.c
> +++ b/async.c
> @@ -136,10 +136,73 @@ void aio_bh_update_timeout(AioContext *ctx, uint32_t 
> *timeout)
>      }
>  }
>  
> +static gboolean
> +aio_ctx_prepare(GSource *source, gint    *timeout)
> +{
> +    AioContext *ctx = (AioContext *) source;
> +    uint32_t wait = -1;
> +    aio_bh_update_timeout(ctx, &wait);
> +
> +    if (wait != -1) {
> +        *timeout = MIN(*timeout, wait);
> +        return wait == 0;
> +    }
> +
> +    return FALSE;
> +}
> +
> +static gboolean
> +aio_ctx_check(GSource *source)
> +{
> +    AioContext *ctx = (AioContext *) source;
> +    QEMUBH *bh;
> +
> +    for (bh = ctx->first_bh; bh; bh = bh->next) {
> +        if (!bh->deleted && bh->scheduled) {
> +            return true;
> +     }
> +    }
> +    return aio_pending(ctx);
> +}

Think you've got some copy/paste leftover glib coding style.  Probably
should use TRUE/true consistently too.  I think using TRUE/FALSE for
gboolean and true/false for bool is reasonable.

> +
> +static gboolean
> +aio_ctx_dispatch(GSource     *source,
> +                 GSourceFunc  callback,
> +                 gpointer     user_data)
> +{
> +    AioContext *ctx = (AioContext *) source;
> +
> +    assert(callback == NULL);
> +    aio_poll(ctx, false);
> +    return TRUE;
> +}
> +
> +static GSourceFuncs aio_source_funcs = {
> +    aio_ctx_prepare,
> +    aio_ctx_check,
> +    aio_ctx_dispatch,
> +    NULL
> +};
> +
> +GSource *aio_get_g_source(AioContext *ctx)
> +{
> +    g_source_ref(&ctx->source);
> +    return &ctx->source;
> +}
>  
>  AioContext *aio_context_new(void)
>  {
> -    return g_new0(AioContext, 1);
> +    return (AioContext *) g_source_new(&aio_source_funcs, 
> sizeof(AioContext));
> +}
> +
> +void aio_context_ref(AioContext *ctx)
> +{
> +    g_source_ref(&ctx->source);
> +}
> +
> +void aio_context_unref(AioContext *ctx)
> +{
> +    g_source_unref(&ctx->source);
>  }
>  
>  void aio_flush(AioContext *ctx)
> diff --git a/qemu-aio.h b/qemu-aio.h
> index ac24896..aedf66c 100644
> --- a/qemu-aio.h
> +++ b/qemu-aio.h
> @@ -44,6 +44,8 @@ typedef void QEMUBHFunc(void *opaque);
>  typedef void IOHandler(void *opaque);
>  
>  typedef struct AioContext {
> +    GSource source;
> +
>      /* The list of registered AIO handlers */
>      QLIST_HEAD(, AioHandler) aio_handlers;
>  
> @@ -75,6 +77,22 @@ typedef int (AioFlushEventNotifierHandler)(EventNotifier 
> *e);
>  AioContext *aio_context_new(void);
>  
>  /**
> + * aio_context_ref:
> + * @ctx: The AioContext to operate on.
> + *
> + * Add a reference to an AioContext.
> + */
> +void aio_context_ref(AioContext *ctx);
> +
> +/**
> + * aio_context_unref:
> + * @ctx: The AioContext to operate on.
> + *
> + * Drop a reference to an AioContext.
> + */
> +void aio_context_unref(AioContext *ctx);
> +
> +/**
>   * aio_bh_new: Allocate a new bottom half structure.
>   *
>   * Bottom halves are lightweight callbacks whose invocation is guaranteed
> @@ -188,6 +206,11 @@ void aio_set_event_notifier(AioContext *ctx,
>                              EventNotifierHandler *io_read,
>                              AioFlushEventNotifierHandler *io_flush);
>  
> +/* Return a GSource that lets the main loop poll the file descriptors 
> attached
> + * to this AioContext.
> + */
> +GSource *aio_get_g_source(AioContext *ctx);
> +
>  /* Functions to operate on the main QEMU AioContext.  */
>  
>  void qemu_aio_flush(void);
> -- 
> 1.7.12

I kind of dislike the fact that we've got a single source for all bottom
halves but this is definitely a good starting point.

The GSource implementation looks right to me.

Regards,

Anthony Liguori