Fabiano Rosas <[email protected]> writes: > This is a revert of one hunk of commit d5e33b5f8f ("accel: make all > calls to qemu_process_cpu_events look the same"). It regressed > device-plug-test on ppc64. Run this in a loop and it deadlocks before > 50 iterations: > > QTEST_QEMU_BINARY=./qemu-system-ppc64 ./tests/qtest/device-plug-test -p > /ppc64/device-plug/spapr-cpu-unplug-request > > The deadlocked stacks are: > T0: > #0 in sigtimedwait > #1 in sigwait > #2 in dummy_cpu_thread_fn (arg=0x558ed4db8eb0) at ../accel/dummy-cpus.c:52 > > T1: > #2 in qemu_thread_join (thread=0x55e3803dfc60) at > ../util/qemu-thread-posix.c:554 > #3 in cpu_remove_sync (cpu=0x558ed4db8eb0) at ../system/cpus.c:633 > #4 in ppc_cpu_unrealize (dev=0x558ed4db8eb0) at > ../target/ppc/cpu_init.c:6967 > > What the test does is to queue a cpu unplug request to be executed > during system reset. So we end up with two cpu_exit() calls affecting > the dummy loop, one via pause_all_cpus() and another via > cpu_remove_sync(). > > Moving qemu_process_cpu_events() to the top of the loop has made the > release of the halt_cond + the read of cpu->unplug not happen > atomically regarding the BQL anymore. > > One cpu_exit() call will cause qemu_process_cpu_events() to make > progress, the BQL be release and the pending SIG_IPI to be consumed by > sigwait(). But since the BQL is unlocked, the second qemu_cpu_kick() > invocation can execute entirely while the BQL is unlocked and issue: > > i) another broadcast on halt_cond, which will be queued and, > ii) another signal, which will be discarded > > After the sigwait() returns and qemu_process_cpu_events() executes > again in the next loop iteration, it exits right away due to the cond > already being posted, but the sigwait() call for that loop won't see > any signal. The thread cannot be joined at this point so there's a > deadlock. > > Since the dummy_cpu loop is so simple, I think the best way to fix > this is to revert that part of the change and move > qemu_process_cpu_events() back to the end of the loop, where it will > be within the same BQL locking window as the cpu->unplug check. > > Fixes: d5e33b5f8f ("accel: make all calls to qemu_process_cpu_events look the > same") > Signed-off-by: Fabiano Rosas <[email protected]> > --- > CI run: https://gitlab.com/farosas/qemu/-/pipelines/2861429046 > --- > accel/dummy-cpus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/accel/dummy-cpus.c b/accel/dummy-cpus.c > index 5752f6302c..225a47c31f 100644 > --- a/accel/dummy-cpus.c > +++ b/accel/dummy-cpus.c > @@ -43,7 +43,6 @@ static void *dummy_cpu_thread_fn(void *arg) > qemu_guest_random_seed_thread_part2(cpu->random_seed); > > do { > - qemu_process_cpu_events(cpu); > bql_unlock(); > #ifndef _WIN32 > do { > @@ -58,6 +57,7 @@ static void *dummy_cpu_thread_fn(void *arg) > qemu_sem_wait(&cpu->sem); > #endif > bql_lock(); > + qemu_process_cpu_events(cpu); > } while (!cpu->unplug); > > bql_unlock();
+ppc and mshv folks @Chinmay, just to make you aware that there's a broken test for ppc @Shivang, we're discussing about cpu_remove_sync() down in this thread, maybe that's of interest to you. Philippe is suggesting we could maybe move the call up a layer. @Doru, Magnus, for your awareness. This bug is about the dummy cpu thread that qtest and xen use, but the pattern of coming out of qemu_process_cpu_events() and unlocking the BQL is present in mshv as well.
