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

[Philippe Mathieu-Daudé]

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