Re: [PATCH v9 02/13] accel: collecting TB execution count

[Richard Henderson]

On 10/7/19 11:28 AM, Alex Bennée wrote:
> +void HELPER(inc_exec_freq)(void *ptr)
> +{
> +    TBStatistics *stats = (TBStatistics *) ptr;
> +    g_assert(stats);
> +    atomic_inc(&stats->executions.normal);
> +}

tcg_debug_assert.

> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 114ebe48bf..b7dd1a78e5 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1784,10 +1784,17 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>      /*
>       * We want to fetch the stats structure before we start code
>       * generation so we can count interesting things about this
> -     * generation.
> +     * generation. If dfilter is in effect we will only collect stats
> +     * for the specified range.
>       */
> -    if (tb_stats_collection_enabled()) {
> +    if (tb_stats_collection_enabled() &&
> +        qemu_log_in_addr_range(tb->pc)) {
> +        uint32_t flag = get_default_tbstats_flag();
>          tb->tb_stats = tb_get_stats(phys_pc, pc, cs_base, flags);
> +
> +        if (flag & TB_EXEC_STATS) {
> +            tb->tb_stats->stats_enabled |= TB_EXEC_STATS;
> +        }

Is this intended to be

  tb->tb_stats->stats_enabled =
      (tb->tb_stats->stats_enabled & ~TB_EXEC_STATS)
    | (flag & TB_EXEC_STATS);

so that the flag eventually gets cleared?  I also don't understand placing
stats_enabled within a structure that is shared between multiple TB.

I can only imagine that TB_EXEC_STATS should really be a bit in tb->cflags.  It
wouldn't need to be in CF_HASH_MASK, but that seems to be the logical place to
put it.

> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index 70c66c538c..ec6bd829a0 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -46,6 +46,7 @@ void translator_loop(const TranslatorOps *ops, 
> DisasContextBase *db,
>  
>      ops->init_disas_context(db, cpu);
>      tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
> +    gen_tb_exec_count(tb);

Too early.  This should go after gen_tb_start().

>      /* Reset the temp count so that we can identify leaks */
>      tcg_clear_temp_count();
> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
> index 822c43cfd3..be006383b9 100644
> --- a/include/exec/gen-icount.h
> +++ b/include/exec/gen-icount.h
> @@ -32,6 +32,15 @@ static inline void gen_io_end(void)
>      tcg_temp_free_i32(tmp);
>  }
>  
> +static inline void gen_tb_exec_count(TranslationBlock *tb)
> +{
> +    if (tb_stats_enabled(tb, TB_EXEC_STATS)) {
> +        TCGv_ptr ptr = tcg_const_ptr(tb->tb_stats);
> +        gen_helper_inc_exec_freq(ptr);
> +        tcg_temp_free_ptr(ptr);
> +    }
> +}

I think this could go into translator.c, instead of gen-icount.h; it shouldn't
be used anywhere else.

> +#define TB_NOTHING    (1 << 0)

What's this?


r~