Philippe Mathieu-Daudé <[email protected]> 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.
