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

[Wu, Fei]

On 6/1/2023 9:23 AM, Richard Henderson wrote:
> 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.
> 
maybe it's necessary to discuss the user scenario, I suppose the
original design is for this case:
* start
* pause - there are some interesting stats we want to keep
* not interested timeline
* start again - continue to investigate the interesting ones

>> 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?
> 
STOPPED is the same as DISABLED.

>>     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.
> 
closer to tb-stats, or closer to monitor? It seems to me monitor.c only
contains the shim layer of qmp/hmp, the real stuffs which are large
enough are put together with their logic part, e.g. dump_exec_info().

Thanks,
Fei.

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