Re: [RFC PATCH] softmmu: Fix async_run_on_cpu() use in tcg_commit_cpu()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 7/9/23 21:36, Philippe Mathieu-Daudé wrote:
> On 7/9/23 18:28, Richard Henderson wrote:
>> On 9/7/23 09:14, Philippe Mathieu-Daudé wrote:
>>> CPUState::halt_cond is an accelerator specific pointer, used
>>> in particular by TCG (which tcg_commit() is about).
>>> The pointer is set by the AccelOpsClass::create_vcpu_thread()
>>> handler.
>>> AccelOpsClass::create_vcpu_thread() is called by the generic
>>> qemu_init_vcpu(), which expect the accelerator handler to
>>> eventually call cpu_thread_signal_created() which is protected
>>> with a QemuCond. It is safer to check the vCPU is created with
>>> this field rather than the 'halt_cond' pointer set in
>>> create_vcpu_thread() before the vCPU thread is initialized.
>>>
>>> This avoids calling tcg_commit() until all CPUs are realized.
>>>
>>> Here we can see for a machine with N CPUs, tcg_commit()
>>> is called N times before the 'machine_creation_done' event:
>>>
>>>    (lldb) settings set -- target.run-args  "-M" "virt" "-smp" "512" 
>>> "-display" "none"
>>>    (lldb) breakpoint set --name qemu_machine_creation_done --one-shot 
>>> true
>>>    (lldb) breakpoint set --name tcg_commit_cpu --auto-continue true
>>>    (lldb) run
>>>    Process 84089 launched: 'qemu-system-aarch64' (arm64)
>>>    Process 84089 stopped
>>>    * thread #1, queue = 'com.apple.main-thread', stop reason = 
>>> one-shot breakpoint 2
>>>    (lldb) breakpoint list --brief
>>>    Current breakpoints:
>>>    2: name = 'tcg_commit_cpu', locations = 2, resolved = 2, hit count 
>>> = 512 Options: enabled auto-continue
>>>               ^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^
>>>
>>
>> Of course the function is called 512 times: you asked for 512 cpus, 
>> and each has its own address space which needs initializing.

My commit description is too confusing. I'll split in tiny simple
changes and try to better describe each.

> The AS are still initialized at the same time, but we defer the listener
> callback until the vCPU is ready (what was expected first IIUC).
> 
>> If you skip the call before cpu->created, when exactly are you going 
>> to do it?
> 
> With this patch tcg_commit_cpu() is only called by vCPU threads, in
> their processing loop. i.e: comparing backtraces, now the first hit
> is:
> (lldb) bt
> * thread #514, stop reason = breakpoint 4.2
>    * frame #0: 0x1005d9d48 
> qemu-system-aarch64`tcg_commit_cpu(cpu=0x173358000, data=...) at 
> physmem.c:2493:63
>      frame #1: 0x10000d684 
> qemu-system-aarch64`process_queued_cpu_work(cpu=0x173358000) at 
> cpus-common.c:360:13
>      frame #2: 0x100297390 qemu-system-aarch64`qemu_wait_io_event 
> [inlined] qemu_wait_io_event_common(cpu=<unavailable>) at cpus.c:412:5 
> [artificial]
>      frame #3: 0x100623b98 
> qemu-system-aarch64`mttcg_cpu_thread_fn(arg=0x173358000) at 
> tcg-accel-ops-mttcg.c:123:9
>      frame #4: 0x10079f15c 
> qemu-system-aarch64`qemu_thread_start(args=<unavailable>) at 
> qemu-thread-posix.c:541:9
>      frame #5: 0x18880ffa8 libsystem_pthread.dylib`_pthread_start + 148
>