Re: [PATCH v3 1/7] accel/tcg: fold the dynamic cflags into CPUState::tcg_cflags

["Alex Bennée" <alex.bennee@linaro.org>]

Matt Turner <mattst88@gmail.com> 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 <mattst88@gmail.com>
> ---
>  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