On Wed, Aug 26, 2026 at 3:46 AM Alex Bennée <[email protected]> wrote: > > Matt Turner <[email protected]> writes: > > > curr_cflags() is called once per TB dispatch, from helper_lookup_tb_ptr() > > and from the cpu_exec() loop. It recomputes the same value every time: > > > > uint32_t cflags = cpu->tcg_cflags; > > if (unlikely(cpu_single_stepping(cpu))) { ... } > > else if (qatomic_read(&one_insn_per_tb)) { ... } > > else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) { ... } > > > > That is three loads and three branches on the hottest path in the > > interpreter, for state that changes only when gdb enables single-step, > > when one-insn-per-tb is toggled, or when the log mask changes. > > > > None of the three has to be sampled at dispatch time. Fold each into > > CPUState::tcg_cflags where it changes and curr_cflags() becomes a single > > load of a field that TB lookup has to read anyway. > > > > The derived bits -- CF_COUNT_MASK, CF_NO_GOTO_TB, CF_NO_GOTO_PTR and > > CF_SINGLE_STEP -- are never set by tcg_cflags_set(), so tcg_update_cflags() > > can recompute them in place without disturbing the rest, and conversely > > tcg_cflags_set() ORs in its bits without disturbing them. > > > > There are three places to call it: > > > > - tcg_exec_realizefn(), so that a CPU created after the command line has > > been parsed starts out with the right value. This covers user-only, > > where tcg_cpu_init_cflags() is not reached. linux-user's cpu_copy() > > copies tcg_cflags wholesale, so a cloned thread inherits it. > > > > - cpu_single_step(), which changes one CPU and runs either on that CPU's > > thread or with it stopped. > > > > - tcg_set_one_insn_per_tb() and qemu_set_log_internal(), which change > > every CPU. Both can be reached from the monitor while the vCPUs are > > running -- 'one-insn-per-tb on' and 'log nochain' -- so the update is > > queued with async_safe_run_on_cpu() and each CPU writes its own cflags > > with the others halted. > > > > Measured with qemu-alpha running an emulated alpha gcc 16.2.0 compiling > > the SQLite 3.45.1 amalgamation (255k lines, -O2) on an x86-64 host, in a > > build configured with --enable-lto: > > > > before: 1,646,994,254,249 instructions > > after: 1,562,204,796,597 instructions -5.15% > > > > That workload issues 8.4 billion dispatches, so the per-call saving is > > small but the aggregate is not. The emulated compiler produces > > byte-identical output before and after. > > > > Wall clock does not move: 133.19s to 132.58s, a 0.46% difference against a > > run-to-run spread larger than that. The removed work is a few predictable > > loads and branches that the host executes largely in parallel with the > > surrounding dispatch, so this patch is worth taking for the instruction > > count and for what it enables, not for a time saving that can be measured > > on its own. > > > > Signed-off-by: Matt Turner <[email protected]> > > --- > > accel/tcg/cpu-exec-common.c | 33 ++++++++++++++++++++++++++++++--- > > accel/tcg/cpu-exec.c | 3 +++ > > accel/tcg/internal-common.h | 11 +++++++++-- > > accel/tcg/tcg-all.c | 1 + > > cpu-target.c | 3 +++ > > include/system/tcg.h | 12 ++++++++++++ > > stubs/meson.build | 1 + > > stubs/tcg-cflags.c | 16 ++++++++++++++++ > > util/log.c | 4 ++++ > > 9 files changed, 79 insertions(+), 5 deletions(-) > > create mode 100644 stubs/tcg-cflags.c > > > > diff --git ./accel/tcg/cpu-exec-common.c ./accel/tcg/cpu-exec-common.c > > index 44e84344f3..dd2be475e2 100644 > > --- ./accel/tcg/cpu-exec-common.c > > +++ ./accel/tcg/cpu-exec-common.c > > @@ -36,9 +36,16 @@ void tcg_cflags_set(CPUState *cpu, uint32_t flags) > > cpu->tcg_cflags |= flags; > > } > > > > -uint32_t curr_cflags(CPUState *cpu) > > +/* > > + * The bits of CPUState::tcg_cflags that tcg_cflags_set() never sets, > > because > > + * they are derived from gdb single-step, one-insn-per-tb and -d nochain. > > + */ > > +#define CF_DERIVED (CF_COUNT_MASK | CF_NO_GOTO_TB | CF_NO_GOTO_PTR | \ > > + CF_SINGLE_STEP) > > + > > +void tcg_update_cflags(CPUState *cpu) > > { > > - uint32_t cflags = cpu->tcg_cflags; > > + uint32_t cflags = cpu->tcg_cflags & ~CF_DERIVED; > > > > /* > > * Record gdb single-step. We should be exiting the TB by raising > > @@ -55,7 +62,27 @@ uint32_t curr_cflags(CPUState *cpu) > > cflags |= CF_NO_GOTO_TB; > > } > > > > - return cflags; > > + cpu->tcg_cflags = cflags; > > +} > > + > > +static void tcg_update_cflags_work(CPUState *cpu, run_on_cpu_data data) > > +{ > > + tcg_update_cflags(cpu); > > +} > > + > > +void tcg_update_all_cflags(void) > > +{ > > + CPUState *cpu; > > + > > + /* > > + * one-insn-per-tb and -d nochain can both be changed from the monitor > > + * while the vCPUs are running. Have each CPU update its own cflags > > + * with the others halted, so that no dispatch can read a value that > > + * another thread is in the middle of writing. > > + */ > > + CPU_FOREACH(cpu) { > > + async_safe_run_on_cpu(cpu, tcg_update_cflags_work, > > RUN_ON_CPU_NULL); > > I don't think this is wrong but are we really seeing cross-vCPU updates > of cpu->cflags?
Fixed in v4. The monitor path was the cross-vCPU one -- v3 had the monitor thread storing into every cpu->tcg_cflags -- and v4 queues the update instead, so each vCPU writes its own field from its own thread. The one foreign writer left is cpu_single_step(), and it's the same one that was already there. gdb_continue_partial() in gdbstub/user.c walks CPU_FOREACH and can hit a thread that's still running, since gdb_handlesig() only parks the thread that trapped. But before this patch, curr_cflags() on CPU X read X->singlestep_flags and cpu_single_step() stored to it from whichever thread gdb was on. Now the store lands in X->tcg_cflags. Same width, same plain accesses, same writer, same reader -- the patch changes which field carries the state, not how it's synchronized. Nothing here wants atomics that didn't want them before. I did fix the commit message, which claimed cpu_single_step() always runs on the owning thread or with the CPU stopped. True in system mode, not in the user-mode gdb_continue_partial() case. > I suspect async_run_on_cpu would be enough to trigger an > update from a non-vCPU thread to the vCPU. Agreed, fixed in v4. async_safe_run_on_cpu() was overkill. > You could even pass the sub-set of flags down in the user data and maybe > avoid having to use global atomics for those flags. I left this, but tell me if you'd rather have it. The only atomic is qatomic_read(&one_insn_per_tb), and the patch already takes it off the hot path -- it ran on every dispatch, now it runs at realize, at a gdb step change, or on an HMP command. Passing a snapshot would remove that last read, but one_insn_per_tb and qemu_loglevel are read by other code too, so the queued CPU would be working from a snapshot while everyone else sees the live global. For a path this cold that seemed like the worse trade.
