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