[PATCH 0/2] AioContext removal: LinuxAioState and ThreadPool

[PATCH 0/2] AioContext removal: LinuxAioState and ThreadPool

[Emanuele Giuseppe Esposito]

Just remove some AioContext lock in LinuxAioState and ThreadPool.
Not related to anything specific, so I decided to send it as
a separate patch.

These patches are taken from Paolo's old draft series.

Emanuele Giuseppe Esposito (1):
  thread-pool: use ThreadPool from the running thread

Paolo Bonzini (1):
  linux-aio: use LinuxAioState from the running thread

 block/file-posix.c    | 22 +++++++++++-----------
 block/file-win32.c    |  2 +-
 block/linux-aio.c     | 13 ++++++-------
 block/qcow2-threads.c |  2 +-
 include/block/aio.h   |  4 ----
 util/thread-pool.c    |  6 +-----
 6 files changed, 20 insertions(+), 29 deletions(-)

-- 
2.31.1

[PATCH 1/2] linux-aio: use LinuxAioState from the running thread

[Emanuele Giuseppe Esposito]

From: Paolo Bonzini <pbonzini@redhat.com>

Remove usage of aio_context_acquire by always submitting asynchronous
AIO to the current thread's LinuxAioState.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/file-posix.c  |  3 ++-
 block/linux-aio.c   | 13 ++++++-------
 include/block/aio.h |  4 ----
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 48cd096624..33f92f004a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2086,7 +2086,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
 #endif
 #ifdef CONFIG_LINUX_AIO
     } else if (s->use_linux_aio) {
-        LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
+        AioContext *ctx = qemu_get_current_aio_context();
+        LinuxAioState *aio = aio_get_linux_aio(ctx);
         assert(qiov->size == bytes);
         return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
                               s->aio_max_batch);
diff --git a/block/linux-aio.c b/block/linux-aio.c
index 4c423fcccf..1d3cc767d1 100644
--- a/block/linux-aio.c
+++ b/block/linux-aio.c
@@ -16,6 +16,9 @@
 #include "qemu/coroutine.h"
 #include "qapi/error.h"
 
+/* Only used for assertions.  */
+#include "qemu/coroutine_int.h"
+
 #include <libaio.h>
 
 /*
@@ -56,10 +59,8 @@ struct LinuxAioState {
     io_context_t ctx;
     EventNotifier e;
 
-    /* io queue for submit at batch.  Protected by AioContext lock. */
+    /* All data is only used in one I/O thread.  */
     LaioQueue io_q;
-
-    /* I/O completion processing.  Only runs in I/O thread.  */
     QEMUBH *completion_bh;
     int event_idx;
     int event_max;
@@ -102,9 +103,8 @@ static void qemu_laio_process_completion(struct qemu_laiocb *laiocb)
      * later.  Coroutines cannot be entered recursively so avoid doing
      * that!
      */
-    if (!qemu_coroutine_entered(laiocb->co)) {
-        aio_co_wake(laiocb->co);
-    }
+    assert(laiocb->co->ctx == laiocb->ctx->aio_context);
+    qemu_coroutine_enter_if_inactive(laiocb->co);
 }
 
 /**
@@ -238,7 +238,6 @@ static void qemu_laio_process_completions_and_submit(LinuxAioState *s)
     if (!s->io_q.plugged && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
         ioq_submit(s);
     }
-    aio_context_release(s->aio_context);
 }
 
 static void qemu_laio_completion_bh(void *opaque)
diff --git a/include/block/aio.h b/include/block/aio.h
index d128558f1d..8bb5eea4a9 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -200,10 +200,6 @@ struct AioContext {
     struct ThreadPool *thread_pool;
 
 #ifdef CONFIG_LINUX_AIO
-    /*
-     * State for native Linux AIO.  Uses aio_context_acquire/release for
-     * locking.
-     */
     struct LinuxAioState *linux_aio;
 #endif
 #ifdef CONFIG_LINUX_IO_URING
-- 
2.31.1

[PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Emanuele Giuseppe Esposito]

Remove usage of aio_context_acquire by always submitting work items
to the current thread's ThreadPool.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/file-posix.c    | 19 +++++++++----------
 block/file-win32.c    |  2 +-
 block/qcow2-threads.c |  2 +-
 util/thread-pool.c    |  6 +-----
 4 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 33f92f004a..15765453b3 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2053,11 +2053,10 @@ out:
     return result;
 }
 
-static int coroutine_fn raw_thread_pool_submit(BlockDriverState *bs,
-                                               ThreadPoolFunc func, void *arg)
+static int coroutine_fn raw_thread_pool_submit(ThreadPoolFunc func, void *arg)
 {
     /* @bs can be NULL, bdrv_get_aio_context() returns the main context then */
-    ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+    ThreadPool *pool = aio_get_thread_pool(qemu_get_current_aio_context());
     return thread_pool_submit_co(pool, func, arg);
 }
 
@@ -2107,7 +2106,7 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
     };
 
     assert(qiov->size == bytes);
-    return raw_thread_pool_submit(bs, handle_aiocb_rw, &acb);
+    return raw_thread_pool_submit(handle_aiocb_rw, &acb);
 }
 
 static int coroutine_fn raw_co_preadv(BlockDriverState *bs, int64_t offset,
@@ -2182,7 +2181,7 @@ static int raw_co_flush_to_disk(BlockDriverState *bs)
         return luring_co_submit(bs, aio, s->fd, 0, NULL, QEMU_AIO_FLUSH);
     }
 #endif
-    return raw_thread_pool_submit(bs, handle_aiocb_flush, &acb);
+    return raw_thread_pool_submit(handle_aiocb_flush, &acb);
 }
 
 static void raw_aio_attach_aio_context(BlockDriverState *bs,
@@ -2244,7 +2243,7 @@ raw_regular_truncate(BlockDriverState *bs, int fd, int64_t offset,
         },
     };
 
-    return raw_thread_pool_submit(bs, handle_aiocb_truncate, &acb);
+    return raw_thread_pool_submit(handle_aiocb_truncate, &acb);
 }
 
 static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
@@ -2994,7 +2993,7 @@ raw_do_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes,
         acb.aio_type |= QEMU_AIO_BLKDEV;
     }
 
-    ret = raw_thread_pool_submit(bs, handle_aiocb_discard, &acb);
+    ret = raw_thread_pool_submit(handle_aiocb_discard, &acb);
     raw_account_discard(s, bytes, ret);
     return ret;
 }
@@ -3069,7 +3068,7 @@ raw_do_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
         handler = handle_aiocb_write_zeroes;
     }
 
-    return raw_thread_pool_submit(bs, handler, &acb);
+    return raw_thread_pool_submit(handler, &acb);
 }
 
 static int coroutine_fn raw_co_pwrite_zeroes(
@@ -3280,7 +3279,7 @@ static int coroutine_fn raw_co_copy_range_to(BlockDriverState *bs,
         },
     };
 
-    return raw_thread_pool_submit(bs, handle_aiocb_copy_range, &acb);
+    return raw_thread_pool_submit(handle_aiocb_copy_range, &acb);
 }
 
 BlockDriver bdrv_file = {
@@ -3626,7 +3625,7 @@ hdev_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
         },
     };
 
-    return raw_thread_pool_submit(bs, handle_aiocb_ioctl, &acb);
+    return raw_thread_pool_submit(handle_aiocb_ioctl, &acb);
 }
 #endif /* linux */
 
diff --git a/block/file-win32.c b/block/file-win32.c
index ec9d64d0e4..3d7f59a592 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -167,7 +167,7 @@ static BlockAIOCB *paio_submit(BlockDriverState *bs, HANDLE hfile,
     acb->aio_offset = offset;
 
     trace_file_paio_submit(acb, opaque, offset, count, type);
-    pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+    pool = aio_get_thread_pool(qemu_get_current_aio_context());
     return thread_pool_submit_aio(pool, aio_worker, acb, cb, opaque);
 }
 
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 1914baf456..9e370acbb3 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -42,7 +42,7 @@ qcow2_co_process(BlockDriverState *bs, ThreadPoolFunc *func, void *arg)
 {
     int ret;
     BDRVQcow2State *s = bs->opaque;
-    ThreadPool *pool = aio_get_thread_pool(bdrv_get_aio_context(bs));
+    ThreadPool *pool = aio_get_thread_pool(qemu_get_current_aio_context());
 
     qemu_co_mutex_lock(&s->lock);
     while (s->nb_threads >= QCOW2_MAX_THREADS) {
diff --git a/util/thread-pool.c b/util/thread-pool.c
index 31113b5860..74ce35f7a6 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -48,7 +48,7 @@ struct ThreadPoolElement {
     /* Access to this list is protected by lock.  */
     QTAILQ_ENTRY(ThreadPoolElement) reqs;
 
-    /* Access to this list is protected by the global mutex.  */
+    /* This list is only written by the thread pool's mother thread.  */
     QLIST_ENTRY(ThreadPoolElement) all;
 };
 
@@ -175,7 +175,6 @@ static void thread_pool_completion_bh(void *opaque)
     ThreadPool *pool = opaque;
     ThreadPoolElement *elem, *next;
 
-    aio_context_acquire(pool->ctx);
 restart:
     QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
         if (elem->state != THREAD_DONE) {
@@ -195,9 +194,7 @@ restart:
              */
             qemu_bh_schedule(pool->completion_bh);
 
-            aio_context_release(pool->ctx);
             elem->common.cb(elem->common.opaque, elem->ret);
-            aio_context_acquire(pool->ctx);
 
             /* We can safely cancel the completion_bh here regardless of someone
              * else having scheduled it meanwhile because we reenter the
@@ -211,7 +208,6 @@ restart:
             qemu_aio_unref(elem);
         }
     }
-    aio_context_release(pool->ctx);
 }
 
 static void thread_pool_cancel(BlockAIOCB *acb)
-- 
2.31.1

Re: [PATCH 1/2] linux-aio: use LinuxAioState from the running thread

[Kevin Wolf]

Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Remove usage of aio_context_acquire by always submitting asynchronous
> AIO to the current thread's LinuxAioState.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block/file-posix.c  |  3 ++-
>  block/linux-aio.c   | 13 ++++++-------
>  include/block/aio.h |  4 ----
>  3 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 48cd096624..33f92f004a 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -2086,7 +2086,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
>  #endif
>  #ifdef CONFIG_LINUX_AIO
>      } else if (s->use_linux_aio) {
> -        LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
> +        AioContext *ctx = qemu_get_current_aio_context();
> +        LinuxAioState *aio = aio_get_linux_aio(ctx);
>          assert(qiov->size == bytes);
>          return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
>                                s->aio_max_batch);

raw_aio_plug() and raw_aio_unplug() need the same change.

I wonder if we should actually better remove the 'aio' parameter from
the functions that linux-aio.c offers to avoid suggesting that any
LinuxAioState works for any thread. Getting it from the current
AioContext is something it can do by itself. But this would be code
cleanup for a separate patch.

> diff --git a/block/linux-aio.c b/block/linux-aio.c
> index 4c423fcccf..1d3cc767d1 100644
> --- a/block/linux-aio.c
> +++ b/block/linux-aio.c
> @@ -16,6 +16,9 @@
>  #include "qemu/coroutine.h"
>  #include "qapi/error.h"
>  
> +/* Only used for assertions.  */
> +#include "qemu/coroutine_int.h"
> +
>  #include <libaio.h>
>  
>  /*
> @@ -56,10 +59,8 @@ struct LinuxAioState {
>      io_context_t ctx;
>      EventNotifier e;
>  
> -    /* io queue for submit at batch.  Protected by AioContext lock. */
> +    /* All data is only used in one I/O thread.  */
>      LaioQueue io_q;
> -
> -    /* I/O completion processing.  Only runs in I/O thread.  */
>      QEMUBH *completion_bh;
>      int event_idx;
>      int event_max;
> @@ -102,9 +103,8 @@ static void qemu_laio_process_completion(struct qemu_laiocb *laiocb)
>       * later.  Coroutines cannot be entered recursively so avoid doing
>       * that!
>       */
> -    if (!qemu_coroutine_entered(laiocb->co)) {
> -        aio_co_wake(laiocb->co);
> -    }
> +    assert(laiocb->co->ctx == laiocb->ctx->aio_context);
> +    qemu_coroutine_enter_if_inactive(laiocb->co);
>  }
>  
>  /**
> @@ -238,7 +238,6 @@ static void qemu_laio_process_completions_and_submit(LinuxAioState *s)
>      if (!s->io_q.plugged && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
>          ioq_submit(s);
>      }
> -    aio_context_release(s->aio_context);
>  }

I certainly expected the aio_context_acquire() in the same function to
go away, too! Am I missing something?

Kevin

Re: [PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Kevin Wolf]

Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
> Remove usage of aio_context_acquire by always submitting work items
> to the current thread's ThreadPool.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

The thread pool is used by things outside of the file-* block drivers,
too. Even outside the block layer. Not all of these seem to submit work
in the same thread.


For example:

postcopy_ram_listen_thread() -> qemu_loadvm_state_main() ->
qemu_loadvm_section_start_full() -> vmstate_load() ->
vmstate_load_state() -> spapr_nvdimm_flush_post_load(), which has:

ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
...
thread_pool_submit_aio(pool, flush_worker_cb, state,
                       spapr_nvdimm_flush_completion_cb, state);

So it seems to me that we may be submitting work for the main thread
from a postcopy migration thread.

I believe the other direct callers of thread_pool_submit_aio() all
submit work for the main thread and also run in the main thread.


For thread_pool_submit_co(), pr_manager_execute() calls it with the pool
it gets passed as a parameter. This is still bdrv_get_aio_context(bs) in
hdev_co_ioctl() and should probably be changed the same way as for the
AIO call in file-posix, i.e. use qemu_get_current_aio_context().


We could consider either asserting in thread_pool_submit_aio() that we
are really in the expected thread, or like I suggested for LinuxAio drop
the pool parameter and always get it from the current thread (obviously
this is only possible if migration could in fact schedule the work on
its current thread - if it schedules it on the main thread and then
exits the migration thread (which destroys the thread pool), that
wouldn't be good).

Kevin

Re: [PATCH 1/2] linux-aio: use LinuxAioState from the running thread

[Emanuele Giuseppe Esposito]

Am 29/09/2022 um 16:52 schrieb Kevin Wolf:
> Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
>> From: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Remove usage of aio_context_acquire by always submitting asynchronous
>> AIO to the current thread's LinuxAioState.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>  block/file-posix.c  |  3 ++-
>>  block/linux-aio.c   | 13 ++++++-------
>>  include/block/aio.h |  4 ----
>>  3 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 48cd096624..33f92f004a 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -2086,7 +2086,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
>>  #endif
>>  #ifdef CONFIG_LINUX_AIO
>>      } else if (s->use_linux_aio) {
>> -        LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
>> +        AioContext *ctx = qemu_get_current_aio_context();
>> +        LinuxAioState *aio = aio_get_linux_aio(ctx);
>>          assert(qiov->size == bytes);
>>          return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
>>                                s->aio_max_batch);
> 
> raw_aio_plug() and raw_aio_unplug() need the same change.
> 
> I wonder if we should actually better remove the 'aio' parameter from
> the functions that linux-aio.c offers to avoid suggesting that any
> LinuxAioState works for any thread. Getting it from the current
> AioContext is something it can do by itself. But this would be code
> cleanup for a separate patch.

I do not think that this would work. At least not for all functions of
the API. I tried removing the ctx parameter from aio_setup_linux_aio and
it's already problematic, as it used by raw_aio_attach_aio_context()
which is a .bdrv_attach_aio_context() callback, which should be called
by the main thread. So that function needs the aiocontext parameter.

So maybe for now just simplify aio_get_linux_aio()? In a separate patch.
> 
>> diff --git a/block/linux-aio.c b/block/linux-aio.c
>> index 4c423fcccf..1d3cc767d1 100644
>> --- a/block/linux-aio.c
>> +++ b/block/linux-aio.c
>> @@ -16,6 +16,9 @@
>>  #include "qemu/coroutine.h"
>>  #include "qapi/error.h"
>>  
>> +/* Only used for assertions.  */
>> +#include "qemu/coroutine_int.h"
>> +
>>  #include <libaio.h>
>>  
>>  /*
>> @@ -56,10 +59,8 @@ struct LinuxAioState {
>>      io_context_t ctx;
>>      EventNotifier e;
>>  
>> -    /* io queue for submit at batch.  Protected by AioContext lock. */
>> +    /* All data is only used in one I/O thread.  */
>>      LaioQueue io_q;
>> -
>> -    /* I/O completion processing.  Only runs in I/O thread.  */
>>      QEMUBH *completion_bh;
>>      int event_idx;
>>      int event_max;
>> @@ -102,9 +103,8 @@ static void qemu_laio_process_completion(struct qemu_laiocb *laiocb)
>>       * later.  Coroutines cannot be entered recursively so avoid doing
>>       * that!
>>       */
>> -    if (!qemu_coroutine_entered(laiocb->co)) {
>> -        aio_co_wake(laiocb->co);
>> -    }
>> +    assert(laiocb->co->ctx == laiocb->ctx->aio_context);
>> +    qemu_coroutine_enter_if_inactive(laiocb->co);
>>  }
>>  
>>  /**
>> @@ -238,7 +238,6 @@ static void qemu_laio_process_completions_and_submit(LinuxAioState *s)
>>      if (!s->io_q.plugged && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
>>          ioq_submit(s);
>>      }
>> -    aio_context_release(s->aio_context);
>>  }
> 
> I certainly expected the aio_context_acquire() in the same function to
> go away, too! Am I missing something?

ops

Emanuele

Re: [PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Emanuele Giuseppe Esposito]

Am 29/09/2022 um 17:30 schrieb Kevin Wolf:
> Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
>> Remove usage of aio_context_acquire by always submitting work items
>> to the current thread's ThreadPool.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> The thread pool is used by things outside of the file-* block drivers,
> too. Even outside the block layer. Not all of these seem to submit work
> in the same thread.
> 
> 
> For example:
> 
> postcopy_ram_listen_thread() -> qemu_loadvm_state_main() ->
> qemu_loadvm_section_start_full() -> vmstate_load() ->
> vmstate_load_state() -> spapr_nvdimm_flush_post_load(), which has:
> 
> ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
> ...
> thread_pool_submit_aio(pool, flush_worker_cb, state,
>                        spapr_nvdimm_flush_completion_cb, state);
> 
> So it seems to me that we may be submitting work for the main thread
> from a postcopy migration thread.
> 
> I believe the other direct callers of thread_pool_submit_aio() all
> submit work for the main thread and also run in the main thread.
> 
> 
> For thread_pool_submit_co(), pr_manager_execute() calls it with the pool
> it gets passed as a parameter. This is still bdrv_get_aio_context(bs) in
> hdev_co_ioctl() and should probably be changed the same way as for the
> AIO call in file-posix, i.e. use qemu_get_current_aio_context().
> 
> 
> We could consider either asserting in thread_pool_submit_aio() that we
> are really in the expected thread, or like I suggested for LinuxAio drop
> the pool parameter and always get it from the current thread (obviously
> this is only possible if migration could in fact schedule the work on
> its current thread - if it schedules it on the main thread and then
> exits the migration thread (which destroys the thread pool), that
> wouldn't be good).

Dumb question: why not extend the already-existing poll->lock to cover
also the necessary fields like pool->head that are accessed by other
threads (only case I could find with thread_pool_submit_aio is the one
you pointed above)?

Thank you,
Emanuele

Re: [PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Emanuele Giuseppe Esposito]

Am 30/09/2022 um 14:17 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 29/09/2022 um 17:30 schrieb Kevin Wolf:
>> Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
>>> Remove usage of aio_context_acquire by always submitting work items
>>> to the current thread's ThreadPool.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>
>> The thread pool is used by things outside of the file-* block drivers,
>> too. Even outside the block layer. Not all of these seem to submit work
>> in the same thread.
>>
>>
>> For example:
>>
>> postcopy_ram_listen_thread() -> qemu_loadvm_state_main() ->
>> qemu_loadvm_section_start_full() -> vmstate_load() ->
>> vmstate_load_state() -> spapr_nvdimm_flush_post_load(), which has:
>>
>> ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
>> ...
>> thread_pool_submit_aio(pool, flush_worker_cb, state,
>>                        spapr_nvdimm_flush_completion_cb, state);
>>
>> So it seems to me that we may be submitting work for the main thread
>> from a postcopy migration thread.
>>
>> I believe the other direct callers of thread_pool_submit_aio() all
>> submit work for the main thread and also run in the main thread.
>>
>>
>> For thread_pool_submit_co(), pr_manager_execute() calls it with the pool
>> it gets passed as a parameter. This is still bdrv_get_aio_context(bs) in
>> hdev_co_ioctl() and should probably be changed the same way as for the
>> AIO call in file-posix, i.e. use qemu_get_current_aio_context().
>>
>>
>> We could consider either asserting in thread_pool_submit_aio() that we
>> are really in the expected thread, or like I suggested for LinuxAio drop
>> the pool parameter and always get it from the current thread (obviously
>> this is only possible if migration could in fact schedule the work on
>> its current thread - if it schedules it on the main thread and then
>> exits the migration thread (which destroys the thread pool), that
>> wouldn't be good).
> 
> Dumb question: why not extend the already-existing poll->lock to cover
> also the necessary fields like pool->head that are accessed by other
> threads (only case I could find with thread_pool_submit_aio is the one
> you pointed above)?
> 

That would be a good replacement for the aio_context lock in
thread_pool_completion_bh(), I think.

> Thank you,
> Emanuele
> 

Re: [PATCH 1/2] linux-aio: use LinuxAioState from the running thread

[Kevin Wolf]

Am 30.09.2022 um 12:00 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> Am 29/09/2022 um 16:52 schrieb Kevin Wolf:
> > Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
> >> From: Paolo Bonzini <pbonzini@redhat.com>
> >>
> >> Remove usage of aio_context_acquire by always submitting asynchronous
> >> AIO to the current thread's LinuxAioState.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >> ---
> >>  block/file-posix.c  |  3 ++-
> >>  block/linux-aio.c   | 13 ++++++-------
> >>  include/block/aio.h |  4 ----
> >>  3 files changed, 8 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/block/file-posix.c b/block/file-posix.c
> >> index 48cd096624..33f92f004a 100644
> >> --- a/block/file-posix.c
> >> +++ b/block/file-posix.c
> >> @@ -2086,7 +2086,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
> >>  #endif
> >>  #ifdef CONFIG_LINUX_AIO
> >>      } else if (s->use_linux_aio) {
> >> -        LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
> >> +        AioContext *ctx = qemu_get_current_aio_context();
> >> +        LinuxAioState *aio = aio_get_linux_aio(ctx);
> >>          assert(qiov->size == bytes);
> >>          return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
> >>                                s->aio_max_batch);
> > 
> > raw_aio_plug() and raw_aio_unplug() need the same change.
> > 
> > I wonder if we should actually better remove the 'aio' parameter from
> > the functions that linux-aio.c offers to avoid suggesting that any
> > LinuxAioState works for any thread. Getting it from the current
> > AioContext is something it can do by itself. But this would be code
> > cleanup for a separate patch.
> 
> I do not think that this would work. At least not for all functions of
> the API. I tried removing the ctx parameter from aio_setup_linux_aio and
> it's already problematic, as it used by raw_aio_attach_aio_context()
> which is a .bdrv_attach_aio_context() callback, which should be called
> by the main thread. So that function needs the aiocontext parameter.
> 
> So maybe for now just simplify aio_get_linux_aio()? In a separate patch.

Oh, I don't mind the ctx parameter in these functions at all.

I was talking about the functions in linux-aio.c, specifically
laio_co_submit(), laio_io_plug() and laio_io_unplug(). They could call
aio_get_linux_aio() internally for the current thread instead of letting
the caller do that and giving the false impression that there is more
than one correct value for their LinuxAioState parameter.

But anyway, as I said, this would be a separate cleanup patch. For this
one, it's just important that at least file-posix.c does the right thing
for plug/unplug, too.

> >> diff --git a/block/linux-aio.c b/block/linux-aio.c
> >> index 4c423fcccf..1d3cc767d1 100644
> >> --- a/block/linux-aio.c
> >> +++ b/block/linux-aio.c
> >> @@ -16,6 +16,9 @@
> >>  #include "qemu/coroutine.h"
> >>  #include "qapi/error.h"
> >>  
> >> +/* Only used for assertions.  */
> >> +#include "qemu/coroutine_int.h"
> >> +
> >>  #include <libaio.h>
> >>  
> >>  /*
> >> @@ -56,10 +59,8 @@ struct LinuxAioState {
> >>      io_context_t ctx;
> >>      EventNotifier e;
> >>  
> >> -    /* io queue for submit at batch.  Protected by AioContext lock. */
> >> +    /* All data is only used in one I/O thread.  */
> >>      LaioQueue io_q;
> >> -
> >> -    /* I/O completion processing.  Only runs in I/O thread.  */
> >>      QEMUBH *completion_bh;
> >>      int event_idx;
> >>      int event_max;
> >> @@ -102,9 +103,8 @@ static void qemu_laio_process_completion(struct qemu_laiocb *laiocb)
> >>       * later.  Coroutines cannot be entered recursively so avoid doing
> >>       * that!
> >>       */
> >> -    if (!qemu_coroutine_entered(laiocb->co)) {
> >> -        aio_co_wake(laiocb->co);
> >> -    }
> >> +    assert(laiocb->co->ctx == laiocb->ctx->aio_context);
> >> +    qemu_coroutine_enter_if_inactive(laiocb->co);
> >>  }
> >>  
> >>  /**
> >> @@ -238,7 +238,6 @@ static void qemu_laio_process_completions_and_submit(LinuxAioState *s)
> >>      if (!s->io_q.plugged && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
> >>          ioq_submit(s);
> >>      }
> >> -    aio_context_release(s->aio_context);
> >>  }
> > 
> > I certainly expected the aio_context_acquire() in the same function to
> > go away, too! Am I missing something?
> 
> ops

:-)

If it's unintentional, I'm actually surprised that locking without
unlocking later didn't cause problems immediately.

Kevin

Re: [PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Kevin Wolf]

Am 30.09.2022 um 14:17 hat Emanuele Giuseppe Esposito geschrieben:
> Am 29/09/2022 um 17:30 schrieb Kevin Wolf:
> > Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
> >> Remove usage of aio_context_acquire by always submitting work items
> >> to the current thread's ThreadPool.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > 
> > The thread pool is used by things outside of the file-* block drivers,
> > too. Even outside the block layer. Not all of these seem to submit work
> > in the same thread.
> > 
> > 
> > For example:
> > 
> > postcopy_ram_listen_thread() -> qemu_loadvm_state_main() ->
> > qemu_loadvm_section_start_full() -> vmstate_load() ->
> > vmstate_load_state() -> spapr_nvdimm_flush_post_load(), which has:
> > 
> > ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
> > ...
> > thread_pool_submit_aio(pool, flush_worker_cb, state,
> >                        spapr_nvdimm_flush_completion_cb, state);
> > 
> > So it seems to me that we may be submitting work for the main thread
> > from a postcopy migration thread.
> > 
> > I believe the other direct callers of thread_pool_submit_aio() all
> > submit work for the main thread and also run in the main thread.
> > 
> > 
> > For thread_pool_submit_co(), pr_manager_execute() calls it with the pool
> > it gets passed as a parameter. This is still bdrv_get_aio_context(bs) in
> > hdev_co_ioctl() and should probably be changed the same way as for the
> > AIO call in file-posix, i.e. use qemu_get_current_aio_context().
> > 
> > 
> > We could consider either asserting in thread_pool_submit_aio() that we
> > are really in the expected thread, or like I suggested for LinuxAio drop
> > the pool parameter and always get it from the current thread (obviously
> > this is only possible if migration could in fact schedule the work on
> > its current thread - if it schedules it on the main thread and then
> > exits the migration thread (which destroys the thread pool), that
> > wouldn't be good).
> 
> Dumb question: why not extend the already-existing poll->lock to cover
> also the necessary fields like pool->head that are accessed by other
> threads (only case I could find with thread_pool_submit_aio is the one
> you pointed above)?

Other people are more familiar with this code, but I believe this could
have performance implications. I seem to remember that this code is
careful to avoid locking to synchronise between worker threads and the
main thread.

But looking at the patch again, I have actually a dumb question, too:
The locking you're removing is in thread_pool_completion_bh(). As this
is a BH, it's running the the ThreadPool's context either way, no matter
which thread called thread_pool_submit_aio().

I'm not sure what this aio_context_acquire/release pair is actually
supposed to protect. Paolo's commit 1919631e6b5 introduced it. Was it
just more careful than it needs to be?

Kevin

Re: [PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Emanuele Giuseppe Esposito]

Am 30/09/2022 um 17:45 schrieb Kevin Wolf:
> Am 30.09.2022 um 14:17 hat Emanuele Giuseppe Esposito geschrieben:
>> Am 29/09/2022 um 17:30 schrieb Kevin Wolf:
>>> Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
>>>> Remove usage of aio_context_acquire by always submitting work items
>>>> to the current thread's ThreadPool.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>
>>> The thread pool is used by things outside of the file-* block drivers,
>>> too. Even outside the block layer. Not all of these seem to submit work
>>> in the same thread.
>>>
>>>
>>> For example:
>>>
>>> postcopy_ram_listen_thread() -> qemu_loadvm_state_main() ->
>>> qemu_loadvm_section_start_full() -> vmstate_load() ->
>>> vmstate_load_state() -> spapr_nvdimm_flush_post_load(), which has:
>>>
>>> ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
>>> ...
>>> thread_pool_submit_aio(pool, flush_worker_cb, state,
>>>                        spapr_nvdimm_flush_completion_cb, state);
>>>
>>> So it seems to me that we may be submitting work for the main thread
>>> from a postcopy migration thread.
>>>
>>> I believe the other direct callers of thread_pool_submit_aio() all
>>> submit work for the main thread and also run in the main thread.
>>>
>>>
>>> For thread_pool_submit_co(), pr_manager_execute() calls it with the pool
>>> it gets passed as a parameter. This is still bdrv_get_aio_context(bs) in
>>> hdev_co_ioctl() and should probably be changed the same way as for the
>>> AIO call in file-posix, i.e. use qemu_get_current_aio_context().
>>>
>>>
>>> We could consider either asserting in thread_pool_submit_aio() that we
>>> are really in the expected thread, or like I suggested for LinuxAio drop
>>> the pool parameter and always get it from the current thread (obviously
>>> this is only possible if migration could in fact schedule the work on
>>> its current thread - if it schedules it on the main thread and then
>>> exits the migration thread (which destroys the thread pool), that
>>> wouldn't be good).
>>
>> Dumb question: why not extend the already-existing poll->lock to cover
>> also the necessary fields like pool->head that are accessed by other
>> threads (only case I could find with thread_pool_submit_aio is the one
>> you pointed above)?
> 
> Other people are more familiar with this code, but I believe this could
> have performance implications. I seem to remember that this code is
> careful to avoid locking to synchronise between worker threads and the
> main thread.
> 
> But looking at the patch again, I have actually a dumb question, too:
> The locking you're removing is in thread_pool_completion_bh(). As this
> is a BH, it's running the the ThreadPool's context either way, no matter
> which thread called thread_pool_submit_aio().
> 
> I'm not sure what this aio_context_acquire/release pair is actually
> supposed to protect. Paolo's commit 1919631e6b5 introduced it. Was it
> just more careful than it needs to be?
> 

I think the goal is still to protect pool->head, but if so the
aiocontext lock is put in the wrong place, because as you said the bh is
always run in the thread pool context. Otherwise it seems to make no sense.

On the other side, thread_pool_submit_aio could be called by other
threads on behalf of the main loop, which means pool->head could be
modified (iothread calls thread_pool_submit_aio) while being read by the
main loop (another worker thread schedules thread_pool_completion_bh).

What are the performance implications? I mean, if the aiocontext lock in
the bh is actually useful and the bh really has to wait to take it,
being taken in much more places throughout the block layer won't be
better than extending the poll->lock I guess.

Thank you,
Emanuele

Re: [PATCH 1/2] linux-aio: use LinuxAioState from the running thread

[Emanuele Giuseppe Esposito]

Am 30/09/2022 um 17:32 schrieb Kevin Wolf:
> Am 30.09.2022 um 12:00 hat Emanuele Giuseppe Esposito geschrieben:
>>
>>
>> Am 29/09/2022 um 16:52 schrieb Kevin Wolf:
>>> Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
>>>> From: Paolo Bonzini <pbonzini@redhat.com>
>>>>
>>>> Remove usage of aio_context_acquire by always submitting asynchronous
>>>> AIO to the current thread's LinuxAioState.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> ---
>>>>  block/file-posix.c  |  3 ++-
>>>>  block/linux-aio.c   | 13 ++++++-------
>>>>  include/block/aio.h |  4 ----
>>>>  3 files changed, 8 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/block/file-posix.c b/block/file-posix.c
>>>> index 48cd096624..33f92f004a 100644
>>>> --- a/block/file-posix.c
>>>> +++ b/block/file-posix.c
>>>> @@ -2086,7 +2086,8 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
>>>>  #endif
>>>>  #ifdef CONFIG_LINUX_AIO
>>>>      } else if (s->use_linux_aio) {
>>>> -        LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
>>>> +        AioContext *ctx = qemu_get_current_aio_context();
>>>> +        LinuxAioState *aio = aio_get_linux_aio(ctx);
>>>>          assert(qiov->size == bytes);
>>>>          return laio_co_submit(bs, aio, s->fd, offset, qiov, type,
>>>>                                s->aio_max_batch);
>>>
>>> raw_aio_plug() and raw_aio_unplug() need the same change.
>>>
>>> I wonder if we should actually better remove the 'aio' parameter from
>>> the functions that linux-aio.c offers to avoid suggesting that any
>>> LinuxAioState works for any thread. Getting it from the current
>>> AioContext is something it can do by itself. But this would be code
>>> cleanup for a separate patch.
>>
>> I do not think that this would work. At least not for all functions of
>> the API. I tried removing the ctx parameter from aio_setup_linux_aio and
>> it's already problematic, as it used by raw_aio_attach_aio_context()
>> which is a .bdrv_attach_aio_context() callback, which should be called
>> by the main thread. So that function needs the aiocontext parameter.
>>
>> So maybe for now just simplify aio_get_linux_aio()? In a separate patch.
> 
> Oh, I don't mind the ctx parameter in these functions at all.
> 
> I was talking about the functions in linux-aio.c, specifically
> laio_co_submit(), laio_io_plug() and laio_io_unplug(). They could call
> aio_get_linux_aio() internally for the current thread instead of letting
> the caller do that and giving the false impression that there is more
> than one correct value for their LinuxAioState parameter.
> 
> But anyway, as I said, this would be a separate cleanup patch. For this
> one, it's just important that at least file-posix.c does the right thing
> for plug/unplug, too.
> 

Make sense

>>>> diff --git a/block/linux-aio.c b/block/linux-aio.c
>>>> index 4c423fcccf..1d3cc767d1 100644
>>>> --- a/block/linux-aio.c
>>>> +++ b/block/linux-aio.c
>>>> @@ -16,6 +16,9 @@
>>>>  #include "qemu/coroutine.h"
>>>>  #include "qapi/error.h"
>>>>  
>>>> +/* Only used for assertions.  */
>>>> +#include "qemu/coroutine_int.h"
>>>> +
>>>>  #include <libaio.h>
>>>>  
>>>>  /*
>>>> @@ -56,10 +59,8 @@ struct LinuxAioState {
>>>>      io_context_t ctx;
>>>>      EventNotifier e;
>>>>  
>>>> -    /* io queue for submit at batch.  Protected by AioContext lock. */
>>>> +    /* All data is only used in one I/O thread.  */
>>>>      LaioQueue io_q;
>>>> -
>>>> -    /* I/O completion processing.  Only runs in I/O thread.  */
>>>>      QEMUBH *completion_bh;
>>>>      int event_idx;
>>>>      int event_max;
>>>> @@ -102,9 +103,8 @@ static void qemu_laio_process_completion(struct qemu_laiocb *laiocb)
>>>>       * later.  Coroutines cannot be entered recursively so avoid doing
>>>>       * that!
>>>>       */
>>>> -    if (!qemu_coroutine_entered(laiocb->co)) {
>>>> -        aio_co_wake(laiocb->co);
>>>> -    }
>>>> +    assert(laiocb->co->ctx == laiocb->ctx->aio_context);
>>>> +    qemu_coroutine_enter_if_inactive(laiocb->co);
>>>>  }
>>>>  
>>>>  /**
>>>> @@ -238,7 +238,6 @@ static void qemu_laio_process_completions_and_submit(LinuxAioState *s)
>>>>      if (!s->io_q.plugged && !QSIMPLEQ_EMPTY(&s->io_q.pending)) {
>>>>          ioq_submit(s);
>>>>      }
>>>> -    aio_context_release(s->aio_context);
>>>>  }
>>>
>>> I certainly expected the aio_context_acquire() in the same function to
>>> go away, too! Am I missing something?
>>
>> ops
> 
> :-)
> 
> If it's unintentional, I'm actually surprised that locking without
> unlocking later didn't cause problems immediately.

Seems that iotests/unit tests do not trigger it.

> 
> Kevin
> 

Re: [PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5291 bytes --]

On Mon, Oct 03, 2022 at 10:52:33AM +0200, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 30/09/2022 um 17:45 schrieb Kevin Wolf:
> > Am 30.09.2022 um 14:17 hat Emanuele Giuseppe Esposito geschrieben:
> >> Am 29/09/2022 um 17:30 schrieb Kevin Wolf:
> >>> Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
> >>>> Remove usage of aio_context_acquire by always submitting work items
> >>>> to the current thread's ThreadPool.
> >>>>
> >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> >>>
> >>> The thread pool is used by things outside of the file-* block drivers,
> >>> too. Even outside the block layer. Not all of these seem to submit work
> >>> in the same thread.
> >>>
> >>>
> >>> For example:
> >>>
> >>> postcopy_ram_listen_thread() -> qemu_loadvm_state_main() ->
> >>> qemu_loadvm_section_start_full() -> vmstate_load() ->
> >>> vmstate_load_state() -> spapr_nvdimm_flush_post_load(), which has:
> >>>
> >>> ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
                         ^^^^^^^^^^^^^^^^^^^

aio_get_thread_pool() isn't thread safe either:

  ThreadPool *aio_get_thread_pool(AioContext *ctx)
  {
      if (!ctx->thread_pool) {
          ctx->thread_pool = thread_pool_new(ctx);
	  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Two threads could race in aio_get_thread_pool().

I think post-copy is broken here: it's calling code that was only
designed to be called from the main loop thread.

I have CCed Juan and David.

> >>> ...
> >>> thread_pool_submit_aio(pool, flush_worker_cb, state,
> >>>                        spapr_nvdimm_flush_completion_cb, state);
> >>>
> >>> So it seems to me that we may be submitting work for the main thread
> >>> from a postcopy migration thread.
> >>>
> >>> I believe the other direct callers of thread_pool_submit_aio() all
> >>> submit work for the main thread and also run in the main thread.
> >>>
> >>>
> >>> For thread_pool_submit_co(), pr_manager_execute() calls it with the pool
> >>> it gets passed as a parameter. This is still bdrv_get_aio_context(bs) in
> >>> hdev_co_ioctl() and should probably be changed the same way as for the
> >>> AIO call in file-posix, i.e. use qemu_get_current_aio_context().
> >>>
> >>>
> >>> We could consider either asserting in thread_pool_submit_aio() that we
> >>> are really in the expected thread, or like I suggested for LinuxAio drop
> >>> the pool parameter and always get it from the current thread (obviously
> >>> this is only possible if migration could in fact schedule the work on
> >>> its current thread - if it schedules it on the main thread and then
> >>> exits the migration thread (which destroys the thread pool), that
> >>> wouldn't be good).
> >>
> >> Dumb question: why not extend the already-existing poll->lock to cover
> >> also the necessary fields like pool->head that are accessed by other
> >> threads (only case I could find with thread_pool_submit_aio is the one
> >> you pointed above)?
> > 
> > Other people are more familiar with this code, but I believe this could
> > have performance implications. I seem to remember that this code is
> > careful to avoid locking to synchronise between worker threads and the
> > main thread.
> > 
> > But looking at the patch again, I have actually a dumb question, too:
> > The locking you're removing is in thread_pool_completion_bh(). As this
> > is a BH, it's running the the ThreadPool's context either way, no matter
> > which thread called thread_pool_submit_aio().
> > 
> > I'm not sure what this aio_context_acquire/release pair is actually
> > supposed to protect. Paolo's commit 1919631e6b5 introduced it. Was it
> > just more careful than it needs to be?
> > 
> 
> I think the goal is still to protect pool->head, but if so the
> aiocontext lock is put in the wrong place, because as you said the bh is
> always run in the thread pool context. Otherwise it seems to make no sense.
> 
> On the other side, thread_pool_submit_aio could be called by other
> threads on behalf of the main loop, which means pool->head could be
> modified (iothread calls thread_pool_submit_aio) while being read by the
> main loop (another worker thread schedules thread_pool_completion_bh).
> 
> What are the performance implications? I mean, if the aiocontext lock in
> the bh is actually useful and the bh really has to wait to take it,
> being taken in much more places throughout the block layer won't be
> better than extending the poll->lock I guess.

thread_pool_submit_aio() is missing documentation on how it is supposed
to be called.

Taking pool->lock is conservative and fine in the short-term.

In the longer term we need to clarify how thread_pool_submit_aio() is
supposed to be used and remove locking to protect pool->head if
possible.

A bunch of the event loop APIs are thread-safe (aio_set_fd_handler(),
qemu_schedule_bh(), etc) so it's somewhat natural to make
thread_pool_submit_aio() thread-safe too. However, it would be nice to
avoid synchronization and existing callers mostly call it from the same
event loop thread that runs the BH and we can avoid locking in that
case.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Dr. David Alan Gilbert]

* Stefan Hajnoczi (stefanha@redhat.com) wrote:
> On Mon, Oct 03, 2022 at 10:52:33AM +0200, Emanuele Giuseppe Esposito wrote:
> > 
> > 
> > Am 30/09/2022 um 17:45 schrieb Kevin Wolf:
> > > Am 30.09.2022 um 14:17 hat Emanuele Giuseppe Esposito geschrieben:
> > >> Am 29/09/2022 um 17:30 schrieb Kevin Wolf:
> > >>> Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
> > >>>> Remove usage of aio_context_acquire by always submitting work items
> > >>>> to the current thread's ThreadPool.
> > >>>>
> > >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > >>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > >>>
> > >>> The thread pool is used by things outside of the file-* block drivers,
> > >>> too. Even outside the block layer. Not all of these seem to submit work
> > >>> in the same thread.
> > >>>
> > >>>
> > >>> For example:
> > >>>
> > >>> postcopy_ram_listen_thread() -> qemu_loadvm_state_main() ->
> > >>> qemu_loadvm_section_start_full() -> vmstate_load() ->
> > >>> vmstate_load_state() -> spapr_nvdimm_flush_post_load(), which has:
> > >>>
> > >>> ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
>                          ^^^^^^^^^^^^^^^^^^^
> 
> aio_get_thread_pool() isn't thread safe either:
> 
>   ThreadPool *aio_get_thread_pool(AioContext *ctx)
>   {
>       if (!ctx->thread_pool) {
>           ctx->thread_pool = thread_pool_new(ctx);
> 	  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Two threads could race in aio_get_thread_pool().
> 
> I think post-copy is broken here: it's calling code that was only
> designed to be called from the main loop thread.
> 
> I have CCed Juan and David.

In theory the path that you describe there shouldn't happen - although
there is perhaps not enough protection on the load side to stop it
happening if presented with a bad stream.
This is documented in docs/devel/migration.rst under 'Destination
behaviour'; but to recap, during postcopy load we have a problem that we
need to be able to load incoming iterative (ie. RAM) pages during the
loading of normal devices, because the loading of a device may access
RAM that's not yet been transferred.

To do that, the device state of all the non-iterative devices (which I
think includes your spapr_nvdimm) is serialised into a separate
migration stream and sent as a 'package'.

We read the package off the stream on the main thread, but don't process
it until we fire off the 'listen' thread - which you spotted the
creation of above; the listen thread now takes over reading the
migration stream to process RAM pages, and since it's in the same
format, it calls qemu_loadvm_state_main() - but it doesn't expect
any devices in that other than the RAM devices; it's just expecting RAM.

In parallel with that, the main thread carries on loading the contents
of the 'package' - and that contains your spapr_nvdimm device (and any
other 'normal' devices); but that's OK because that's the main thread.

Now if something was very broken and sent a header for the spapr-nvdimm
down the main thread rather than into the package then, yes, we'd
trigger your case, but that shouldn't happen.

Dave

> > >>> ...
> > >>> thread_pool_submit_aio(pool, flush_worker_cb, state,
> > >>>                        spapr_nvdimm_flush_completion_cb, state);
> > >>>
> > >>> So it seems to me that we may be submitting work for the main thread
> > >>> from a postcopy migration thread.
> > >>>
> > >>> I believe the other direct callers of thread_pool_submit_aio() all
> > >>> submit work for the main thread and also run in the main thread.
> > >>>
> > >>>
> > >>> For thread_pool_submit_co(), pr_manager_execute() calls it with the pool
> > >>> it gets passed as a parameter. This is still bdrv_get_aio_context(bs) in
> > >>> hdev_co_ioctl() and should probably be changed the same way as for the
> > >>> AIO call in file-posix, i.e. use qemu_get_current_aio_context().
> > >>>
> > >>>
> > >>> We could consider either asserting in thread_pool_submit_aio() that we
> > >>> are really in the expected thread, or like I suggested for LinuxAio drop
> > >>> the pool parameter and always get it from the current thread (obviously
> > >>> this is only possible if migration could in fact schedule the work on
> > >>> its current thread - if it schedules it on the main thread and then
> > >>> exits the migration thread (which destroys the thread pool), that
> > >>> wouldn't be good).
> > >>
> > >> Dumb question: why not extend the already-existing poll->lock to cover
> > >> also the necessary fields like pool->head that are accessed by other
> > >> threads (only case I could find with thread_pool_submit_aio is the one
> > >> you pointed above)?
> > > 
> > > Other people are more familiar with this code, but I believe this could
> > > have performance implications. I seem to remember that this code is
> > > careful to avoid locking to synchronise between worker threads and the
> > > main thread.
> > > 
> > > But looking at the patch again, I have actually a dumb question, too:
> > > The locking you're removing is in thread_pool_completion_bh(). As this
> > > is a BH, it's running the the ThreadPool's context either way, no matter
> > > which thread called thread_pool_submit_aio().
> > > 
> > > I'm not sure what this aio_context_acquire/release pair is actually
> > > supposed to protect. Paolo's commit 1919631e6b5 introduced it. Was it
> > > just more careful than it needs to be?
> > > 
> > 
> > I think the goal is still to protect pool->head, but if so the
> > aiocontext lock is put in the wrong place, because as you said the bh is
> > always run in the thread pool context. Otherwise it seems to make no sense.
> > 
> > On the other side, thread_pool_submit_aio could be called by other
> > threads on behalf of the main loop, which means pool->head could be
> > modified (iothread calls thread_pool_submit_aio) while being read by the
> > main loop (another worker thread schedules thread_pool_completion_bh).
> > 
> > What are the performance implications? I mean, if the aiocontext lock in
> > the bh is actually useful and the bh really has to wait to take it,
> > being taken in much more places throughout the block layer won't be
> > better than extending the poll->lock I guess.
> 
> thread_pool_submit_aio() is missing documentation on how it is supposed
> to be called.
> 
> Taking pool->lock is conservative and fine in the short-term.
> 
> In the longer term we need to clarify how thread_pool_submit_aio() is
> supposed to be used and remove locking to protect pool->head if
> possible.
> 
> A bunch of the event loop APIs are thread-safe (aio_set_fd_handler(),
> qemu_schedule_bh(), etc) so it's somewhat natural to make
> thread_pool_submit_aio() thread-safe too. However, it would be nice to
> avoid synchronization and existing callers mostly call it from the same
> event loop thread that runs the BH and we can avoid locking in that
> case.
> 
> Stefan


-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/2] thread-pool: use ThreadPool from the running thread

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3770 bytes --]

On Thu, Oct 20, 2022 at 05:22:17PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Hajnoczi (stefanha@redhat.com) wrote:
> > On Mon, Oct 03, 2022 at 10:52:33AM +0200, Emanuele Giuseppe Esposito wrote:
> > > 
> > > 
> > > Am 30/09/2022 um 17:45 schrieb Kevin Wolf:
> > > > Am 30.09.2022 um 14:17 hat Emanuele Giuseppe Esposito geschrieben:
> > > >> Am 29/09/2022 um 17:30 schrieb Kevin Wolf:
> > > >>> Am 09.06.2022 um 15:44 hat Emanuele Giuseppe Esposito geschrieben:
> > > >>>> Remove usage of aio_context_acquire by always submitting work items
> > > >>>> to the current thread's ThreadPool.
> > > >>>>
> > > >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > >>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > > >>>
> > > >>> The thread pool is used by things outside of the file-* block drivers,
> > > >>> too. Even outside the block layer. Not all of these seem to submit work
> > > >>> in the same thread.
> > > >>>
> > > >>>
> > > >>> For example:
> > > >>>
> > > >>> postcopy_ram_listen_thread() -> qemu_loadvm_state_main() ->
> > > >>> qemu_loadvm_section_start_full() -> vmstate_load() ->
> > > >>> vmstate_load_state() -> spapr_nvdimm_flush_post_load(), which has:
> > > >>>
> > > >>> ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
> >                          ^^^^^^^^^^^^^^^^^^^
> > 
> > aio_get_thread_pool() isn't thread safe either:
> > 
> >   ThreadPool *aio_get_thread_pool(AioContext *ctx)
> >   {
> >       if (!ctx->thread_pool) {
> >           ctx->thread_pool = thread_pool_new(ctx);
> > 	  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Two threads could race in aio_get_thread_pool().
> > 
> > I think post-copy is broken here: it's calling code that was only
> > designed to be called from the main loop thread.
> > 
> > I have CCed Juan and David.
> 
> In theory the path that you describe there shouldn't happen - although
> there is perhaps not enough protection on the load side to stop it
> happening if presented with a bad stream.
> This is documented in docs/devel/migration.rst under 'Destination
> behaviour'; but to recap, during postcopy load we have a problem that we
> need to be able to load incoming iterative (ie. RAM) pages during the
> loading of normal devices, because the loading of a device may access
> RAM that's not yet been transferred.
> 
> To do that, the device state of all the non-iterative devices (which I
> think includes your spapr_nvdimm) is serialised into a separate
> migration stream and sent as a 'package'.
> 
> We read the package off the stream on the main thread, but don't process
> it until we fire off the 'listen' thread - which you spotted the
> creation of above; the listen thread now takes over reading the
> migration stream to process RAM pages, and since it's in the same
> format, it calls qemu_loadvm_state_main() - but it doesn't expect
> any devices in that other than the RAM devices; it's just expecting RAM.
> 
> In parallel with that, the main thread carries on loading the contents
> of the 'package' - and that contains your spapr_nvdimm device (and any
> other 'normal' devices); but that's OK because that's the main thread.
> 
> Now if something was very broken and sent a header for the spapr-nvdimm
> down the main thread rather than into the package then, yes, we'd
> trigger your case, but that shouldn't happen.

Thanks for explaining that. A way to restrict the listen thread to only
process RAM pages would be good both as documentation and to prevent
invalid migration streams for causing problems.

For Emanuele and Kevin's original question about this code, it seems the
thread pool won't be called from the listen thread.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]