[Qemu-devel] [PATCH 0/3] Collecting TB Execution Frequency

[Qemu-devel] [PATCH 0/3] Collecting TB Execution Frequency

[vandersonmr]

This is the first series of patches related to the TCGCodeQuality GSoC project 
More at https://wiki.qemu.org/Features/TCGCodeQuality

It adds an option to instrument TBs and collects their execution frequency.
The execution frequency is then store/accumulated in an auxiliary structure
every time a tb_flush or a read happens. 

[Qemu-Devel][PATCH 1/3] Adding an optional tb execution counter.
[Qemu-Devel][PATCH 2/3] Saving counters between tb_flush events.
[Qemu-Devel][PATCH 3/3] Adding command line option to linux-user.

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

[vandersonmr]

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;
     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));
+        gen_helper_inc_exec_freq(tb_ptr);
+    }
+
     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;
 
 int icount_align_option;
 
-- 
2.22.0

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

[Richard Henderson]

On 6/14/19 6:53 AM, vandersonmr wrote:
> +void HELPER(inc_exec_freq)(void *ptr)
> +{
> +    TranslationBlock* tb = (TranslationBlock*) ptr;
> +    atomic_inc(&tb->exec_freq);
> +}
...
> +DEF_HELPER_FLAGS_1(inc_exec_freq, TCG_CALL_NO_RWG, void, ptr)
...
>      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;

It's not a frequency, but a count.

>      uint16_t icount;
>      uint32_t cflags;    /* compile flags */

Consider where you've placed the data with respect to the packing of other
members of the structure.

>  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));
> +        gen_helper_inc_exec_freq(tb_ptr);
> +    }
> +
>      tcg_ctx->exitreq_label = gen_new_label();
>      if (tb_cflags(tb) & CF_USE_ICOUNT) {
>          count = tcg_temp_local_new_i32();

By placing the increment before the exit for interrupt check instead of after,
you're kinda counting the wrong thing, because the TB hasn't executed.


> 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;

This is being declared in multiple places and initialized in multiple places.
This needs to go elsewhere.


r~

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

[Alex Bennée]

[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

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

[vandersonmr]

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) {
+    const TBStatistics *a = ap;
+    const TBStatistics *b = bp;
+    return a->pc == b->pc && a->cs_base == b->cs_base && a->flags == b->flags;
+}
+
 static bool tb_cmp(const void *ap, const void *bp)
 {
     const TranslationBlock *a = ap;
@@ -1137,6 +1143,7 @@ static void tb_htable_init(void)
     unsigned int mode = QHT_MODE_AUTO_RESIZE;
 
     qht_init(&tb_ctx.htable, tb_cmp, CODE_GEN_HTABLE_SIZE, mode);
+    qht_init(&tb_ctx.tb_statistics, statistics_cmp, CODE_GEN_HTABLE_SIZE, QHT_MODE_AUTO_RESIZE);
 }
 
 /* Must be called before using the QEMU cpus. 'tb_size' is the size
@@ -1228,6 +1235,53 @@ static gboolean tb_host_size_iter(gpointer key, gpointer value, gpointer data)
     return false;
 }
 
+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
+}
+
+void tb_dump_all_exec_freq(void)
+{
+    tb_read_exec_freq();
+    qht_iter(&tb_ctx.tb_statistics, do_tb_dump_exec_freq, NULL);
+}
+
+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);
+    }
+}
+
+void tb_read_exec_freq(void)
+{
+    qht_iter(&tb_ctx.htable, do_tb_read_exec_freq, &tb_ctx.tb_statistics);
+}
+
 /* flush all the translation blocks */
 static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
 {
@@ -1252,6 +1306,10 @@ static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
         cpu_tb_jmp_cache_clear(cpu);
     }
 
+    if (enable_freq_count) {
+        tb_read_exec_freq();
+    }
+
     qht_reset_size(&tb_ctx.htable, CODE_GEN_HTABLE_SIZE);
     page_flush_tb();
 
@@ -1723,6 +1781,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     tb->flags = flags;
     tb->cflags = cflags;
     tb->trace_vcpu_dstate = *cpu->trace_dstate;
+    tb->exec_freq = 0;
     tcg_ctx->tb_cflags = cflags;
  tb_overflow:
 
diff --git a/accel/tcg/translate-all.h b/accel/tcg/translate-all.h
index 64f5fd9a05..e13088c36d 100644
--- a/accel/tcg/translate-all.h
+++ b/accel/tcg/translate-all.h
@@ -32,6 +32,8 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                    int is_cpu_write_access);
 void tb_check_watchpoint(CPUState *cpu);
 
+extern bool enable_freq_count;
+
 #ifdef CONFIG_USER_ONLY
 int page_unprotect(target_ulong address, uintptr_t pc);
 #endif
diff --git a/exec.c b/exec.c
index e7622d1956..9b64a012ee 100644
--- a/exec.c
+++ b/exec.c
@@ -1013,6 +1013,13 @@ const char *parse_cpu_option(const char *cpu_option)
     return cpu_type;
 }
 
+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;
+}
+
 #if defined(CONFIG_USER_ONLY)
 void tb_invalidate_phys_addr(target_ulong addr)
 {
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index a80407622e..5d3d829d18 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -513,4 +513,7 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
 /* vl.c */
 extern int singlestep;
 
+void tb_read_exec_freq(void);
+void tb_dump_all_exec_freq(void);
+
 #endif
diff --git a/include/exec/tb-context.h b/include/exec/tb-context.h
index feb585e0a7..ba518d47a0 100644
--- a/include/exec/tb-context.h
+++ b/include/exec/tb-context.h
@@ -28,6 +28,14 @@
 
 typedef struct TranslationBlock TranslationBlock;
 typedef struct TBContext TBContext;
+typedef struct TBStatistics TBStatistics;
+
+struct TBStatistics {
+    target_ulong pc;
+    target_ulong cs_base;
+    uint32_t flags;
+    uint64_t total_exec_freq;
+};
 
 struct TBContext {
 
@@ -35,6 +43,7 @@ struct TBContext {
 
     /* statistics */
     unsigned tb_flush_count;
+    struct qht tb_statistics;
 };
 
 extern TBContext tb_ctx;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 5ee0046b62..593c1f1137 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -474,6 +474,8 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
     }
 }
 
+uint64_t tb_get_and_reset_exec_freq(struct TranslationBlock*);
+
 /**
  * qemu_tcg_mttcg_enabled:
  * Check whether we are running MultiThread TCG or not.
-- 
2.22.0

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

[Richard Henderson]

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~

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

[Alex Bennée]

vandersonmr <vandersonmr2@gmail.com> writes:

> 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>

I forgot to mention the formatting looks a little off here. It should be
your full name (accents and all) before the email address, e.g:

  Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  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) {
> +    const TBStatistics *a = ap;
> +    const TBStatistics *b = bp;
> +    return a->pc == b->pc && a->cs_base == b->cs_base && a->flags ==
> b->flags;

Looking at tb_cmp() bellow this I wonder if we should also take into
account the page_address values. Some TB's will be translated over a
page boundary and in theory that can change with new mappings so we need
to ensure they are really equivalent.

> +}
> +
>  static bool tb_cmp(const void *ap, const void *bp)
>  {
>      const TranslationBlock *a = ap;
> @@ -1137,6 +1143,7 @@ static void tb_htable_init(void)
>      unsigned int mode = QHT_MODE_AUTO_RESIZE;
>
>      qht_init(&tb_ctx.htable, tb_cmp, CODE_GEN_HTABLE_SIZE, mode);
> +    qht_init(&tb_ctx.tb_statistics, statistics_cmp, CODE_GEN_HTABLE_SIZE, QHT_MODE_AUTO_RESIZE);
>  }
>
>  /* Must be called before using the QEMU cpus. 'tb_size' is the size
> @@ -1228,6 +1235,53 @@ static gboolean tb_host_size_iter(gpointer key, gpointer value, gpointer data)
>      return false;
>  }
>
> +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
> +}

This will need some fixing up so deal with output to the HMP monitor. We
don't want to be directly spamming stdout with results.

> +
> +void tb_dump_all_exec_freq(void)
> +{
> +    tb_read_exec_freq();
> +    qht_iter(&tb_ctx.tb_statistics, do_tb_dump_exec_freq, NULL);
> +}

I would be tempted to insert these into a sorted GList first so we can
dump the first N hotblocks.

> +
> +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));

use g_new0(TBStatistics, 1)

> +        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);
> +    }
> +}
> +

Comment here about what we are doing? Maybe tb_copy_old_counts()?

> +void tb_read_exec_freq(void)
> +{
> +    qht_iter(&tb_ctx.htable, do_tb_read_exec_freq, &tb_ctx.tb_statistics);
> +}
> +
>  /* flush all the translation blocks */
>  static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
>  {
> @@ -1252,6 +1306,10 @@ static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
>          cpu_tb_jmp_cache_clear(cpu);
>      }
>
> +    if (enable_freq_count) {
> +        tb_read_exec_freq();
> +    }
> +
>      qht_reset_size(&tb_ctx.htable, CODE_GEN_HTABLE_SIZE);
>      page_flush_tb();
>
> @@ -1723,6 +1781,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>      tb->flags = flags;
>      tb->cflags = cflags;
>      tb->trace_vcpu_dstate = *cpu->trace_dstate;
> +    tb->exec_freq = 0;
>      tcg_ctx->tb_cflags = cflags;
>   tb_overflow:
>
> diff --git a/accel/tcg/translate-all.h b/accel/tcg/translate-all.h
> index 64f5fd9a05..e13088c36d 100644
> --- a/accel/tcg/translate-all.h
> +++ b/accel/tcg/translate-all.h
> @@ -32,6 +32,8 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>                                     int is_cpu_write_access);
>  void tb_check_watchpoint(CPUState *cpu);
>
> +extern bool enable_freq_count;
> +
>  #ifdef CONFIG_USER_ONLY
>  int page_unprotect(target_ulong address, uintptr_t pc);
>  #endif
> diff --git a/exec.c b/exec.c
> index e7622d1956..9b64a012ee 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1013,6 +1013,13 @@ const char *parse_cpu_option(const char *cpu_option)
>      return cpu_type;
>  }
>
> +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);

I'm not sure you need acquire/release semantics here as you are only
reading this in an exclusive period when all in-flight updates should
be synced (locks are implicit barriers). Folding this up into
do_tb_read_exec_freq might make this clearer.

> +    return exec_freq;
> +}
> +
>  #if defined(CONFIG_USER_ONLY)
>  void tb_invalidate_phys_addr(target_ulong addr)
>  {
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index a80407622e..5d3d829d18 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -513,4 +513,7 @@ hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>  /* vl.c */
>  extern int singlestep;
>
> +void tb_read_exec_freq(void);
> +void tb_dump_all_exec_freq(void);
> +
>  #endif
> diff --git a/include/exec/tb-context.h b/include/exec/tb-context.h
> index feb585e0a7..ba518d47a0 100644
> --- a/include/exec/tb-context.h
> +++ b/include/exec/tb-context.h
> @@ -28,6 +28,14 @@
>
>  typedef struct TranslationBlock TranslationBlock;
>  typedef struct TBContext TBContext;
> +typedef struct TBStatistics TBStatistics;
> +
> +struct TBStatistics {
> +    target_ulong pc;
> +    target_ulong cs_base;
> +    uint32_t flags;
> +    uint64_t total_exec_freq;
> +};
>
>  struct TBContext {
>
> @@ -35,6 +43,7 @@ struct TBContext {
>
>      /* statistics */
>      unsigned tb_flush_count;
> +    struct qht tb_statistics;
>  };
>
>  extern TBContext tb_ctx;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 5ee0046b62..593c1f1137 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -474,6 +474,8 @@ static inline void cpu_tb_jmp_cache_clear(CPUState *cpu)
>      }
>  }
>
> +uint64_t tb_get_and_reset_exec_freq(struct TranslationBlock*);
> +
>  /**
>   * qemu_tcg_mttcg_enabled:
>   * Check whether we are running MultiThread TCG or not.


--
Alex Bennée

[Qemu-devel] [Qemu-Devel][PATCH 3/3] Adding command line option to linux-user.

[vandersonmr]

Added -execfreq to enable execution frequency counting and dump
all the TB's addresses and their execution frequency at the end
of the execution.

Signed-off-by: vandersonmr <vandersonmr2@gmail.com>
---
 linux-user/exit.c | 5 +++++
 linux-user/main.c | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/linux-user/exit.c b/linux-user/exit.c
index bdda720553..0c6a2f2d5b 100644
--- a/linux-user/exit.c
+++ b/linux-user/exit.c
@@ -26,8 +26,13 @@
 extern void __gcov_dump(void);
 #endif
 
+extern bool enable_freq_count;
+
 void preexit_cleanup(CPUArchState *env, int code)
 {
+    if (enable_freq_count) {
+        tb_dump_all_exec_freq();
+    }
 #ifdef TARGET_GPROF
         _mcleanup();
 #endif
diff --git a/linux-user/main.c b/linux-user/main.c
index 1bf7155670..ece2d8bd8b 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -388,6 +388,11 @@ static void handle_arg_trace(const char *arg)
     trace_file = trace_opt_parse(arg);
 }
 
+static void handle_arg_execfreq(const char *arg)
+{
+    enable_freq_count = true;
+}
+
 struct qemu_argument {
     const char *argv;
     const char *env;
@@ -439,6 +444,8 @@ static const struct qemu_argument arg_table[] = {
      "",           "Seed for pseudo-random number generator"},
     {"trace",      "QEMU_TRACE",       true,  handle_arg_trace,
      "",           "[[enable=]<pattern>][,events=<file>][,file=<file>]"},
+    {"execfreq",   "QEMU_EXEC_FREQ",   false,  handle_arg_execfreq,
+     "",           "enable and dump TB's execution frequency counting"},
     {"version",    "QEMU_VERSION",     false, handle_arg_version,
      "",           "display version information and exit"},
     {NULL, NULL, false, NULL, NULL, NULL}
-- 
2.22.0

Re: [Qemu-devel] [Qemu-Devel][PATCH 3/3] Adding command line option to linux-user.

[Richard Henderson]

On 6/14/19 6:53 AM, vandersonmr wrote:
> Added -execfreq to enable execution frequency counting and dump
> all the TB's addresses and their execution frequency at the end
> of the execution.
> 
> Signed-off-by: vandersonmr <vandersonmr2@gmail.com>
> ---
>  linux-user/exit.c | 5 +++++
>  linux-user/main.c | 7 +++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/linux-user/exit.c b/linux-user/exit.c
> index bdda720553..0c6a2f2d5b 100644
> --- a/linux-user/exit.c
> +++ b/linux-user/exit.c
> @@ -26,8 +26,13 @@
>  extern void __gcov_dump(void);
>  #endif
>  
> +extern bool enable_freq_count;

A third declaration.


r~

Re: [Qemu-devel] [Qemu-Devel][PATCH 3/3] Adding command line option to linux-user.

[Alex Bennée]

[added Markus to Cc for his view on options]

vandersonmr <vandersonmr2@gmail.com> writes:

> Added -execfreq to enable execution frequency counting and dump
> all the TB's addresses and their execution frequency at the end
> of the execution.
>
> Signed-off-by: vandersonmr <vandersonmr2@gmail.com>

This works well enough but we are going to need a way to enable this for
softmmu as well. The preference for that is to add a option group to
qemu-options.hx which will allow a number of related options to be
grouped together.

The last thing that was added was the thread=multi flag to -accel
tcg,thread=multi but unfortunately the -accel option is very much a
softmmu only option group.

Maybe it's time to add a -tcg option for all the various options so we
can have (and are likely to add)?:


  -tcg tbcount=true
  -tcg tbcount=true,tbstats=file,file=output.txt
  -tcg tbcount=true,tbstats=chardev,id=statschar

There are going to future enhancements we might consider:

  -tcg jitperf=/tmp/perf.map

A long time in the future we may even have:

  -tcg multiexitblocks=true

until it defaults to true and the knob is just there to turn it off.

The closest example of an option group shared between linux-user and
softmmu is the trace code. Both builds do a:

   qemu_add_opts(&qemu_trace_opts);

(why can't this be done by module init code?)

And then they can call the general qemu_opts parser on the top level
option:

  trace_opt_parse(arg);

So maybe we need to bite the bullet and create tcg/tcg-options.c and put
our tweaking machinery in there (and the real location of
enable_freq_count)?

Markus do you have a view?


> ---
>  linux-user/exit.c | 5 +++++
>  linux-user/main.c | 7 +++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/linux-user/exit.c b/linux-user/exit.c
> index bdda720553..0c6a2f2d5b 100644
> --- a/linux-user/exit.c
> +++ b/linux-user/exit.c
> @@ -26,8 +26,13 @@
>  extern void __gcov_dump(void);
>  #endif
>
> +extern bool enable_freq_count;
> +
>  void preexit_cleanup(CPUArchState *env, int code)
>  {
> +    if (enable_freq_count) {
> +        tb_dump_all_exec_freq();
> +    }
>  #ifdef TARGET_GPROF
>          _mcleanup();
>  #endif
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 1bf7155670..ece2d8bd8b 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -388,6 +388,11 @@ static void handle_arg_trace(const char *arg)
>      trace_file = trace_opt_parse(arg);
>  }
>
> +static void handle_arg_execfreq(const char *arg)
> +{
> +    enable_freq_count = true;
> +}
> +
>  struct qemu_argument {
>      const char *argv;
>      const char *env;
> @@ -439,6 +444,8 @@ static const struct qemu_argument arg_table[] = {
>       "",           "Seed for pseudo-random number generator"},
>      {"trace",      "QEMU_TRACE",       true,  handle_arg_trace,
>       "",           "[[enable=]<pattern>][,events=<file>][,file=<file>]"},
> +    {"execfreq",   "QEMU_EXEC_FREQ",   false,  handle_arg_execfreq,
> +     "",           "enable and dump TB's execution frequency counting"},
>      {"version",    "QEMU_VERSION",     false, handle_arg_version,
>       "",           "display version information and exit"},
>      {NULL, NULL, false, NULL, NULL, NULL}


--
Alex Bennée

Re: [Qemu-devel] [PATCH 0/3] Collecting TB Execution Frequency

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190614135332.12777-1-vandersonmr2@gmail.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/3] Collecting TB Execution Frequency
Type: series
Message-id: 20190614135332.12777-1-vandersonmr2@gmail.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/155912548463.2019004.3515830305299809902.stgit@bahia.lan -> patchew/155912548463.2019004.3515830305299809902.stgit@bahia.lan
 - [tag update]      patchew/156051774276.244890.8660277280145466396.stgit@bahia.lan -> patchew/156051774276.244890.8660277280145466396.stgit@bahia.lan
Switched to a new branch 'test'
6512387 Adding command line option to linux-user.
af52efd Saving counters between tb_flush events.
906c8ef Adding an optional tb execution counter.

=== OUTPUT BEGIN ===
1/3 Checking commit 906c8ef92aac (Adding an optional tb execution counter.)
ERROR: "(foo*)" should be "(foo *)"
#24: FILE: accel/tcg/tcg-runtime.c:173:
+    TranslationBlock* tb = (TranslationBlock*) ptr;

ERROR: do not initialise globals to 0 or NULL
#83: FILE: linux-user/main.c:61:
+bool enable_freq_count = false;

ERROR: do not initialise globals to 0 or NULL
#95: FILE: vl.c:193:
+bool enable_freq_count = false;

total: 3 errors, 0 warnings, 56 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/3 Checking commit af52efd7842d (Saving counters between tb_flush events.)
ERROR: "foo* bar" should be "foo *bar"
#22: FILE: accel/tcg/translate-all.c:1121:
+static bool statistics_cmp(const void* ap, const void *bp) {

ERROR: open brace '{' following function declarations go on the next line
#22: FILE: accel/tcg/translate-all.c:1121:
+static bool statistics_cmp(const void* ap, const void *bp) {

ERROR: line over 90 characters
#35: FILE: accel/tcg/translate-all.c:1146:
+    qht_init(&tb_ctx.tb_statistics, statistics_cmp, CODE_GEN_HTABLE_SIZE, QHT_MODE_AUTO_RESIZE);

ERROR: "(foo*)" should be "(foo *)"
#70: FILE: accel/tcg/translate-all.c:1265:
+    uint64_t exec_freq = tb_get_and_reset_exec_freq((TranslationBlock*) p);

ERROR: "(foo*)" should be "(foo *)"
#190: FILE: include/qom/cpu.h:477:
+uint64_t tb_get_and_reset_exec_freq(struct TranslationBlock*);

total: 5 errors, 0 warnings, 146 lines checked

Patch 2/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/3 Checking commit 651238793473 (Adding command line option to linux-user.)
ERROR: externs should be avoided in .c files
#22: FILE: linux-user/exit.c:29:
+extern bool enable_freq_count;

total: 1 errors, 0 warnings, 32 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190614135332.12777-1-vandersonmr2@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 0/3] Collecting TB Execution Frequency

[Alex Bennée]

vandersonmr <vandersonmr2@gmail.com> writes:

> This is the first series of patches related to the TCGCodeQuality GSoC project
> More at https://wiki.qemu.org/Features/TCGCodeQuality
>
> It adds an option to instrument TBs and collects their execution frequency.
> The execution frequency is then store/accumulated in an auxiliary structure
> every time a tb_flush or a read happens.
>
> [Qemu-Devel][PATCH 1/3] Adding an optional tb execution counter.
> [Qemu-Devel][PATCH 2/3] Saving counters between tb_flush events.
> [Qemu-Devel][PATCH 3/3] Adding command line option to linux-user.

Did you explicitly set the patch prefix to qemu-devel? You don't need
to, they are added by the mailing list software.

--
Alex Bennée

Re: [Qemu-devel] [PATCH 0/3] Collecting TB Execution Frequency

[Alex Bennée]

vandersonmr <vandersonmr2@gmail.com> writes:

> This is the first series of patches related to the TCGCodeQuality GSoC project
> More at https://wiki.qemu.org/Features/TCGCodeQuality
>
> It adds an option to instrument TBs and collects their execution frequency.
> The execution frequency is then store/accumulated in an auxiliary structure
> every time a tb_flush or a read happens.
>
> [Qemu-Devel][PATCH 1/3] Adding an optional tb execution counter.
> [Qemu-Devel][PATCH 2/3] Saving counters between tb_flush events.
> [Qemu-Devel][PATCH 3/3] Adding command line option to linux-user.

One more thing:

  https://app.shippable.com/github/stsquad/qemu/runs/866/summary/console

The use of:

  uint64_t exec_freq;

breaks 32 bit builds as we violate ATOMIC_REG_SIZE. Maybe we can get
away with uint32_t? I guess we need more of an idea of the range of
these counters are likely to hit (and maybe detect overflow in the
helper?).

--
Alex Bennée