Re: [PATCH v2] async: the main AioContext is only "current" if under the BQL

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

09.06.2021 15:22, Paolo Bonzini wrote:
> If we want to wake up a coroutine from a worker thread, aio_co_wake()
> currently does not work.  In that scenario, aio_co_wake() calls
> aio_co_enter(), but there is no current AioContext and therefore
> qemu_get_current_aio_context() returns the main thread.  aio_co_wake()
> then attempts to call aio_context_acquire() instead of going through
> aio_co_schedule().
> 
> The default case of qemu_get_current_aio_context() was added to cover
> synchronous I/O started from the vCPU thread, but the main and vCPU
> threads are quite different.  The main thread is an I/O thread itself,
> only running a more complicated event loop; the vCPU thread instead
> is essentially a worker thread that occasionally calls
> qemu_mutex_lock_iothread().  It is only in those critical sections
> that it acts as if it were the home thread of the main AioContext.
> 
> Therefore, this patch detaches qemu_get_current_aio_context() from
> iothreads, which is a useless complication.  The AioContext pointer
> is stored directly in the thread-local variable, including for the
> main loop.  Worker threads (including vCPU threads) optionally behave
> as temporary home threads if they have taken the big QEMU lock,
> but if that is not the case they will always schedule coroutines
> on remote threads via aio_co_schedule().
> 
> With this change, qemu_mutex_iothread_locked() must be changed from

Did you miss "qemu_mutex_iothread_locked() stub function"?

> true to false.  The previous value of true was needed because the
> main thread did not have an AioContext in the thread-local variable,
> but now it does have one.
> 
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Weak as I'm not good in all these iothreads, neither I know does all old callers of qemu_get_current_aio_context() are ok with new behavior. Technically looks OK:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Also, it works well with my nbd upcoming series, thanks:

Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Ask just in case: aio context of iothread is set only once and never changes, yes?


> ---
>   include/block/aio.h   |  5 ++++-
>   iothread.c            |  9 +--------
>   stubs/iothread-lock.c |  2 +-
>   stubs/iothread.c      |  8 --------
>   stubs/meson.build     |  1 -
>   tests/unit/iothread.c |  9 +--------
>   util/async.c          | 20 ++++++++++++++++++++
>   util/main-loop.c      |  1 +
>   8 files changed, 28 insertions(+), 27 deletions(-)
>   delete mode 100644 stubs/iothread.c
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 5f342267d5..10fcae1515 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -691,10 +691,13 @@ void aio_co_enter(AioContext *ctx, struct Coroutine *co);
>    * Return the AioContext whose event loop runs in the current thread.
>    *
>    * If called from an IOThread this will be the IOThread's AioContext.  If
> - * called from another thread it will be the main loop AioContext.
> + * called from the main thread or with the "big QEMU lock" taken it
> + * will be the main loop AioContext.

worth add: In other cases return NULL ?


-- 
Best regards,
Vladimir