Re: [Qemu-devel] [Qemu-Devel][PATCH 1/3] Adding an optional tb execution counter.

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

[repeat of reply to wrong email...]

vandersonmr <vandersonmr2@gmail.com> writes:

> An uint64_t counter was added in the TranslationBlock struct and
> it is incremented every time that the TB is executed.
>
> Signed-off-by: vandersonmr <vandersonmr2@gmail.com>
> ---
>  accel/tcg/tcg-runtime.c   | 6 ++++++
>  accel/tcg/tcg-runtime.h   | 2 ++
>  include/exec/exec-all.h   | 1 +
>  include/exec/gen-icount.h | 7 +++++++
>  linux-user/main.c         | 1 +
>  vl.c                      | 1 +
>  6 files changed, 18 insertions(+)
>
> diff --git a/accel/tcg/tcg-runtime.c b/accel/tcg/tcg-runtime.c
> index 8a1e408e31..d1f4127d31 100644
> --- a/accel/tcg/tcg-runtime.c
> +++ b/accel/tcg/tcg-runtime.c
> @@ -167,3 +167,9 @@ void HELPER(exit_atomic)(CPUArchState *env)
>  {
>      cpu_loop_exit_atomic(env_cpu(env), GETPC());
>  }
> +
> +void HELPER(inc_exec_freq)(void *ptr)
> +{
> +    TranslationBlock* tb = (TranslationBlock*) ptr;
> +    atomic_inc(&tb->exec_freq);
> +}
> 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/include/exec/exec-all.h b/include/exec/exec-all.h
> index 16034ee651..a80407622e 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -342,6 +342,7 @@ struct TranslationBlock {
>      uint32_t flags; /* flags defining in which context the code was generated */
>      uint16_t size;      /* size of target code for this block (1 <=
>                             size <= TARGET_PAGE_SIZE) */
> +    uint64_t exec_freq;

Maybe exec_count would be more correct. Frequency occurs over a given
time. Also a single line comment would be useful.

>      uint16_t icount;
>      uint32_t cflags;    /* compile flags */
>  #define CF_COUNT_MASK  0x00007fff
> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
> index f7669b6841..a540d12005 100644
> --- a/include/exec/gen-icount.h
> +++ b/include/exec/gen-icount.h
> @@ -6,11 +6,18 @@
>  /* Helpers for instruction counting code generation.  */
>
>  static TCGOp *icount_start_insn;
> +extern bool enable_freq_count;
>
>  static inline void gen_tb_start(TranslationBlock *tb)
>  {
>      TCGv_i32 count, imm;
>
> +    if (enable_freq_count) {
> +        TCGv_ptr tb_ptr = tcg_temp_new_ptr();
> +        tcg_gen_trunc_i64_ptr(tb_ptr, tcg_const_i64((int64_t) tb));

TIL: tcg_const_ptr which elides the details of casting to a TCGv_ptr on
32 and 64 bit hosts. So:

    TCGv_ptr tb_ptr = tcg_const_ptr(tb);
    gen_helper_inc_exec_freq(tb_ptr);
    tcg_temp_free_ptr(tb_ptr);

(don't forget to free temporaries once used, otherwise the translator
will keep tracking them. This should show up in a --enable-debug-tcg build).


> +        gen_helper_inc_exec_freq(tb_ptr);
> +    }
> +

I think we should move the counter bellow the icount check otherwise we
might be counting TB's that never execute because an IRQ was pending.

>      tcg_ctx->exitreq_label = gen_new_label();
>      if (tb_cflags(tb) & CF_USE_ICOUNT) {
>          count = tcg_temp_local_new_i32();
> diff --git a/linux-user/main.c b/linux-user/main.c
> index a59ae9439d..1bf7155670 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -58,6 +58,7 @@ static const char *seed_optarg;
>  unsigned long mmap_min_addr;
>  unsigned long guest_base;
>  int have_guest_base;
> +bool enable_freq_count = false;
>
>  /*
>   * When running 32-on-64 we should make sure we can fit all of the possible
> diff --git a/vl.c b/vl.c
> index 005468cbfb..cb6c0ad63d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -190,6 +190,7 @@ bool boot_strict;
>  uint8_t *boot_splash_filedata;
>  int only_migratable; /* turn it off unless user states otherwise */
>  bool wakeup_suspend_enabled;
> +bool enable_freq_count = false;

We only need the space allocated once in common code. Maybe
accel/tcg/translator.c?

Also you don't need to initialise to false (== 0 == initial bss setting).

>
>  int icount_align_option;

--
Alex Bennée