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

2019-10-08 Thread 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(>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~



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

2019-10-07 Thread Alex Bennée
From: "Vanderson M. do Rosario" 

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 
Message-Id: <20190829173437.5926-3-vanderson...@gmail.com>
[AJB: Fix author]
Signed-off-by: Alex Bennée 

---
AJB:
  - move default_tbstats_flag to tb-stats
  - hoist dfilter check
---
 accel/tcg/cpu-exec.c  |  4 
 accel/tcg/tb-stats.c  |  6 ++
 accel/tcg/tcg-runtime.c   |  7 +++
 accel/tcg/tcg-runtime.h   |  2 ++
 accel/tcg/translate-all.c | 11 +--
 accel/tcg/translator.c|  1 +
 include/exec/gen-icount.h |  9 +
 include/exec/tb-stats.h   | 18 ++
 8 files changed, 56 insertions(+), 2 deletions(-)

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 8208f4a0ad..ee0506bff1 100644
--- a/accel/tcg/tb-stats.c
+++ b/accel/tcg/tb-stats.c
@@ -16,6 +16,7 @@
 enum TBStatsStatus { TB_STATS_RUNNING, TB_STATS_PAUSED, TB_STATS_STOPPED };
 
 static enum TBStatsStatus tcg_collect_tb_stats;
+static uint32_t default_tbstats_flag;
 
 void init_tb_stats_htable_if_not(void)
 {
@@ -50,3 +51,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(>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 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;
+}
 } 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