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>
---
accel/tcg/monitor.c | 45 +++++++++++++
accel/tcg/tb-stats.c | 121 +++++++++++++++++++++++++++++++++-
hmp-commands.hx | 16 +++++
include/exec/tb-stats-flags.h | 2 +
include/exec/tb-stats.h | 10 +++
include/monitor/hmp.h | 1 +
6 files changed, 192 insertions(+), 3 deletions(-)
diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
index 2bc87f2642..2e00f10267 100644
--- a/accel/tcg/monitor.c
+++ b/accel/tcg/monitor.c
@@ -11,7 +11,9 @@
#include "qapi/error.h"
#include "qapi/type-helpers.h"
#include "qapi/qapi-commands-machine.h"
+#include "qapi/qmp/qdict.h"
#include "monitor/monitor.h"
+#include "monitor/hmp.h"
#include "sysemu/cpus.h"
#include "sysemu/cpu-timers.h"
#include "sysemu/tcg.h"
@@ -87,6 +89,49 @@ HumanReadableText *qmp_x_query_opcount(Error **errp)
}
#ifdef CONFIG_TCG
+void hmp_tbstats(Monitor *mon, const QDict *qdict)
+{
+ if (!tcg_enabled()) {
+ error_report("TB information is only available with accel=tcg");
+ return;
+ }
+
+ char *cmd = (char *) qdict_get_try_str(qdict, "command");
+ enum TbstatsCmd icmd = -1;
+
+ if (strcmp(cmd, "start") == 0) {
+ icmd = START;
+ } else if (strcmp(cmd, "pause") == 0) {
+ icmd = PAUSE;
+ } else if (strcmp(cmd, "stop") == 0) {
+ icmd = STOP;
+ } else if (strcmp(cmd, "filter") == 0) {
+ icmd = FILTER;
+ } else {
+ error_report("invalid command!");
+ return;
+ }
+
+ 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;
+ }
+ }
+
+ struct TbstatsCommand *tbscommand = g_new0(struct TbstatsCommand, 1);
+ tbscommand->cmd = icmd;
+ tbscommand->level = level;
+ async_safe_run_on_cpu(first_cpu, do_hmp_tbstats_safe,
+ RUN_ON_CPU_HOST_PTR(tbscommand));
+
+}
+
HumanReadableText *qmp_x_query_profile(Error **errp)
{
g_autoptr(GString) buf = g_string_new("");
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
};
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);
+}
+
void init_tb_stats_htable(void)
{
if (!tb_ctx.tb_stats.map && tb_stats_collection_enabled()) {
@@ -186,7 +283,7 @@ void enable_collect_tb_stats(void)
void disable_collect_tb_stats(void)
{
- tcg_collect_tb_stats = TB_STATS_STOPPED;
+ tcg_collect_tb_stats = TB_STATS_DISABLED;
}
void pause_collect_tb_stats(void)
@@ -199,11 +296,29 @@ bool tb_stats_collection_enabled(void)
return tcg_collect_tb_stats == TB_STATS_RUNNING;
}
+bool tb_stats_collection_disabled(void)
+{
+ return tcg_collect_tb_stats == TB_STATS_DISABLED;
+}
+
bool tb_stats_collection_paused(void)
{
return tcg_collect_tb_stats == TB_STATS_PAUSED;
}
+static void reset_tbstats_flag(void *p, uint32_t hash, void *userp)
+{
+ uint32_t flag = *((int *)userp);
+ 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);
+}
+
uint32_t get_default_tbstats_flag(void)
{
return default_tbstats_flag;
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 2cbd0f77a0..9a40215d34 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1670,6 +1670,22 @@ SRST
Executes a qemu-io command on the given block device.
ERST
+#if defined(CONFIG_TCG)
+ {
+ .name = "tb_stats",
+ .args_type = "command:s,level:s?",
+ .params = "command [stats_level]",
+ .help = "Control tb statistics collection:"
+ "tb_stats (start|pause|stop|filter) [all|jit_stats|exec_stats]",
+ .cmd = hmp_tbstats,
+ },
+#endif
+
+SRST
+``tb_stats`` *command* *stats_level*
+ Control recording tb statistics
+ERST
+
{
.name = "qom-list",
.args_type = "path:s?",
diff --git a/include/exec/tb-stats-flags.h b/include/exec/tb-stats-flags.h
index 04adaee8d9..a3897c99b1 100644
--- a/include/exec/tb-stats-flags.h
+++ b/include/exec/tb-stats-flags.h
@@ -16,12 +16,14 @@
#define TB_JIT_STATS (1 << 2)
#define TB_JIT_TIME (1 << 3)
#define TB_ALL_STATS (TB_EXEC_STATS | TB_JIT_STATS | TB_JIT_TIME)
+#define TB_PAUSED (1 << 4)
/* TBStatistic collection controls */
void enable_collect_tb_stats(void);
void disable_collect_tb_stats(void);
void pause_collect_tb_stats(void);
bool tb_stats_collection_enabled(void);
+bool tb_stats_collection_disabled(void);
bool tb_stats_collection_paused(void);
uint32_t get_default_tbstats_flag(void);
diff --git a/include/exec/tb-stats.h b/include/exec/tb-stats.h
index 72585c448a..4bb343870b 100644
--- a/include/exec/tb-stats.h
+++ b/include/exec/tb-stats.h
@@ -33,6 +33,9 @@
#include "exec/tb-stats-flags.h"
+enum SortBy { SORT_BY_HOTNESS, SORT_BY_HG /* Host/Guest */, SORT_BY_SPILLS };
+enum TbstatsCmd { START, PAUSE, STOP, FILTER };
+
#define tb_stats_enabled(tb, JIT_STATS) \
(tb && tb->tb_stats && (tb->tb_stats->stats_enabled & JIT_STATS))
@@ -114,4 +117,11 @@ void init_tb_stats_htable(void);
void dump_jit_profile_info(TCGProfile *s, GString *buf);
void dump_jit_exec_time_info(uint64_t dev_time, GString *buf);
+struct TbstatsCommand {
+ enum TbstatsCmd cmd;
+ uint32_t level;
+};
+
+void do_hmp_tbstats_safe(CPUState *cpu, run_on_cpu_data icmd);
+
#endif
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 13f9a2dedb..2e7f141754 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -181,5 +181,6 @@ void hmp_ioport_write(Monitor *mon, const QDict *qdict);
void hmp_boot_set(Monitor *mon, const QDict *qdict);
void hmp_info_mtree(Monitor *mon, const QDict *qdict);
void hmp_info_cryptodev(Monitor *mon, const QDict *qdict);
+void hmp_tbstats(Monitor *mon, const QDict *qdict);
#endif
--
2.25.1
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~
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~
On 6/1/23 00:20, Wu, Fei wrote:
> 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
This use case seems very complicated. Perhaps start with something simpler.
>>> 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.
Then this should be squashed back to patch 2, so that it is never added.
>> 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().
I mean accel/tcg/monitor.c, which is built only when there exists a monitor.
r~
© 2016 - 2026 Red Hat, Inc.