Re: [PATCH v9 07/13] monitor: adding tb_stats hmp command

[Richard Henderson]

On 10/7/19 11:28 AM, Alex Bennée wrote:
> From: "Vanderson M. do Rosario" <address@hidden>
> 
> Adding tb_stats [start|pause|stop|filter] command to hmp.
> This allows controlling the collection of statistics.
> It is also possible to set the level of collection:
> all, jit, or exec.
> 
> tb_stats filter allow to only collect statistics for the TB
> in the last_search list.
> 
> The goal of this command is to allow the dynamic exploration
> of the TCG behavior and quality. Therefore, for now, a
> corresponding QMP command is not worthwhile.
> 
> Acked-by: Dr. David Alan Gilbert <address@hidden>
> Signed-off-by: Vanderson M. do Rosario <address@hidden>
> Message-Id: <address@hidden>
> [AJB: fix authorship]
> Signed-off-by: Alex Bennée <address@hidden>
> 
> monitor: add tb_stats [start|pause|stop|filter] command to hmp.
> 
> This allows controlling the collection of statistics.
> It is also possible to set the level of collection:
> all, jit, or exec.
> 
> tb_stats filter allow to only collect statistics for the TB
> in the last_search list.
> 
> The goal of this command is to allow the dynamic exploration
> of the TCG behavior and quality. Therefore, for now, a
> corresponding QMP command is not worthwhile.
> 
> Acked-by: Dr. David Alan Gilbert <address@hidden>
> Signed-off-by: Vanderson M. do Rosario <address@hidden>
> Message-Id: <address@hidden>
> [AJB: fix authorship]
> Signed-off-by: Alex Bennée <address@hidden>
> ---

Duplicated commit message contents.

> +void do_hmp_tbstats_safe(CPUState *cpu, run_on_cpu_data icmd)

Deserves a comment "Run via async_safe_run_on_cpu", as otherwise the "safe"
part of the function name is fairly obscure.

> +    char *slevel = (char *) qdict_get_try_str(qdict, "level");
> +    uint32_t level = TB_EXEC_STATS | TB_JIT_STATS | TB_JIT_TIME;
> +    if (slevel) {
> +        if (strcmp(slevel, "jit") == 0) {
> +            level = TB_JIT_STATS;
> +        } else if (strcmp(slevel, "exec") == 0) {
> +            level = TB_EXEC_STATS;
> +        } else if (strcmp(slevel, "time") == 0) {
> +            level = TB_JIT_TIME;
> +        }
> +    }

No support for "+" or "all" as we do for the command-line?


> +static void reset_tbstats_flag(void *p, uint32_t hash, void *userp)
> +{
> +    uint32_t flag = *((int *)userp);

Type cast doesn't match...

> +    TBStatistics *tbs = p;
> +    tbs->stats_enabled = flag;
> +}
> +
> +void set_tbstats_flags(uint32_t flag)
> +{
> +    /* iterate over tbstats setting their flag as TB_NOTHING */
> +    qht_iter(&tb_ctx.tb_stats, reset_tbstats_flag, &flag);
> +}

... here.

It does make me wonder if you'd do just as well with (void *)(uintptr_t)flag
and avoid the indirection.  It is an opaque quantity shared only between these
two functions.


>  static void hmp_info_profile(Monitor *mon, const QDict *qdict)
>  {
> +#ifdef CONFIG_TCG
>      dump_jit_exec_time_info(dev_time);
>      dev_time = 0;
> +#else
> +    error_report("TCG should be enabled!");
> +#endif
>  }
>  
>  /* Capture support */
> diff --git a/vl.c b/vl.c
> index f91bc6fb4c..781fddaf18 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1796,11 +1796,17 @@ static bool main_loop_should_exit(void)
>  
>  static void main_loop(void)
>  {
> +#ifdef CONFIG_TCG
>      uint64_t ti;
> +#endif
>      while (!main_loop_should_exit()) {
> +#ifdef CONFIG_TCG
>          ti = profile_getclock();
> +#endif
>          main_loop_wait(false);
> +#ifdef CONFIG_TCG
>          dev_time += profile_getclock() - ti;
> +#endif
>      }
>  }

All of this was CONFIG_PROFILE just a few patches ago.  If you need this, then
you broke bisection with !defined(CONFIG_TCG) a few patches ago.


r~