Re: [PATCH v14 06/10] monitor: adding tb_stats hmp command

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

On 5/30/23 01:35, Fei Wu wrote:
> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
> 
> 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 <dgilbert@redhat.com>
> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Message-Id: <20190829173437.5926-8-vandersonmr2@gmail.com>
> Message-Id: <20190829173437.5926-9-vandersonmr2@gmail.com>
> [AJB: fix authorship]
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> ---


I still don't see what pause does.

> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
> index 68ac7d3f73..805e1fc74d 100644
> --- a/accel/tcg/tb-stats.c
> +++ b/accel/tcg/tb-stats.c
> @@ -16,18 +16,20 @@
>   #include "qemu/timer.h"
>   
>   #include "exec/tb-stats.h"
> +#include "exec/tb-flush.h"
>   #include "tb-context.h"
>   
>   /* TBStatistic collection controls */
>   enum TBStatsStatus {
>       TB_STATS_DISABLED = 0,
>       TB_STATS_RUNNING,
> -    TB_STATS_PAUSED,
> -    TB_STATS_STOPPED
> +    TB_STATS_PAUSED
>   };

Why?

>   
>   static enum TBStatsStatus tcg_collect_tb_stats;
>   static uint32_t default_tbstats_flag;
> +/* only accessed in safe work */
> +static GList *last_search;
>   
>   uint64_t dev_time;
>   
> @@ -170,6 +172,101 @@ void dump_jit_profile_info(TCGProfile *s, GString *buf)
>       g_free(jpi);
>   }
>   
> +static void free_tbstats(void *p, uint32_t hash, void *userp)
> +{
> +    g_free(p);
> +}
> +
> +static void clean_tbstats(void)
> +{
> +    /* remove all tb_stats */
> +    qht_iter(&tb_ctx.tb_stats, free_tbstats, NULL);
> +    qht_destroy(&tb_ctx.tb_stats);
> +}
> +
> +void do_hmp_tbstats_safe(CPUState *cpu, run_on_cpu_data icmd)
> +{
> +    struct TbstatsCommand *cmdinfo = icmd.host_ptr;
> +    int cmd = cmdinfo->cmd;
> +    uint32_t level = cmdinfo->level;
> +
> +    switch (cmd) {
> +    case START:
> +        if (tb_stats_collection_enabled()) {
> +            qemu_printf("TB information already being recorded");
> +            return;
> +        } else if (tb_stats_collection_paused()) {
> +            set_tbstats_flags(level);
> +        } else {
> +            qht_init(&tb_ctx.tb_stats, tb_stats_cmp, CODE_GEN_HTABLE_SIZE,
> +                     QHT_MODE_AUTO_RESIZE);
> +        }
> +
> +        set_default_tbstats_flag(level);
> +        enable_collect_tb_stats();
> +        tb_flush(cpu);
> +        break;
> +    case PAUSE:
> +        if (!tb_stats_collection_enabled()) {
> +            qemu_printf("TB information not being recorded");
> +            return;
> +        }
> +
> +        /*
> +         * Continue to create TBStatistic structures but stop collecting
> +         * statistics
> +         */
> +        pause_collect_tb_stats();
> +        set_default_tbstats_flag(TB_NOTHING);
> +        set_tbstats_flags(TB_PAUSED);
> +        tb_flush(cpu);
> +        break;
> +    case STOP:
> +        if (tb_stats_collection_disabled()) {
> +            qemu_printf("TB information not being recorded");
> +            return;
> +        }
> +
> +        /* Dissalloc all TBStatistics structures and stop creating new ones */
> +        disable_collect_tb_stats();
> +        clean_tbstats();
> +        tb_flush(cpu);
> +        break;
> +    case FILTER:
> +        if (!tb_stats_collection_enabled()) {
> +            qemu_printf("TB information not being recorded");
> +            return;
> +        }
> +        if (!last_search) {
> +            qemu_printf(
> +                    "no search on record! execute info tbs before filtering!");
> +            return;
> +        }
> +
> +        set_default_tbstats_flag(TB_NOTHING);
> +
> +        /*
> +         * Set all tbstats as paused, then return only the ones from last_search
> +         */
> +        pause_collect_tb_stats();
> +        set_tbstats_flags(TB_PAUSED);
> +
> +        for (GList *iter = last_search; iter; iter = g_list_next(iter)) {
> +            TBStatistics *tbs = iter->data;
> +            tbs->stats_enabled = level;
> +        }
> +
> +        tb_flush(cpu);
> +
> +        break;
> +    default: /* INVALID */
> +        g_assert_not_reached();
> +        break;
> +    }
> +
> +    g_free(cmdinfo);
> +}

Why isn't all of this in monitor.c?
It's not used or usable from user-only mode.

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

Again, I question why a global variable is not more appropriate.


r~