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

[Kevin Wolf <kwolf@redhat.com>]

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