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