Re: [Qemu-devel] [PATCH 01/18] iothread: release iothread around aio_poll

[Fam Zheng <famz@redhat.com>]

On Thu, 08/06 15:35, Paolo Bonzini wrote:
> This is the first step towards having fine-grained critical sections in
> dataplane threads, which resolves lock ordering problems between
> address_space_* functions (which need the BQL when doing MMIO, even
> after we complete RCU-based dispatch) and the AioContext.
> 
> Because AioContext does not use contention callbacks anymore, the
> unit test has to be changed.
> 
> Previously applied as a0710f7995f914e3044e5899bd8ff6c43c62f916 and
> then reverted.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  async.c                     | 22 +++-------------------
>  docs/multiple-iothreads.txt | 25 +++++++++++++++----------
>  include/block/aio.h         |  3 ---
>  iothread.c                  | 11 ++---------
>  tests/test-aio.c            | 19 +++++++++++--------
>  5 files changed, 31 insertions(+), 49 deletions(-)
> 
> diff --git a/async.c b/async.c
> index efce14b..f992914 100644
> --- a/async.c
> +++ b/async.c
> @@ -79,8 +79,8 @@ int aio_bh_poll(AioContext *ctx)
>           * aio_notify again if necessary.
>           */
>          if (!bh->deleted && atomic_xchg(&bh->scheduled, 0)) {
> -            /* Idle BHs and the notify BH don't count as progress */
> -            if (!bh->idle && bh != ctx->notify_dummy_bh) {
> +            /* Idle BHs don't count as progress */
> +            if (!bh->idle) {
>                  ret = 1;
>              }
>              bh->idle = 0;
> @@ -232,7 +232,6 @@ aio_ctx_finalize(GSource     *source)
>  {
>      AioContext *ctx = (AioContext *) source;
>  
> -    qemu_bh_delete(ctx->notify_dummy_bh);
>      thread_pool_free(ctx->thread_pool);
>  
>      qemu_mutex_lock(&ctx->bh_lock);
> @@ -299,19 +298,6 @@ static void aio_timerlist_notify(void *opaque)
>      aio_notify(opaque);
>  }
>  
> -static void aio_rfifolock_cb(void *opaque)
> -{
> -    AioContext *ctx = opaque;
> -
> -    /* Kick owner thread in case they are blocked in aio_poll() */
> -    qemu_bh_schedule(ctx->notify_dummy_bh);
> -}
> -
> -static void notify_dummy_bh(void *opaque)
> -{
> -    /* Do nothing, we were invoked just to force the event loop to iterate */
> -}
> -
>  static void event_notifier_dummy_cb(EventNotifier *e)
>  {
>  }
> @@ -333,11 +319,9 @@ AioContext *aio_context_new(Error **errp)
>                             event_notifier_dummy_cb);
>      ctx->thread_pool = NULL;
>      qemu_mutex_init(&ctx->bh_lock);
> -    rfifolock_init(&ctx->lock, aio_rfifolock_cb, ctx);
> +    rfifolock_init(&ctx->lock, NULL, NULL);
>      timerlistgroup_init(&ctx->tlg, aio_timerlist_notify, ctx);
>  
> -    ctx->notify_dummy_bh = aio_bh_new(ctx, notify_dummy_bh, NULL);
> -
>      return ctx;
>  }
>  
> diff --git a/docs/multiple-iothreads.txt b/docs/multiple-iothreads.txt
> index 40b8419..723cc7e 100644
> --- a/docs/multiple-iothreads.txt
> +++ b/docs/multiple-iothreads.txt
> @@ -105,13 +105,10 @@ a BH in the target AioContext beforehand and then call qemu_bh_schedule().  No
>  acquire/release or locking is needed for the qemu_bh_schedule() call.  But be
>  sure to acquire the AioContext for aio_bh_new() if necessary.
>  
> -The relationship between AioContext and the block layer
> --------------------------------------------------------
> -The AioContext originates from the QEMU block layer because it provides a
> -scoped way of running event loop iterations until all work is done.  This
> -feature is used to complete all in-flight block I/O requests (see
> -bdrv_drain_all()).  Nowadays AioContext is a generic event loop that can be
> -used by any QEMU subsystem.
> +AioContext and the block layer
> +------------------------------
> +The AioContext originates from the QEMU block layer, even though nowadays
> +AioContext is a generic event loop that can be used by any QEMU subsystem.
>  
>  The block layer has support for AioContext integrated.  Each BlockDriverState
>  is associated with an AioContext using bdrv_set_aio_context() and
> @@ -122,9 +119,17 @@ Block layer code must therefore expect to run in an IOThread and avoid using
>  old APIs that implicitly use the main loop.  See the "How to program for
>  IOThreads" above for information on how to do that.
>  
> -If main loop code such as a QMP function wishes to access a BlockDriverState it
> -must first call aio_context_acquire(bdrv_get_aio_context(bs)) to ensure the
> -IOThread does not run in parallel.
> +If main loop code such as a QMP function wishes to access a BlockDriverState
> +it must first call aio_context_acquire(bdrv_get_aio_context(bs)) to ensure
> +that callbacks in the IOThread do not run in parallel.
> +
> +Code running in the monitor typically uses bdrv_drain() to ensure that
> +past requests from the guest are completed.  When a block device is
> +running in an IOThread, the IOThread can also process requests from
> +the guest (via ioeventfd).  These requests have to be blocked with
> +aio_disable_clients() before calling bdrv_drain().  You can then reenable
> +guest requests with aio_enable_clients() before finally releasing the
> +AioContext and completing the monitor command.

This patch will probably go in before aio_disable_clients, if any, but I'm not
quite confident about the interface yet: listing a precise set of clients from
monitor is an ugly coupling between modules:

    aio_disable_clients(bdrv_get_aio_context(bs),
                        AIO_CLIENT_NBD | AIO_CLIENT_IOEVENTFD | AIO_CLIENT_FOO);

    ...

    aio_enble_clients(bdrv_get_aio_context(bs),
                      AIO_CLIENT_NBD | AIO_CLIENT_IOEVENTFD | AIO_CLIENT_FOO);

I prefer at least an abstraction:

    bdrv_quiesce_begin(bs);

    ...

    bdrv_quiesce_end(bs);

My point is maybe we should leave documenting this to whichever series that
introduces it?

Fam

>  
>  Long-running jobs (usually in the form of coroutines) are best scheduled in the
>  BlockDriverState's AioContext to avoid the need to acquire/release around each
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 400b1b0..9dd32e0 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -114,9 +114,6 @@ struct AioContext {
>      bool notified;
>      EventNotifier notifier;
>  
> -    /* Scheduling this BH forces the event loop it iterate */
> -    QEMUBH *notify_dummy_bh;
> -
>      /* Thread pool for performing work and receiving completion callbacks */
>      struct ThreadPool *thread_pool;
>  
> diff --git a/iothread.c b/iothread.c
> index da6ce7b..8f6d2e4 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -30,7 +30,6 @@ typedef ObjectClass IOThreadClass;
>  static void *iothread_run(void *opaque)
>  {
>      IOThread *iothread = opaque;
> -    bool blocking;
>  
>      rcu_register_thread();
>  
> @@ -39,14 +38,8 @@ static void *iothread_run(void *opaque)
>      qemu_cond_signal(&iothread->init_done_cond);
>      qemu_mutex_unlock(&iothread->init_done_lock);
>  
> -    while (!iothread->stopping) {
> -        aio_context_acquire(iothread->ctx);
> -        blocking = true;
> -        while (!iothread->stopping && aio_poll(iothread->ctx, blocking)) {
> -            /* Progress was made, keep going */
> -            blocking = false;
> -        }
> -        aio_context_release(iothread->ctx);
> +    while (!atomic_read(&iothread->stopping)) {
> +        aio_poll(iothread->ctx, true);
>      }
>  
>      rcu_unregister_thread();
> diff --git a/tests/test-aio.c b/tests/test-aio.c
> index 217e337..b4bd3f1 100644
> --- a/tests/test-aio.c
> +++ b/tests/test-aio.c
> @@ -99,6 +99,7 @@ static void event_ready_cb(EventNotifier *e)
>  
>  typedef struct {
>      QemuMutex start_lock;
> +    EventNotifier notifier;
>      bool thread_acquired;
>  } AcquireTestData;
>  
> @@ -110,6 +111,8 @@ static void *test_acquire_thread(void *opaque)
>      qemu_mutex_lock(&data->start_lock);
>      qemu_mutex_unlock(&data->start_lock);
>  
> +    g_usleep(500000);
> +    event_notifier_set(&data->notifier);
>      aio_context_acquire(ctx);
>      aio_context_release(ctx);
>  
> @@ -118,20 +121,19 @@ static void *test_acquire_thread(void *opaque)
>      return NULL;
>  }
>  
> -static void dummy_notifier_read(EventNotifier *unused)
> +static void dummy_notifier_read(EventNotifier *n)
>  {
> -    g_assert(false); /* should never be invoked */
> +    event_notifier_test_and_clear(n);
>  }
>  
>  static void test_acquire(void)
>  {
>      QemuThread thread;
> -    EventNotifier notifier;
>      AcquireTestData data;
>  
>      /* Dummy event notifier ensures aio_poll() will block */
> -    event_notifier_init(&notifier, false);
> -    aio_set_event_notifier(ctx, &notifier, dummy_notifier_read);
> +    event_notifier_init(&data.notifier, false);
> +    aio_set_event_notifier(ctx, &data.notifier, dummy_notifier_read);
>      g_assert(!aio_poll(ctx, false)); /* consume aio_notify() */
>  
>      qemu_mutex_init(&data.start_lock);
> @@ -145,12 +147,13 @@ static void test_acquire(void)
>      /* Block in aio_poll(), let other thread kick us and acquire context */
>      aio_context_acquire(ctx);
>      qemu_mutex_unlock(&data.start_lock); /* let the thread run */
> -    g_assert(!aio_poll(ctx, true));
> +    g_assert(aio_poll(ctx, true));
> +    g_assert(!data.thread_acquired);
>      aio_context_release(ctx);
>  
>      qemu_thread_join(&thread);
> -    aio_set_event_notifier(ctx, &notifier, NULL);
> -    event_notifier_cleanup(&notifier);
> +    aio_set_event_notifier(ctx, &data.notifier, NULL);
> +    event_notifier_cleanup(&data.notifier);
>  
>      g_assert(data.thread_acquired);
>  }
> -- 
> 2.4.3
> 
>