On 6/1/2023 9:23 AM, Richard Henderson wrote:
> On 5/30/23 01:35, Fei Wu wrote:
>> From: "Vanderson M. do Rosario" <vanderson...@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 <dgilb...@redhat.com>
>> Signed-off-by: Vanderson M. do Rosario <vanderson...@gmail.com>
>> Message-Id: <20190829173437.5926-8-vanderson...@gmail.com>
>> Message-Id: <20190829173437.5926-9-vanderson...@gmail.com>
>> [AJB: fix authorship]
>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>> Signed-off-by: Fei Wu <fei2...@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~
