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 <pbonz...@redhat.com> > >> Signed-off-by: Emanuele Giuseppe Esposito <eespo...@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
