Re: [PATCH] accel: Fix qtest deadlock during unplug

[Fabiano Rosas <farosas@suse.de>]

Fabiano Rosas <farosas@suse.de> 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 <farosas@suse.de>
> ---
> 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.