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

[Fabiano Rosas <farosas@suse.de>]

Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:

> 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.
>

TLDR: ppc introduced it at the machine level (spapr_cpu_core.c) to handle
cpu hot-unplug. It later got moved into cpu unrealize.

Let's look at the history:

1) cpu_remove_sync() was introduced to deal with hot-unplug failures.
2c579042e3 (cpu: Add a sync version of cpu_remove(), 2016-05-12)

2) The cpu->unplug logic is due to the possibility of requesting a cpu
unplug, leaving the cpu object in the list and then later plugging the
cpu again and re-using that object (I'm not sure, looks more like a
comment from KVM point of view).
4c055ab54f (cpu: Reclaim vCPU objects, 2016-05-12)

3) For x86, it seems cpu_remove_sync() is called just for regular
cleanup.
c884776e9d (target-i386: Add x86_cpu_unrealizefn(), 2016-06-24)

3) The _sync version got later turned into the default with the removal
of the async one.
dbadee4ff4 (cpus: join thread when removing a vCPU, 2018-01-30)

I'm confused about two aspects of this:

1) Checking cpu->unplug in the run loops in general because it seems to
make most of cpu_can_run() redundant since cpu->stop is also set during
cpu_remove_sync(). It's not clear to me what sort of concurrency can
keep the loop going once cpu->unplug=true.

To me it looks like we could very well move the cpu->unplug check into
cpu_can_run(), move qemu_process_cpu_events(cpu) back to the end of the
loop and merge both (where there are two) calls to cpu_can_run(). E.g.:

    do {
        bql_unlock();
        excp = tcg_cpu_exec(cpu);
        qemu_process_cpu_events(cpu);
        bql_lock();
    } while (cpu_can_run());

2) Not all targets set cpu->unplug, but the check happens for all. What
exactly stops their vcpus threads? And, obviously, is it safe to now use
cpu->unplug.