Re: [RFC PATCH 1/8] accel/tcg: cache the result of curr_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.

I agree this would be useful to remove but...

>  
>  static inline tb_page_addr_t tb_page_addr0(const TranslationBlock *tb)
> diff --git ./include/hw/core/cpu.h ./include/hw/core/cpu.h
> index b54035fb13..172872d005 100644
> --- ./include/hw/core/cpu.h
> +++ ./include/hw/core/cpu.h
> @@ -411,10 +411,16 @@ struct qemu_work_item;
>   *   to a cluster this will be UNASSIGNED_CLUSTER_INDEX; otherwise it will
>   *   be the same as the cluster-id property of the CPU object's TYPE_CPU_CLUSTER
>   *   QOM parent.
> - *   Under TCG this value is propagated to @tcg_cflags.
> + *   Under TCG this value is propagated to @tcg_cflags_priv.
>   *   See TranslationBlock::TCG CF_CLUSTER_MASK.
>   * @start_powered_off: Indicates whether the CPU starts in powered-off state.
> - * @tcg_cflags: Pre-computed cflags for this cpu.
> + * @tcg_cflags_priv: Pre-computed cflags for this cpu.  Private to
> + *   tcg_cflags_has() and tcg_cflags_set(): @tcg_curr_cflags is derived from
> + *   it and is refreshed by the setter, so a direct assignment here would
> + *   leave the two out of step.  The name is deliberately awkward to make an
> + *   open-coded access fail to compile rather than silently go stale.
> + * @tcg_curr_cflags: Cached result of curr_cflags(), recomputed by
> + *   tcg_update_curr_cflags() whenever any of its inputs change.

This seems like introducing additional cache state to track. Why not
just provide a helper to call when ss/gdb/log conditions change and
change the current curr_cflags() code to only validate the helper has
been called when TCG debugging is turned on. See ppc_get_tb_cpu_state or
assert_hflags_rebuild_correctly for examples.

>   * @nr_threads: Number of threads within this CPU core.
>   * @thread: Host thread details, only live once @created is #true
>   * @sem: WIN32 only semaphore used only for qtest
> @@ -557,7 +563,8 @@ struct CPUState {
>      /* TODO Move common fields from CPUArchState here. */
>      int cpu_index;
>      int cluster_index;
> -    uint32_t tcg_cflags;
> +    uint32_t tcg_cflags_priv;
> +    uint32_t tcg_curr_cflags;
>      uint32_t halted;
>      int32_t exception_index;
>  
<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro