Re: [Qemu-devel] [PATCH v8 02/11] accel: collecting TB execution count

[Alex Bennée]

Vanderson Martins do Rosario <address@hidden> writes:

> Ok. I haven't change it before because I would like to be able to collect
> information for already translated TBs when I, for instance, remove the
> filter during execution. Having the TBStats already created guarantee this.
> To guarantee this in your approach, we would need to tb_flush when changing
> the filter. Does it make sense? Is that ok for you?

I think so. While tb_flush is a bit of hammer translation is pretty
cheap so things will be running pretty quickly afterwards. We don't need
to flush the old TB stats entries though - we can keep them for the
lifetime of the run.

>
> Vanderson M. Rosario
>
>
> On Fri, Aug 30, 2019 at 7:21 AM Alex Bennée <address@hidden> wrote:
>
>>
>> vandersonmr <address@hidden> writes:
>>
>> > 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.
>> >
>> > Signed-off-by: Vanderson M. do Rosario <address@hidden>
>> > ---
>> >  accel/tcg/cpu-exec.c      |  4 ++++
>> >  accel/tcg/tb-stats.c      |  5 +++++
>> >  accel/tcg/tcg-runtime.c   |  7 +++++++
>> >  accel/tcg/tcg-runtime.h   |  2 ++
>> >  accel/tcg/translate-all.c |  7 +++++++
>> >  accel/tcg/translator.c    |  1 +
>> >  include/exec/gen-icount.h |  9 +++++++++
>> >  include/exec/tb-stats.h   | 19 +++++++++++++++++++
>> >  util/log.c                |  1 +
>> >  9 files changed, 55 insertions(+)
>> >
>> > diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> > index 48272c781b..9b2b7bff80 100644
>> > --- a/accel/tcg/cpu-exec.c
>> > +++ b/accel/tcg/cpu-exec.c
>> > @@ -251,6 +251,10 @@ void cpu_exec_step_atomic(CPUState *cpu)
>> >
>> >          start_exclusive();
>> >
>> > +        if (tb_stats_enabled(tb, TB_EXEC_STATS)) {
>> > +            tb->tb_stats->executions.atomic++;
>> > +        }
>> > +
>> >          /* Since we got here, we know that parallel_cpus must be true.
>> */
>> >          parallel_cpus = false;
>> >          in_exclusive_region = true;
>> > diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
>> > index 948b107e68..1db81d83e7 100644
>> > --- a/accel/tcg/tb-stats.c
>> > +++ b/accel/tcg/tb-stats.c
>> > @@ -61,3 +61,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;
>> > +}
>> > diff --git a/accel/tcg/tcg-runtime.c b/accel/tcg/tcg-runtime.c
>> > index 8a1e408e31..6f4aafba11 100644
>> > --- a/accel/tcg/tcg-runtime.c
>> > +++ b/accel/tcg/tcg-runtime.c
>> > @@ -167,3 +167,10 @@ void HELPER(exit_atomic)(CPUArchState *env)
>> >  {
>> >      cpu_loop_exit_atomic(env_cpu(env), GETPC());
>> >  }
>> > +
>> > +void HELPER(inc_exec_freq)(void *ptr)
>> > +{
>> > +    TBStatistics *stats = (TBStatistics *) ptr;
>> > +    g_assert(stats);
>> > +    atomic_inc(&stats->executions.normal);
>> > +}
>> > diff --git a/accel/tcg/tcg-runtime.h b/accel/tcg/tcg-runtime.h
>> > index 4fa61b49b4..bf0b75dbe8 100644
>> > --- a/accel/tcg/tcg-runtime.h
>> > +++ b/accel/tcg/tcg-runtime.h
>> > @@ -28,6 +28,8 @@ DEF_HELPER_FLAGS_1(lookup_tb_ptr, TCG_CALL_NO_WG_SE,
>> ptr, env)
>> >
>> >  DEF_HELPER_FLAGS_1(exit_atomic, TCG_CALL_NO_WG, noreturn, env)
>> >
>> > +DEF_HELPER_FLAGS_1(inc_exec_freq, TCG_CALL_NO_RWG, void, ptr)
>> > +
>> >  #ifdef CONFIG_SOFTMMU
>> >
>> >  DEF_HELPER_FLAGS_5(atomic_cmpxchgb, TCG_CALL_NO_WG,
>> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
>> > index b7bccacd3b..e72aeba682 100644
>> > --- a/accel/tcg/translate-all.c
>> > +++ b/accel/tcg/translate-all.c
>> > @@ -1785,6 +1785,13 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>> >       */
>> >      if (tb_stats_collection_enabled()) {
>> >          tb->tb_stats = tb_get_stats(phys_pc, pc, cs_base, flags, tb);
>> > +
>> > +        if (qemu_log_in_addr_range(tb->pc)) {
>>
>> We can open this out because this test will always pass if no dfilter
>> has been set and there is no point creating a tb_stats record if we
>> won't fill it in. So
>>
>>   if (qemu_log_in_addr_range(tb->pc)) {
>>      tb->tb_stats = tb_get_stats(phys_pc, pc, cs_base, flags, tb);
>>      uint32_t flag = get_default_tbstats_flag();
>>
>>      if (flag & TB_EXEC_STATS) {
>>        ...
>>
>> And the additional tests that get added later. This way we'll only
>> create and collect stats for what we want.
>>
>> > +            uint32_t flag = get_default_tbstats_flag();
>> > +            if (flag & TB_EXEC_STATS) {
>> > +                tb->tb_stats->stats_enabled |= TB_EXEC_STATS;
>> > +            }
>> > +        }
>> >      } else {
>> >          tb->tb_stats = NULL;
>> >      }
>> > 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);
>> >
>> >      /* 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);
>> > +    }
>> > +}
>> > +
>> >  static inline void gen_tb_start(TranslationBlock *tb)
>> >  {
>> >      TCGv_i32 count, imm;
>> > diff --git a/include/exec/tb-stats.h b/include/exec/tb-stats.h
>> > index 898e05a36f..c4a8715400 100644
>> > --- a/include/exec/tb-stats.h
>> > +++ b/include/exec/tb-stats.h
>> > @@ -30,6 +30,9 @@
>> >  #include "exec/tb-context.h"
>> >  #include "tcg.h"
>> >
>> > +#define tb_stats_enabled(tb, JIT_STATS) \
>> > +    (tb && tb->tb_stats && (tb->tb_stats->stats_enabled & JIT_STATS))
>> > +
>> >  typedef struct TBStatistics TBStatistics;
>> >
>> >  /*
>> > @@ -46,6 +49,15 @@ struct TBStatistics {
>> >      uint32_t     flags;
>> >      /* cs_base isn't included in the hash but we do check for matches */
>> >      target_ulong cs_base;
>> > +
>> > +    uint32_t stats_enabled;
>> > +
>> > +    /* Execution stats */
>> > +    struct {
>> > +        unsigned long normal;
>> > +        unsigned long atomic;
>> > +    } executions;
>> > +
>> >      /* current TB linked to this TBStatistics */
>> >      TranslationBlock *tb;
>> >  };
>> > @@ -56,7 +68,12 @@ void init_tb_stats_htable_if_not(void);
>> >
>> >  /* TBStatistic collection controls */
>> >  enum TBStatsStatus { TB_STATS_RUNNING, TB_STATS_PAUSED,
>> TB_STATS_STOPPED };
>> > +
>> > +#define TB_NOTHING    (1 << 0)
>> > +#define TB_EXEC_STATS (1 << 1)
>> > +
>> >  extern int tcg_collect_tb_stats;
>> > +extern uint32_t default_tbstats_flag;
>> >
>> >  void enable_collect_tb_stats(void);
>> >  void disable_collect_tb_stats(void);
>> > @@ -64,4 +81,6 @@ void pause_collect_tb_stats(void);
>> >  bool tb_stats_collection_enabled(void);
>> >  bool tb_stats_collection_paused(void);
>> >
>> > +uint32_t get_default_tbstats_flag(void);
>> > +
>> >  #endif
>> > diff --git a/util/log.c b/util/log.c
>> > index 393a17115b..29021a4584 100644
>> > --- a/util/log.c
>> > +++ b/util/log.c
>> > @@ -32,6 +32,7 @@ static int log_append = 0;
>> >  static GArray *debug_regions;
>> >
>> >  int tcg_collect_tb_stats;
>> > +uint32_t default_tbstats_flag;
>> >
>> >  /* Return the number of characters emitted.  */
>> >  int qemu_log(const char *fmt, ...)
>>
>>
>> --
>> Alex Bennée
>>


-- 
Alex Bennée