Re: [RFC PATCH] cpus: split qemu_init_vcpu and delay vCPU thread creation

["Alex Bennée" <alex.bennee@linaro.org>]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 5/29/24 08:22, Alex Bennée wrote:
>> This ensures we don't start the thread until cpu_common_realizefn has
>> finished. This ensures that plugins will always run
>> qemu_plugin_vcpu_init__async first before any other states. It doesn't
>> totally eliminate the race that plugin_cpu_update__locked has to work
>> around though. I found this while reviewing the ips plugin which makes
>> heavy use of the vcpu phase callbacks.
>> An alternative might be to move the explicit creation of vCPU
>> threads
>> to qdev_machine_creation_done()? It doesn't affect user-mode which
>> already has a thread to execute in and ensures the QOM object has
>> completed creation in cpu_create() before continuing.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> Cc: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/core/cpu.h      |  8 ++++++++
>>   accel/tcg/user-exec-stub.c |  5 +++++
>>   hw/core/cpu-common.c       |  7 ++++++-
>>   plugins/core.c             |  5 +++++
>>   system/cpus.c              | 15 ++++++++++-----
>>   5 files changed, 34 insertions(+), 6 deletions(-)
>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>> index bb398e8237..6920699585 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -1041,6 +1041,14 @@ void end_exclusive(void);
>>    */
>>   void qemu_init_vcpu(CPUState *cpu);
>>   +/**
>> + * qemu_start_vcpu:
>> + * @cpu: The vCPU to start.
>> + *
>> + * Create the vCPU thread and start it running.
>> + */
>> +void qemu_start_vcpu(CPUState *cpu);
>> +
>>   #define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */
>>   #define SSTEP_NOIRQ   0x2  /* Do not use IRQ while single stepping */
>>   #define SSTEP_NOTIMER 0x4  /* Do not Timers while single stepping */
>> diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
>> index 4fbe2dbdc8..162bb72bbe 100644
>> --- a/accel/tcg/user-exec-stub.c
>> +++ b/accel/tcg/user-exec-stub.c
>> @@ -18,6 +18,11 @@ void cpu_exec_reset_hold(CPUState *cpu)
>>   {
>>   }
>>   +void qemu_start_vcpu(CPUState *cpu)
>> +{
>> +    /* NOP for user-mode, we already have a thread */
>> +}
>> +
>>   /* User mode emulation does not support record/replay yet.  */
>>     bool replay_exception(void)
>> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>> index 0f0a247f56..68895ddd59 100644
>> --- a/hw/core/cpu-common.c
>> +++ b/hw/core/cpu-common.c
>> @@ -230,7 +230,12 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>>       }
>>   #endif
>>   -    /* NOTE: latest generic point where the cpu is fully realized
>> */
>> +    /*
>> +     * With everything set up we can finally start the vCPU thread.
>> +     * This is a NOP for linux-user.
>> +     * NOTE: latest generic point where the cpu is fully realized
>> +     */
>> +    qemu_start_vcpu(cpu);
>>   }
>>     static void cpu_common_unrealizefn(DeviceState *dev)
>> diff --git a/plugins/core.c b/plugins/core.c
>> index 0726bc7f25..1e5da7853b 100644
>> --- a/plugins/core.c
>> +++ b/plugins/core.c
>> @@ -65,6 +65,11 @@ static void plugin_cpu_update__locked(gpointer k, gpointer v, gpointer udata)
>>       CPUState *cpu = container_of(k, CPUState, cpu_index);
>>       run_on_cpu_data mask = RUN_ON_CPU_HOST_ULONG(*plugin.mask);
>>   +    /*
>> +     * There is a race condition between the starting of the vCPU
>> +     * thread at the end of cpu_common_realizefn and when realized is
>> +     * finally set.
>> +     */
>
> Could we simply have an active wait here?
> while (!DEVICE(cpu)->realized) {}
>
> We have a guarantee it will be realized shortly, and if it's too hard
> to have a proper synchronization mechanism (introduce a
> realize_cond?), then waiting for the proper state does not seem too
> bad.
>
> It's a bit strange for me to document an existing race condition,
> instead of finding a solution.

If only it were that simple ;-)

Having been digging into this today it looks like there is a careful set
of dependencies on when threads need to be created during CPU
realization. I did try pushing the thread creation out of realization
but that breaks things like KVM which can't initialise some things
until the thread is up.

I'm now trying to move the plugin queuing its async work to after things
are initialised and before threads are created. My only concern now is
if I need to avoid kicking threads before they are created.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro