Re: [PATCH v14 03/10] accel: collecting TB execution count

[Richard Henderson <richard.henderson@linaro.org>]

On 5/30/23 01:35, Fei Wu wrote:
> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
> 
> If a TB has a TBS (TBStatistics) with the TB_EXEC_STATS
> enabled, then we instrument the start code of this TB
> to atomically count the number of times it is executed.
> We count both the number of "normal" executions and atomic
> executions of a TB.
> 
> The execution count of the TB is stored in its respective
> TBS.
> 
> All TBStatistics are created by default with the flags from
> default_tbstats_flag.
> 
> [Richard Henderson created the inline gen_tb_exec_count]
> 
> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Message-Id: <20190829173437.5926-3-vandersonmr2@gmail.com>
> [AJB: Fix author]
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> ---
>   accel/tcg/cpu-exec.c          |  6 ++++++
>   accel/tcg/tb-stats.c          |  6 ++++++
>   accel/tcg/tcg-runtime.c       |  1 +
>   accel/tcg/translate-all.c     |  7 +++++--
>   accel/tcg/translator.c        | 25 +++++++++++++++++++++++++
>   include/exec/gen-icount.h     |  1 +
>   include/exec/tb-stats-flags.h |  5 +++++
>   include/exec/tb-stats.h       | 13 +++++++++++++
>   8 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 0e741960da..c0d8f26237 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -25,6 +25,7 @@
>   #include "trace.h"
>   #include "disas/disas.h"
>   #include "exec/exec-all.h"
> +#include "exec/tb-stats.h"
>   #include "tcg/tcg.h"
>   #include "qemu/atomic.h"
>   #include "qemu/rcu.h"
> @@ -562,7 +563,12 @@ void cpu_exec_step_atomic(CPUState *cpu)
>               mmap_unlock();
>           }
>   
> +        if (tb_stats_enabled(tb, TB_EXEC_STATS)) {
> +            tb->tb_stats->executions.atomic++;
> +        }
> +
>           cpu_exec_enter(cpu);
> +
>           /* execute the generated code */
>           trace_exec_tb(tb, pc);
>           cpu_tb_exec(cpu, tb, &tb_exit);
> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
> index f988bd8a31..143a52ef5c 100644
> --- a/accel/tcg/tb-stats.c
> +++ b/accel/tcg/tb-stats.c
> @@ -22,6 +22,7 @@ enum TBStatsStatus {
>   };
>   
>   static enum TBStatsStatus tcg_collect_tb_stats;
> +static uint32_t default_tbstats_flag;
>   
>   void init_tb_stats_htable(void)
>   {
> @@ -56,3 +57,8 @@ bool tb_stats_collection_paused(void)
>   {
>       return tcg_collect_tb_stats == TB_STATS_PAUSED;
>   }
> +
> +uint32_t get_default_tbstats_flag(void)
> +{
> +    return default_tbstats_flag;
> +}

What is the purpose of this function, instead of a global variable?
What is the meaning of 'default' in its name?

> @@ -295,6 +295,7 @@ static TBStatistics *tb_get_stats(tb_page_addr_t phys_pc, target_ulong pc,
>       new_stats->pc = pc;
>       new_stats->cs_base = cs_base;
>       new_stats->flags = flags;
> +    new_stats->stats_enabled = get_default_tbstats_flag();

Is this merely to record how we have generated a given TB?
What is the purpose of this flag over the global variable?

> diff --git a/include/exec/tb-stats-flags.h b/include/exec/tb-stats-flags.h
> index 87ee3d902e..fa71eb6f0c 100644
> --- a/include/exec/tb-stats-flags.h
> +++ b/include/exec/tb-stats-flags.h
> @@ -11,6 +11,9 @@
>   #ifndef TB_STATS_FLAGS
>   #define TB_STATS_FLAGS
>   
> +#define TB_NOTHING    (1 << 0)
> +#define TB_EXEC_STATS (1 << 1)

Why is NOTHING a non-zero flag?

> --- a/include/exec/tb-stats.h
> +++ b/include/exec/tb-stats.h
> @@ -31,6 +31,9 @@
>   #include "exec/tb-stats-flags.h"
>   #include "tcg/tcg.h"
>   
> +#define tb_stats_enabled(tb, JIT_STATS) \
> +    (tb && tb->tb_stats && (tb->tb_stats->stats_enabled & JIT_STATS))

Inline function, though again, why stats_enabled vs global variable?


r~