[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[RFC PATCH] softmmu: Fix async_run_on_cpu() use in tcg_commit_cpu()
From: |
Philippe Mathieu-Daudé |
Subject: |
[RFC PATCH] softmmu: Fix async_run_on_cpu() use in tcg_commit_cpu() |
Date: |
Thu, 7 Sep 2023 18:14:14 +0200 |
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
^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^
Having the following backtrace:
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 2.1
* frame #0: 0x1005d0fe0 qemu-system-aarch64`tcg_commit [inlined]
tcg_commit_cpu(cpu=0x108460000, data=(host_ptr = 0x600003b05c00, target_ptr =
105553178156032)) at physmem.c:2493:63
frame #1: 0x1005d0fe0
qemu-system-aarch64`tcg_commit(listener=<unavailable>) at physmem.c:2527:9
frame #2: 0x1005cd220 qemu-system-aarch64`memory_listener_register
[inlined] listener_add_address_space(listener=0x600003b05c18, as=<unavailable>)
at memory.c:3014:9
frame #3: 0x1005cd148
qemu-system-aarch64`memory_listener_register(listener=0x600003b05c18,
as=0x16fdfe720) at memory.c:3077:5
frame #4: 0x1005d0f40
qemu-system-aarch64`cpu_address_space_init(cpu=<unavailable>,
asidx=<unavailable>, prefix=<unavailable>, mr=<unavailable>) at physmem.c:773:9
[artificial]
frame #5: 0x100389a64
qemu-system-aarch64`arm_cpu_realizefn(dev=0x108460000, errp=0x16fdfe720) at
cpu.c:2244:5
frame #6: 0x10062af28
qemu-system-aarch64`device_set_realized(obj=<unavailable>, value=<unavailable>,
errp=0x16fdfe7d8) at qdev.c:510:13
frame #7: 0x100632518
qemu-system-aarch64`property_set_bool(obj=0x108460000, v=<unavailable>,
name=<unavailable>, opaque=0x600000013e50, errp=0x16fdfe7d8) at object.c:2285:5
frame #8: 0x100630808
qemu-system-aarch64`object_property_set(obj=0x108460000, name="realized",
v=0x600003e02100, errp=0x16fdfe7d8) at object.c:1420:5
frame #9: 0x1006345ac
qemu-system-aarch64`object_property_set_qobject(obj=<unavailable>,
name=<unavailable>, value=<unavailable>, errp=<unavailable>) at
qom-qobject.c:28:10
frame #10: 0x100630c80
qemu-system-aarch64`object_property_set_bool(obj=<unavailable>,
name=<unavailable>, value=<unavailable>, errp=<unavailable>) at object.c:1489:15
frame #11: 0x10062a188
qemu-system-aarch64`qdev_realize(dev=<unavailable>, bus=<unavailable>,
errp=<unavailable>) at qdev.c:292:12 [artificial]
frame #12: 0x100319c30
qemu-system-aarch64`machvirt_init(machine=0x103562480) at virt.c:2248:9
frame #13: 0x100090edc
qemu-system-aarch64`machine_run_board_init(machine=0x103562480,
mem_path=<unavailable>, errp=<unavailable>) at machine.c:1469:5
frame #14: 0x1002a2684 qemu-system-aarch64`qmp_x_exit_preconfig [inlined]
qemu_init_board at vl.c:2543:5
frame #15: 0x1002a2650
qemu-system-aarch64`qmp_x_exit_preconfig(errp=<unavailable>) at vl.c:2634:5
frame #16: 0x1002a5dd4 qemu-system-aarch64`qemu_init(argc=<unavailable>,
argv=<unavailable>) at vl.c:3642:9
frame #17: 0x100627d64 qemu-system-aarch64`main(argc=<unavailable>,
argv=<unavailable>) at main.c:47:5
When can then invert the if ladders for clarity:
in the unlikely case of the caller being executed on the vCPU
thread, directly dispatch, otherwise defer until quiescence.
Cc: qemu-stable@nongnu.org
Fixes: 0d58c66068 ("softmmu: Use async_run_on_cpu in tcg_commit")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1866
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: I tried my best to understand and explain, but this is
still black magic to me...
---
softmmu/physmem.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 18277ddd67..12ef9d7d27 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2505,22 +2505,27 @@ static void tcg_commit(MemoryListener *listener)
cpuas = container_of(listener, CPUAddressSpace, tcg_as_listener);
cpu = cpuas->cpu;
- /*
- * Defer changes to as->memory_dispatch until the cpu is quiescent.
- * Otherwise we race between (1) other cpu threads and (2) ongoing
- * i/o for the current cpu thread, with data cached by mmu_lookup().
- *
- * In addition, queueing the work function will kick the cpu back to
- * the main loop, which will end the RCU critical section and reclaim
- * the memory data structures.
- *
- * That said, the listener is also called during realize, before
- * all of the tcg machinery for run-on is initialized: thus halt_cond.
- */
- if (cpu->halt_cond) {
- async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
- } else {
+ if (!cpu->created) {
+ /*
+ * The listener is also called during realize, before
+ * all of the tcg machinery for run-on is initialized.
+ */
+ return;
+ }
+
+ if (unlikely(qemu_cpu_is_self(cpu))) {
tcg_commit_cpu(cpu, RUN_ON_CPU_HOST_PTR(cpuas));
+ } else {
+ /*
+ * Defer changes to as->memory_dispatch until the cpu is quiescent.
+ * Otherwise we race between (1) other cpu threads and (2) ongoing
+ * i/o for the current cpu thread, with data cached by mmu_lookup().
+ *
+ * In addition, queueing the work function will kick the cpu back to
+ * the main loop, which will end the RCU critical section and reclaim
+ * the memory data structures.
+ */
+ async_run_on_cpu(cpu, tcg_commit_cpu, RUN_ON_CPU_HOST_PTR(cpuas));
}
}
--
2.41.0
- [RFC PATCH] softmmu: Fix async_run_on_cpu() use in tcg_commit_cpu(),
Philippe Mathieu-Daudé <=