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? I suspect async_run_on_cpu would be enough to trigger an
update from a non-vCPU thread to the vCPU.
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.
> + }
> }
>
> /* exit the current TB, but without causing any exception to be raised */
> diff --git ./accel/tcg/cpu-exec.c ./accel/tcg/cpu-exec.c
> index 257211235d..148e0f583e 100644
> --- ./accel/tcg/cpu-exec.c
> +++ ./accel/tcg/cpu-exec.c
> @@ -1068,6 +1068,9 @@ bool tcg_exec_realizefn(CPUState *cpu, Error **errp)
> tcg_target_initialized = true;
> }
>
> + /* Pick up one-insn-per-tb and -d nochain from the command line. */
> + tcg_update_cflags(cpu);
> +
> cpu->tb_jmp_cache = g_new0(CPUJumpCache, 1);
> tlb_init(cpu);
> #ifndef CONFIG_USER_ONLY
> diff --git ./accel/tcg/internal-common.h ./accel/tcg/internal-common.h
> index 9e7be2d78d..853d1b51ee 100644
> --- ./accel/tcg/internal-common.h
> +++ ./accel/tcg/internal-common.h
> @@ -69,8 +69,15 @@ void tlb_destroy(CPUState *cpu);
> bool tcg_exec_realizefn(CPUState *cpu, Error **errp);
> void tcg_exec_unrealizefn(CPUState *cpu);
>
> -/* current cflags for hashing/comparison */
> -uint32_t curr_cflags(CPUState *cpu);
> +/*
> + * Current cflags for hashing/comparison. Everything that feeds into the
> + * value is folded into CPUState::tcg_cflags when it changes, by
> + * tcg_update_cflags(), so that TB dispatch only has to load it.
> + */
> +static inline uint32_t curr_cflags(CPUState *cpu)
> +{
> + return cpu->tcg_cflags;
> +}
>
> void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr);
>
> diff --git ./accel/tcg/tcg-all.c ./accel/tcg/tcg-all.c
> index 7186c10cf0..c9874a286a 100644
> --- ./accel/tcg/tcg-all.c
> +++ ./accel/tcg/tcg-all.c
> @@ -254,6 +254,7 @@ static void tcg_set_one_insn_per_tb(Object *obj, bool
> value, Error **errp)
> s->one_insn_per_tb = value;
> /* Set the global also: this changes the behaviour */
> qatomic_set(&one_insn_per_tb, value);
> + tcg_update_all_cflags();
> }
>
> static void tcg_accel_class_init(ObjectClass *oc, const void *data)
> diff --git ./cpu-target.c ./cpu-target.c
> index 4783845c9b..50be591acf 100644
> --- ./cpu-target.c
> +++ ./cpu-target.c
> @@ -24,6 +24,7 @@
> #include "exec/replay-core.h"
> #include "exec/log.h"
> #include "hw/core/cpu.h"
> +#include "system/tcg.h"
> #include "trace/trace-root.h"
>
> /* enable or disable single step mode. EXCP_DEBUG is returned by the
> @@ -35,6 +36,8 @@ void cpu_single_step(CPUState *cpu, unsigned flags)
> cpu->singlestep_flags, flags);
> cpu->singlestep_flags = flags;
>
> + tcg_update_cflags(cpu);
> +
> #if !defined(CONFIG_USER_ONLY)
> const AccelOpsClass *ops = cpus_get_accel();
> if (ops->update_guest_debug) {
> diff --git ./include/system/tcg.h ./include/system/tcg.h
> index 7622dcea30..2c2dbc753b 100644
> --- ./include/system/tcg.h
> +++ ./include/system/tcg.h
> @@ -17,6 +17,18 @@ extern bool tcg_allowed;
> #define tcg_enabled() 0
> #endif
>
> +/*
> + * Recompute the parts of CPUState::tcg_cflags that TB dispatch consumes but
> + * tcg_cflags_set() does not provide: gdb single-step, one-insn-per-tb and
> + * the CPU_LOG_TB_NOCHAIN log flag. Call whenever one of those changes.
> + *
> + * tcg_update_cflags() updates one CPU and must be called from that CPU's
> + * thread, or with it stopped. tcg_update_all_cflags() updates every CPU
> + * and is safe to call from the monitor while the vCPUs run.
> + */
> +void tcg_update_cflags(CPUState *cpu);
> +void tcg_update_all_cflags(void);
> +
> /**
> * qemu_tcg_mttcg_enabled:
> * Check whether we are running MultiThread TCG or not.
> diff --git ./stubs/meson.build ./stubs/meson.build
> index 3b2f2680b1..0025e79226 100644
> --- ./stubs/meson.build
> +++ ./stubs/meson.build
> @@ -3,6 +3,7 @@
> # below, so that it is clear who needs the stubbed functionality.
>
> stub_ss.add(files('cpu-get-clock.c'))
> +stub_ss.add(files('tcg-cflags.c'))
> stub_ss.add(files('fdset.c'))
> stub_ss.add(files('iothread-lock.c'))
> stub_ss.add(files('is-daemonized.c'))
> diff --git ./stubs/tcg-cflags.c ./stubs/tcg-cflags.c
> new file mode 100644
> index 0000000000..cb278e94aa
> --- /dev/null
> +++ ./stubs/tcg-cflags.c
> @@ -0,0 +1,16 @@
> +/*
> + * Stub for tcg_update_all_cflags(), for binaries that link util/log.c
> + * or cpu-target.c but not TCG.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#include "qemu/osdep.h"
> +#include "system/tcg.h"
> +
> +void tcg_update_cflags(CPUState *cpu)
> +{
> +}
> +
> +void tcg_update_all_cflags(void)
> +{
> +}
> diff --git ./util/log.c ./util/log.c
> index 7cffbc1bf8..3fa46a67fa 100644
> --- ./util/log.c
> +++ ./util/log.c
> @@ -27,6 +27,7 @@
> #include "qemu/thread.h"
> #include "qemu/lockable.h"
> #include "qemu/rcu.h"
> +#include "system/tcg.h"
> #ifdef CONFIG_LINUX
> #include <sys/syscall.h>
> #endif
> @@ -301,6 +302,9 @@ static bool qemu_set_log_internal(const char *filename,
> bool changed_name,
> #endif
> qemu_loglevel = log_flags;
>
> + /* CPU_LOG_TB_NOCHAIN feeds into the per-CPU cflags. */
> + tcg_update_all_cflags();
> +
> daemonized = is_daemonized();
> need_to_open_file = false;
> if (!daemonized) {
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
