Hi Fabiano, On 2026-09-18 16:00, Fabiano Rosas wrote:
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().
I don't understand why we have these few per-target cpu_remove_sync() calls. IMO it should only be called by core vcpu accel scheduler layer.
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();
