Re: [Qemu-devel] [Qemu-Devel][PATCH 2/3] Saving counters between tb_flush events.

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

On 6/14/19 6:53 AM, vandersonmr wrote:
> A new hash map was added to store the accumulated execution
> frequency of the TBs even after tb_flush events. A dump
> function was also added as a way to visualize these frequencies.
> 
> Signed-off-by: vandersonmr <vandersonmr2@gmail.com>
> ---
>  accel/tcg/translate-all.c | 59 +++++++++++++++++++++++++++++++++++++++
>  accel/tcg/translate-all.h |  2 ++
>  exec.c                    |  7 +++++
>  include/exec/exec-all.h   |  3 ++
>  include/exec/tb-context.h |  9 ++++++
>  include/qom/cpu.h         |  2 ++
>  6 files changed, 82 insertions(+)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index 5d1e08b169..0bc670ffad 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1118,6 +1118,12 @@ static inline void code_gen_alloc(size_t tb_size)
>      }
>  }
>  
> +static bool statistics_cmp(const void* ap, const void *bp) {

Watch the formatting.

> +static void do_tb_dump_exec_freq(void *p, uint32_t hash, void *userp)
> +{
> +#if TARGET_LONG_SIZE == 8
> +    TBStatistics *tbs = p;
> +    printf("%016lx\t%lu\n", tbs->pc, tbs->total_exec_freq);
> +#elif TARGET_LONG_SIZE == 4
> +    TBStatistics *tbs = p;
> +    printf("%016x\t%lu\n", tbs->pc, tbs->total_exec_freq);
> +#endif
> +}

TARGET_FMT_lx.

> +static void do_tb_read_exec_freq(void *p, uint32_t hash, void *userp)
> +{
> +    TranslationBlock *tb = p;
> +    TBStatistics tbscmp;
> +    tbscmp.pc = tb->pc;
> +    tbscmp.cs_base = tb->cs_base;
> +    tbscmp.flags = tb->flags;
> +
> +    TBStatistics *tbs = qht_lookup(userp, &tbscmp, hash);
> +
> +    uint64_t exec_freq = tb_get_and_reset_exec_freq((TranslationBlock*) p);
> +
> +    if (tbs) {
> +        tbs->total_exec_freq += exec_freq;
> +    } else {
> +        void *existing;
> +        tbs = malloc(sizeof(TBStatistics));
> +        tbs->total_exec_freq = exec_freq;
> +        tbs->pc = tb->pc;
> +        tbs->cs_base = tb->cs_base;
> +        tbs->flags = tb->flags;
> +        qht_insert(userp, tbs, hash, &existing);

If you're going to ignore the result, leave the last argument NULL.

> +    }
> +}
> +
> +void tb_read_exec_freq(void)
> +{
> +    qht_iter(&tb_ctx.htable, do_tb_read_exec_freq, &tb_ctx.tb_statistics);
> +}

Perhaps a comment that this is called with mmap_lock held.

> +extern bool enable_freq_count;

Second declaration.

> +uint64_t tb_get_and_reset_exec_freq(TranslationBlock *tb)
> +{
> +    uint64_t exec_freq = atomic_load_acquire(&tb->exec_freq);
> +    atomic_store_release(&tb->exec_freq, 0);
> +    return exec_freq;
> +}

What are you intending here?  Either this needs a comment that it is called
with a lock held, and this does not need barriers at all (atomic_read,
atomic_set).  Or this should use atomic_xchg and do the load and store in one
atomic operation.


r~