Re: [Qemu-devel] [PATCH 2/3] AioContext: acquire/release AioContext during aio_poll

[Fam Zheng <famz@redhat.com>]

On Fri, 02/20 17:26, Paolo Bonzini wrote:
> This is the first step in pushing down acquire/release, and will let
> rfifolock drop the contention callback feature.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  aio-posix.c         |  9 +++++++++
>  aio-win32.c         |  8 ++++++++
>  include/block/aio.h | 15 ++++++++-------
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/aio-posix.c b/aio-posix.c
> index 4a30b77..292ae84 100644
> --- a/aio-posix.c
> +++ b/aio-posix.c
> @@ -238,6 +238,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
>      bool progress;
>      int64_t timeout;
>  
> +    aio_context_acquire(ctx);
>      was_dispatching = ctx->dispatching;
>      progress = false;
>  
> @@ -267,7 +268,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
>      timeout = blocking ? aio_compute_timeout(ctx) : 0;
>  
>      /* wait until next event */
> +    if (timeout) {
> +        aio_context_release(ctx);
> +    }
>      ret = qemu_poll_ns((GPollFD *)pollfds, npfd, timeout);

If two threads poll concurrently on this ctx, they will get the same set of
events, is that safe? Doesn't that lead to double dispatch?

Fam

> +    if (timeout) {
> +        aio_context_acquire(ctx);
> +    }
>  
>      /* if we have any readable fds, dispatch event */
>      if (ret > 0) {
> @@ -285,5 +292,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
>      }
>  
>      aio_set_dispatching(ctx, was_dispatching);
> +    aio_context_release(ctx);
> +
>      return progress;
>  }
> diff --git a/aio-win32.c b/aio-win32.c
> index e6f4ced..233d8f5 100644
> --- a/aio-win32.c
> +++ b/aio-win32.c
> @@ -283,6 +283,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
>      int count;
>      int timeout;
>  
> +    aio_context_acquire(ctx);
>      have_select_revents = aio_prepare(ctx);
>      if (have_select_revents) {
>          blocking = false;
> @@ -323,7 +324,13 @@ bool aio_poll(AioContext *ctx, bool blocking)
>  
>          timeout = blocking
>              ? qemu_timeout_ns_to_ms(aio_compute_timeout(ctx)) : 0;
> +        if (timeout) {
> +            aio_context_release(ctx);
> +        }
>          ret = WaitForMultipleObjects(count, events, FALSE, timeout);
> +        if (timeout) {
> +            aio_context_acquire(ctx);
> +        }
>          aio_set_dispatching(ctx, true);
>  
>          if (first && aio_bh_poll(ctx)) {
> @@ -349,5 +356,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
>      progress |= timerlistgroup_run_timers(&ctx->tlg);
>  
>      aio_set_dispatching(ctx, was_dispatching);
> +    aio_context_release(ctx);
>      return progress;
>  }
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 499efd0..e77409d 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -118,13 +118,14 @@ void aio_context_ref(AioContext *ctx);
>  void aio_context_unref(AioContext *ctx);
>  
>  /* Take ownership of the AioContext.  If the AioContext will be shared between
> - * threads, a thread must have ownership when calling aio_poll().
> - *
> - * Note that multiple threads calling aio_poll() means timers, BHs, and
> - * callbacks may be invoked from a different thread than they were registered
> - * from.  Therefore, code must use AioContext acquire/release or use
> - * fine-grained synchronization to protect shared state if other threads will
> - * be accessing it simultaneously.
> + * threads, and a thread does not want to be interrupted, it will have to
> + * take ownership around calls to aio_poll().  Otherwise, aio_poll()
> + * automatically takes care of calling aio_context_acquire and
> + * aio_context_release.
> + *
> + * Access to timers and BHs from a thread that has not acquired AioContext
> + * is possible.  Access to callbacks for now must be done while the AioContext
> + * is owned by the thread (FIXME).
>   */
>  void aio_context_acquire(AioContext *ctx);
>  
> -- 
> 2.3.0
> 
>