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
>