* [PATCH v17 01/16] accel/tcg: Move HMP info jit and info opcount code
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-10 12:09 ` Philippe Mathieu-Daudé
2023-10-15 12:58 ` Alex Bennée
2023-10-03 18:30 ` [PATCH v17 02/16] tcg: Record orig_nb_ops TCGContext Richard Henderson
` (15 subsequent siblings)
16 siblings, 2 replies; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu
Move all of it into accel/tcg/monitor.c. This puts everything
about tcg that is only used by the monitor in the same place.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/internal-common.h | 2 -
include/exec/cputlb.h | 1 -
include/tcg/tcg.h | 3 -
accel/tcg/cputlb.c | 15 ----
accel/tcg/monitor.c | 154 ++++++++++++++++++++++++++++++++++++
accel/tcg/translate-all.c | 127 -----------------------------
tcg/tcg.c | 10 ---
7 files changed, 154 insertions(+), 158 deletions(-)
diff --git a/accel/tcg/internal-common.h b/accel/tcg/internal-common.h
index 3b2277e6e9..edefd0dcb7 100644
--- a/accel/tcg/internal-common.h
+++ b/accel/tcg/internal-common.h
@@ -14,8 +14,6 @@
extern int64_t max_delay;
extern int64_t max_advance;
-void dump_exec_info(GString *buf);
-
/*
* Return true if CS is not running in parallel with other cpus, either
* because there are no other cpus or we are within an exclusive context.
diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
index 19b16e58f8..6da1462c4f 100644
--- a/include/exec/cputlb.h
+++ b/include/exec/cputlb.h
@@ -26,6 +26,5 @@
/* cputlb.c */
void tlb_protect_code(ram_addr_t ram_addr);
void tlb_unprotect_code(ram_addr_t ram_addr);
-void tlb_flush_counts(size_t *full, size_t *part, size_t *elide);
#endif
#endif
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 680ff00722..82b4625773 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -842,9 +842,6 @@ static inline TCGv_ptr tcg_temp_new_ptr(void)
return temp_tcgv_ptr(t);
}
-void tcg_dump_info(GString *buf);
-void tcg_dump_op_count(GString *buf);
-
#define TCG_CT_CONST 1 /* any constant of register size */
typedef struct TCGArgConstraint {
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index b8c5e345b8..13986820fe 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -321,21 +321,6 @@ static void flush_all_helper(CPUState *src, run_on_cpu_func fn,
}
}
-void tlb_flush_counts(size_t *pfull, size_t *ppart, size_t *pelide)
-{
- CPUState *cpu;
- size_t full = 0, part = 0, elide = 0;
-
- CPU_FOREACH(cpu) {
- full += qatomic_read(&cpu->neg.tlb.c.full_flush_count);
- part += qatomic_read(&cpu->neg.tlb.c.part_flush_count);
- elide += qatomic_read(&cpu->neg.tlb.c.elide_flush_count);
- }
- *pfull = full;
- *ppart = part;
- *pelide = elide;
-}
-
static void tlb_flush_by_mmuidx_async_work(CPUState *cpu, run_on_cpu_data data)
{
uint16_t asked = data.host_int;
diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
index caf1189e0b..093efe9714 100644
--- a/accel/tcg/monitor.c
+++ b/accel/tcg/monitor.c
@@ -8,6 +8,7 @@
#include "qemu/osdep.h"
#include "qemu/accel.h"
+#include "qemu/qht.h"
#include "qapi/error.h"
#include "qapi/type-helpers.h"
#include "qapi/qapi-commands-machine.h"
@@ -17,6 +18,7 @@
#include "sysemu/tcg.h"
#include "tcg/tcg.h"
#include "internal-common.h"
+#include "tb-context.h"
static void dump_drift_info(GString *buf)
@@ -50,6 +52,153 @@ static void dump_accel_info(GString *buf)
one_insn_per_tb ? "on" : "off");
}
+static void print_qht_statistics(struct qht_stats hst, GString *buf)
+{
+ uint32_t hgram_opts;
+ size_t hgram_bins;
+ char *hgram;
+
+ if (!hst.head_buckets) {
+ return;
+ }
+ g_string_append_printf(buf, "TB hash buckets %zu/%zu "
+ "(%0.2f%% head buckets used)\n",
+ hst.used_head_buckets, hst.head_buckets,
+ (double)hst.used_head_buckets /
+ hst.head_buckets * 100);
+
+ hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
+ hgram_opts |= QDIST_PR_100X | QDIST_PR_PERCENT;
+ if (qdist_xmax(&hst.occupancy) - qdist_xmin(&hst.occupancy) == 1) {
+ hgram_opts |= QDIST_PR_NODECIMAL;
+ }
+ hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
+ g_string_append_printf(buf, "TB hash occupancy %0.2f%% avg chain occ. "
+ "Histogram: %s\n",
+ qdist_avg(&hst.occupancy) * 100, hgram);
+ g_free(hgram);
+
+ hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
+ hgram_bins = qdist_xmax(&hst.chain) - qdist_xmin(&hst.chain);
+ if (hgram_bins > 10) {
+ hgram_bins = 10;
+ } else {
+ hgram_bins = 0;
+ hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
+ }
+ hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
+ g_string_append_printf(buf, "TB hash avg chain %0.3f buckets. "
+ "Histogram: %s\n",
+ qdist_avg(&hst.chain), hgram);
+ g_free(hgram);
+}
+
+struct tb_tree_stats {
+ size_t nb_tbs;
+ size_t host_size;
+ size_t target_size;
+ size_t max_target_size;
+ size_t direct_jmp_count;
+ size_t direct_jmp2_count;
+ size_t cross_page;
+};
+
+static gboolean tb_tree_stats_iter(gpointer key, gpointer value, gpointer data)
+{
+ const TranslationBlock *tb = value;
+ struct tb_tree_stats *tst = data;
+
+ tst->nb_tbs++;
+ tst->host_size += tb->tc.size;
+ tst->target_size += tb->size;
+ if (tb->size > tst->max_target_size) {
+ tst->max_target_size = tb->size;
+ }
+ if (tb->page_addr[1] != -1) {
+ tst->cross_page++;
+ }
+ if (tb->jmp_reset_offset[0] != TB_JMP_OFFSET_INVALID) {
+ tst->direct_jmp_count++;
+ if (tb->jmp_reset_offset[1] != TB_JMP_OFFSET_INVALID) {
+ tst->direct_jmp2_count++;
+ }
+ }
+ return false;
+}
+
+static void tlb_flush_counts(size_t *pfull, size_t *ppart, size_t *pelide)
+{
+ CPUState *cpu;
+ size_t full = 0, part = 0, elide = 0;
+
+ CPU_FOREACH(cpu) {
+ full += qatomic_read(&cpu->neg.tlb.c.full_flush_count);
+ part += qatomic_read(&cpu->neg.tlb.c.part_flush_count);
+ elide += qatomic_read(&cpu->neg.tlb.c.elide_flush_count);
+ }
+ *pfull = full;
+ *ppart = part;
+ *pelide = elide;
+}
+
+static void tcg_dump_info(GString *buf)
+{
+ g_string_append_printf(buf, "[TCG profiler not compiled]\n");
+}
+
+static void dump_exec_info(GString *buf)
+{
+ struct tb_tree_stats tst = {};
+ struct qht_stats hst;
+ size_t nb_tbs, flush_full, flush_part, flush_elide;
+
+ tcg_tb_foreach(tb_tree_stats_iter, &tst);
+ nb_tbs = tst.nb_tbs;
+ /* XXX: avoid using doubles ? */
+ g_string_append_printf(buf, "Translation buffer state:\n");
+ /*
+ * Report total code size including the padding and TB structs;
+ * otherwise users might think "-accel tcg,tb-size" is not honoured.
+ * For avg host size we use the precise numbers from tb_tree_stats though.
+ */
+ g_string_append_printf(buf, "gen code size %zu/%zu\n",
+ tcg_code_size(), tcg_code_capacity());
+ g_string_append_printf(buf, "TB count %zu\n", nb_tbs);
+ g_string_append_printf(buf, "TB avg target size %zu max=%zu bytes\n",
+ nb_tbs ? tst.target_size / nb_tbs : 0,
+ tst.max_target_size);
+ g_string_append_printf(buf, "TB avg host size %zu bytes "
+ "(expansion ratio: %0.1f)\n",
+ nb_tbs ? tst.host_size / nb_tbs : 0,
+ tst.target_size ?
+ (double)tst.host_size / tst.target_size : 0);
+ g_string_append_printf(buf, "cross page TB count %zu (%zu%%)\n",
+ tst.cross_page,
+ nb_tbs ? (tst.cross_page * 100) / nb_tbs : 0);
+ g_string_append_printf(buf, "direct jump count %zu (%zu%%) "
+ "(2 jumps=%zu %zu%%)\n",
+ tst.direct_jmp_count,
+ nb_tbs ? (tst.direct_jmp_count * 100) / nb_tbs : 0,
+ tst.direct_jmp2_count,
+ nb_tbs ? (tst.direct_jmp2_count * 100) / nb_tbs : 0);
+
+ qht_statistics_init(&tb_ctx.htable, &hst);
+ print_qht_statistics(hst, buf);
+ qht_statistics_destroy(&hst);
+
+ g_string_append_printf(buf, "\nStatistics:\n");
+ g_string_append_printf(buf, "TB flush count %u\n",
+ qatomic_read(&tb_ctx.tb_flush_count));
+ g_string_append_printf(buf, "TB invalidate count %u\n",
+ qatomic_read(&tb_ctx.tb_phys_invalidate_count));
+
+ tlb_flush_counts(&flush_full, &flush_part, &flush_elide);
+ g_string_append_printf(buf, "TLB full flushes %zu\n", flush_full);
+ g_string_append_printf(buf, "TLB partial flushes %zu\n", flush_part);
+ g_string_append_printf(buf, "TLB elided flushes %zu\n", flush_elide);
+ tcg_dump_info(buf);
+}
+
HumanReadableText *qmp_x_query_jit(Error **errp)
{
g_autoptr(GString) buf = g_string_new("");
@@ -66,6 +215,11 @@ HumanReadableText *qmp_x_query_jit(Error **errp)
return human_readable_text_from_str(buf);
}
+static void tcg_dump_op_count(GString *buf)
+{
+ g_string_append_printf(buf, "[TCG profiler not compiled]\n");
+}
+
HumanReadableText *qmp_x_query_opcount(Error **errp)
{
g_autoptr(GString) buf = g_string_new("");
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 8cb6ad3511..e579b0891d 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -645,133 +645,6 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
cpu_loop_exit_noexc(cpu);
}
-static void print_qht_statistics(struct qht_stats hst, GString *buf)
-{
- uint32_t hgram_opts;
- size_t hgram_bins;
- char *hgram;
-
- if (!hst.head_buckets) {
- return;
- }
- g_string_append_printf(buf, "TB hash buckets %zu/%zu "
- "(%0.2f%% head buckets used)\n",
- hst.used_head_buckets, hst.head_buckets,
- (double)hst.used_head_buckets /
- hst.head_buckets * 100);
-
- hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
- hgram_opts |= QDIST_PR_100X | QDIST_PR_PERCENT;
- if (qdist_xmax(&hst.occupancy) - qdist_xmin(&hst.occupancy) == 1) {
- hgram_opts |= QDIST_PR_NODECIMAL;
- }
- hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
- g_string_append_printf(buf, "TB hash occupancy %0.2f%% avg chain occ. "
- "Histogram: %s\n",
- qdist_avg(&hst.occupancy) * 100, hgram);
- g_free(hgram);
-
- hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
- hgram_bins = qdist_xmax(&hst.chain) - qdist_xmin(&hst.chain);
- if (hgram_bins > 10) {
- hgram_bins = 10;
- } else {
- hgram_bins = 0;
- hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
- }
- hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
- g_string_append_printf(buf, "TB hash avg chain %0.3f buckets. "
- "Histogram: %s\n",
- qdist_avg(&hst.chain), hgram);
- g_free(hgram);
-}
-
-struct tb_tree_stats {
- size_t nb_tbs;
- size_t host_size;
- size_t target_size;
- size_t max_target_size;
- size_t direct_jmp_count;
- size_t direct_jmp2_count;
- size_t cross_page;
-};
-
-static gboolean tb_tree_stats_iter(gpointer key, gpointer value, gpointer data)
-{
- const TranslationBlock *tb = value;
- struct tb_tree_stats *tst = data;
-
- tst->nb_tbs++;
- tst->host_size += tb->tc.size;
- tst->target_size += tb->size;
- if (tb->size > tst->max_target_size) {
- tst->max_target_size = tb->size;
- }
- if (tb_page_addr1(tb) != -1) {
- tst->cross_page++;
- }
- if (tb->jmp_reset_offset[0] != TB_JMP_OFFSET_INVALID) {
- tst->direct_jmp_count++;
- if (tb->jmp_reset_offset[1] != TB_JMP_OFFSET_INVALID) {
- tst->direct_jmp2_count++;
- }
- }
- return false;
-}
-
-void dump_exec_info(GString *buf)
-{
- struct tb_tree_stats tst = {};
- struct qht_stats hst;
- size_t nb_tbs, flush_full, flush_part, flush_elide;
-
- tcg_tb_foreach(tb_tree_stats_iter, &tst);
- nb_tbs = tst.nb_tbs;
- /* XXX: avoid using doubles ? */
- g_string_append_printf(buf, "Translation buffer state:\n");
- /*
- * Report total code size including the padding and TB structs;
- * otherwise users might think "-accel tcg,tb-size" is not honoured.
- * For avg host size we use the precise numbers from tb_tree_stats though.
- */
- g_string_append_printf(buf, "gen code size %zu/%zu\n",
- tcg_code_size(), tcg_code_capacity());
- g_string_append_printf(buf, "TB count %zu\n", nb_tbs);
- g_string_append_printf(buf, "TB avg target size %zu max=%zu bytes\n",
- nb_tbs ? tst.target_size / nb_tbs : 0,
- tst.max_target_size);
- g_string_append_printf(buf, "TB avg host size %zu bytes "
- "(expansion ratio: %0.1f)\n",
- nb_tbs ? tst.host_size / nb_tbs : 0,
- tst.target_size ?
- (double)tst.host_size / tst.target_size : 0);
- g_string_append_printf(buf, "cross page TB count %zu (%zu%%)\n",
- tst.cross_page,
- nb_tbs ? (tst.cross_page * 100) / nb_tbs : 0);
- g_string_append_printf(buf, "direct jump count %zu (%zu%%) "
- "(2 jumps=%zu %zu%%)\n",
- tst.direct_jmp_count,
- nb_tbs ? (tst.direct_jmp_count * 100) / nb_tbs : 0,
- tst.direct_jmp2_count,
- nb_tbs ? (tst.direct_jmp2_count * 100) / nb_tbs : 0);
-
- qht_statistics_init(&tb_ctx.htable, &hst);
- print_qht_statistics(hst, buf);
- qht_statistics_destroy(&hst);
-
- g_string_append_printf(buf, "\nStatistics:\n");
- g_string_append_printf(buf, "TB flush count %u\n",
- qatomic_read(&tb_ctx.tb_flush_count));
- g_string_append_printf(buf, "TB invalidate count %u\n",
- qatomic_read(&tb_ctx.tb_phys_invalidate_count));
-
- tlb_flush_counts(&flush_full, &flush_part, &flush_elide);
- g_string_append_printf(buf, "TLB full flushes %zu\n", flush_full);
- g_string_append_printf(buf, "TLB partial flushes %zu\n", flush_part);
- g_string_append_printf(buf, "TLB elided flushes %zu\n", flush_elide);
- tcg_dump_info(buf);
-}
-
#else /* CONFIG_USER_ONLY */
void cpu_interrupt(CPUState *cpu, int mask)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index f664cf1484..71c25f1974 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -5919,11 +5919,6 @@ static void tcg_out_st_helper_args(TCGContext *s, const TCGLabelQemuLdst *ldst,
tcg_out_helper_load_common_args(s, ldst, parm, info, next_arg);
}
-void tcg_dump_op_count(GString *buf)
-{
- g_string_append_printf(buf, "[TCG profiler not compiled]\n");
-}
-
int tcg_gen_code(TCGContext *s, TranslationBlock *tb, uint64_t pc_start)
{
int i, start_words, num_insns;
@@ -6120,11 +6115,6 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb, uint64_t pc_start)
return tcg_current_code_size(s);
}
-void tcg_dump_info(GString *buf)
-{
- g_string_append_printf(buf, "[TCG profiler not compiled]\n");
-}
-
#ifdef ELF_HOST_MACHINE
/* In order to use this feature, the backend needs to do three things:
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v17 02/16] tcg: Record orig_nb_ops TCGContext
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
2023-10-03 18:30 ` [PATCH v17 01/16] accel/tcg: Move HMP info jit and info opcount code Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-15 12:57 ` Alex Bennée
2023-10-03 18:30 ` [PATCH v17 03/16] tcg: Record nb_deleted_ops in TCGContext Richard Henderson
` (14 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu
Remember the value of nb_ops before optimization.
To be copied into TBStatistics when desired.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/tcg/tcg.h | 3 +++
tcg/tcg.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 82b4625773..e49574b7c7 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -564,6 +564,9 @@ struct TCGContext {
uint16_t gen_insn_end_off[TCG_MAX_INSNS];
uint64_t *gen_insn_data;
+ /* Used by TBStatistics */
+ int orig_nb_ops;
+
/* Exit to translator on overflow. */
sigjmp_buf jmp_trans;
};
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 71c25f1974..e90b0a0c69 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -5924,6 +5924,8 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb, uint64_t pc_start)
int i, start_words, num_insns;
TCGOp *op;
+ s->orig_nb_ops = s->nb_ops;
+
if (unlikely(qemu_loglevel_mask(CPU_LOG_TB_OP)
&& qemu_log_in_addr_range(pc_start))) {
FILE *logfile = qemu_log_trylock();
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v17 03/16] tcg: Record nb_deleted_ops in TCGContext
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
2023-10-03 18:30 ` [PATCH v17 01/16] accel/tcg: Move HMP info jit and info opcount code Richard Henderson
2023-10-03 18:30 ` [PATCH v17 02/16] tcg: Record orig_nb_ops TCGContext Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-15 12:58 ` Alex Bennée
2023-10-03 18:30 ` [PATCH v17 04/16] tcg: Record nb_spills " Richard Henderson
` (13 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu
Record the number of ops that are removed during optimization.
To be copied into TBStatistics when desired.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/tcg/tcg.h | 1 +
tcg/tcg.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index e49574b7c7..d60349878f 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -566,6 +566,7 @@ struct TCGContext {
/* Used by TBStatistics */
int orig_nb_ops;
+ int nb_deleted_ops;
/* Exit to translator on overflow. */
sigjmp_buf jmp_trans;
diff --git a/tcg/tcg.c b/tcg/tcg.c
index e90b0a0c69..60be2f429c 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1494,6 +1494,7 @@ void tcg_func_start(TCGContext *s)
s->nb_ops = 0;
s->nb_labels = 0;
+ s->nb_deleted_ops = 0;
s->current_frame_offset = s->frame_start;
#ifdef CONFIG_DEBUG_TCG
@@ -3049,6 +3050,7 @@ void tcg_op_remove(TCGContext *s, TCGOp *op)
QTAILQ_REMOVE(&s->ops, op, link);
QTAILQ_INSERT_TAIL(&s->free_ops, op, link);
s->nb_ops--;
+ s->nb_deleted_ops++;
}
void tcg_remove_ops_after(TCGOp *op)
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v17 04/16] tcg: Record nb_spills in TCGContext
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (2 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 03/16] tcg: Record nb_deleted_ops in TCGContext Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-15 12:59 ` Alex Bennée
2023-10-03 18:30 ` [PATCH v17 05/16] accel/tcg: Add TBStatistics structure Richard Henderson
` (12 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu
Record the number of times a temporary is forced into memory
and the store would not have been required if there an infinite
number of call-saved cpu registers available. This excludes
stores that are required by semantics to return computed values
to their home slot in ENV, i.e. NEED_SYNC_ARG.
To be copied into TBStatistics when desired.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/tcg/tcg.h | 1 +
tcg/tcg.c | 36 +++++++++++++++++++++++-------------
2 files changed, 24 insertions(+), 13 deletions(-)
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index d60349878f..c2b1a2e187 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -567,6 +567,7 @@ struct TCGContext {
/* Used by TBStatistics */
int orig_nb_ops;
int nb_deleted_ops;
+ int nb_spills;
/* Exit to translator on overflow. */
sigjmp_buf jmp_trans;
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 60be2f429c..471e5eaad9 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1495,6 +1495,7 @@ void tcg_func_start(TCGContext *s)
s->nb_ops = 0;
s->nb_labels = 0;
s->nb_deleted_ops = 0;
+ s->nb_spills = 0;
s->current_frame_offset = s->frame_start;
#ifdef CONFIG_DEBUG_TCG
@@ -4118,8 +4119,11 @@ static inline void temp_dead(TCGContext *s, TCGTemp *ts)
is non-zero, subsequently release the temporary; if it is positive, the
temp is dead; if it is negative, the temp is free. */
static void temp_sync(TCGContext *s, TCGTemp *ts, TCGRegSet allocated_regs,
- TCGRegSet preferred_regs, int free_or_dead)
+ TCGRegSet preferred_regs, int free_or_dead,
+ bool account_spill)
{
+ bool did_spill = false;
+
if (!temp_readonly(ts) && !ts->mem_coherent) {
if (!ts->mem_allocated) {
temp_allocate_frame(s, ts);
@@ -4132,6 +4136,7 @@ static void temp_sync(TCGContext *s, TCGTemp *ts, TCGRegSet allocated_regs,
if (free_or_dead
&& tcg_out_sti(s, ts->type, ts->val,
ts->mem_base->reg, ts->mem_offset)) {
+ did_spill = account_spill;
break;
}
temp_load(s, ts, tcg_target_available_regs[ts->type],
@@ -4139,6 +4144,7 @@ static void temp_sync(TCGContext *s, TCGTemp *ts, TCGRegSet allocated_regs,
/* fallthrough */
case TEMP_VAL_REG:
+ did_spill = account_spill;
tcg_out_st(s, ts->type, ts->reg,
ts->mem_base->reg, ts->mem_offset);
break;
@@ -4155,6 +4161,9 @@ static void temp_sync(TCGContext *s, TCGTemp *ts, TCGRegSet allocated_regs,
if (free_or_dead) {
temp_free_or_dead(s, ts, free_or_dead);
}
+ if (did_spill) {
+ s->nb_spills += 1;
+ }
}
/* free register 'reg' by spilling the corresponding temporary if necessary */
@@ -4162,7 +4171,7 @@ static void tcg_reg_free(TCGContext *s, TCGReg reg, TCGRegSet allocated_regs)
{
TCGTemp *ts = s->reg_to_temp[reg];
if (ts != NULL) {
- temp_sync(s, ts, allocated_regs, 0, -1);
+ temp_sync(s, ts, allocated_regs, 0, -1, true);
}
}
@@ -4442,7 +4451,8 @@ static void tcg_reg_alloc_do_movi(TCGContext *s, TCGTemp *ots,
ots->val = val;
ots->mem_coherent = 0;
if (NEED_SYNC_ARG(0)) {
- temp_sync(s, ots, s->reserved_regs, preferred_regs, IS_DEAD_ARG(0));
+ temp_sync(s, ots, s->reserved_regs, preferred_regs,
+ IS_DEAD_ARG(0), false);
} else if (IS_DEAD_ARG(0)) {
temp_dead(s, ots);
}
@@ -4544,7 +4554,7 @@ static void tcg_reg_alloc_mov(TCGContext *s, const TCGOp *op)
ots->mem_coherent = 0;
if (NEED_SYNC_ARG(0)) {
- temp_sync(s, ots, allocated_regs, 0, 0);
+ temp_sync(s, ots, allocated_regs, 0, 0, false);
}
}
@@ -4621,7 +4631,7 @@ static void tcg_reg_alloc_dup(TCGContext *s, const TCGOp *op)
break;
}
/* Sync the temp back to its slot and load from there. */
- temp_sync(s, its, s->reserved_regs, 0, 0);
+ temp_sync(s, its, s->reserved_regs, 0, 0, true);
}
/* fall through */
@@ -4652,7 +4662,7 @@ static void tcg_reg_alloc_dup(TCGContext *s, const TCGOp *op)
temp_dead(s, its);
}
if (NEED_SYNC_ARG(0)) {
- temp_sync(s, ots, s->reserved_regs, 0, 0);
+ temp_sync(s, ots, s->reserved_regs, 0, 0, false);
}
if (IS_DEAD_ARG(0)) {
temp_dead(s, ots);
@@ -4870,7 +4880,7 @@ static void tcg_reg_alloc_op(TCGContext *s, const TCGOp *op)
* Cross register class move not supported. Sync the
* temp back to its slot and load from there.
*/
- temp_sync(s, ts, i_allocated_regs, 0, 0);
+ temp_sync(s, ts, i_allocated_regs, 0, 0, true);
tcg_out_ld(s, ts->type, reg,
ts->mem_base->reg, ts->mem_offset);
}
@@ -5019,7 +5029,7 @@ static void tcg_reg_alloc_op(TCGContext *s, const TCGOp *op)
tcg_debug_assert(!temp_readonly(ts));
if (NEED_SYNC_ARG(i)) {
- temp_sync(s, ts, o_allocated_regs, 0, IS_DEAD_ARG(i));
+ temp_sync(s, ts, o_allocated_regs, 0, IS_DEAD_ARG(i), false);
} else if (IS_DEAD_ARG(i)) {
temp_dead(s, ts);
}
@@ -5086,8 +5096,8 @@ static bool tcg_reg_alloc_dup2(TCGContext *s, const TCGOp *op)
itsl == itsh + (HOST_BIG_ENDIAN ? 1 : -1)) {
TCGTemp *its = itsl - HOST_BIG_ENDIAN;
- temp_sync(s, its + 0, s->reserved_regs, 0, 0);
- temp_sync(s, its + 1, s->reserved_regs, 0, 0);
+ temp_sync(s, its + 0, s->reserved_regs, 0, 0, true);
+ temp_sync(s, its + 1, s->reserved_regs, 0, 0, true);
if (tcg_out_dupm_vec(s, vtype, MO_64, ots->reg,
its->mem_base->reg, its->mem_offset)) {
@@ -5107,7 +5117,7 @@ static bool tcg_reg_alloc_dup2(TCGContext *s, const TCGOp *op)
temp_dead(s, itsh);
}
if (NEED_SYNC_ARG(0)) {
- temp_sync(s, ots, s->reserved_regs, 0, IS_DEAD_ARG(0));
+ temp_sync(s, ots, s->reserved_regs, 0, IS_DEAD_ARG(0), false);
} else if (IS_DEAD_ARG(0)) {
temp_dead(s, ots);
}
@@ -5125,7 +5135,7 @@ static void load_arg_reg(TCGContext *s, TCGReg reg, TCGTemp *ts,
* Cross register class move not supported. Sync the
* temp back to its slot and load from there.
*/
- temp_sync(s, ts, allocated_regs, 0, 0);
+ temp_sync(s, ts, allocated_regs, 0, 0, true);
tcg_out_ld(s, ts->type, reg,
ts->mem_base->reg, ts->mem_offset);
}
@@ -5307,7 +5317,7 @@ static void tcg_reg_alloc_call(TCGContext *s, TCGOp *op)
for (i = 0; i < nb_oargs; i++) {
TCGTemp *ts = arg_temp(op->args[i]);
if (NEED_SYNC_ARG(i)) {
- temp_sync(s, ts, s->reserved_regs, 0, IS_DEAD_ARG(i));
+ temp_sync(s, ts, s->reserved_regs, 0, IS_DEAD_ARG(i), false);
} else if (IS_DEAD_ARG(i)) {
temp_dead(s, ts);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 04/16] tcg: Record nb_spills in TCGContext
2023-10-03 18:30 ` [PATCH v17 04/16] tcg: Record nb_spills " Richard Henderson
@ 2023-10-15 12:59 ` Alex Bennée
0 siblings, 0 replies; 39+ messages in thread
From: Alex Bennée @ 2023-10-15 12:59 UTC (permalink / raw)
To: Richard Henderson; +Cc: fei2.wu, qemu-devel
Richard Henderson <richard.henderson@linaro.org> writes:
> Record the number of times a temporary is forced into memory
> and the store would not have been required if there an infinite
> number of call-saved cpu registers available. This excludes
> stores that are required by semantics to return computed values
> to their home slot in ENV, i.e. NEED_SYNC_ARG.
>
> To be copied into TBStatistics when desired.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 05/16] accel/tcg: Add TBStatistics structure
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (3 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 04/16] tcg: Record nb_spills " Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-16 14:38 ` Alex Bennée
2023-10-03 18:30 ` [PATCH v17 06/16] accel/tcg: Collect TB execution statistics Richard Henderson
` (11 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu, Vanderson M . do Rosario
Add code to allocate, reset, and free the structures along
with their corresponding TranslationBlocks. We do not yet
collect, display, or enable the statistics.
Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
[rth: Significantly reorganized.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/tb-context.h | 2 +-
include/exec/translation-block.h | 3 +
include/qemu/typedefs.h | 1 +
include/tcg/tb-stats.h | 132 +++++++++++++++++++++++++++++++
accel/tcg/tb-maint.c | 3 +-
accel/tcg/tb-stats.c | 85 ++++++++++++++++++++
accel/tcg/translate-all.c | 19 +++++
accel/tcg/meson.build | 1 +
8 files changed, 244 insertions(+), 2 deletions(-)
create mode 100644 include/tcg/tb-stats.h
create mode 100644 accel/tcg/tb-stats.c
diff --git a/accel/tcg/tb-context.h b/accel/tcg/tb-context.h
index cac62d9749..4b1abe392b 100644
--- a/accel/tcg/tb-context.h
+++ b/accel/tcg/tb-context.h
@@ -29,8 +29,8 @@
typedef struct TBContext TBContext;
struct TBContext {
-
struct qht htable;
+ struct qht stats;
/* statistics */
unsigned tb_flush_count;
diff --git a/include/exec/translation-block.h b/include/exec/translation-block.h
index b785751774..4206a72600 100644
--- a/include/exec/translation-block.h
+++ b/include/exec/translation-block.h
@@ -141,6 +141,9 @@ struct TranslationBlock {
uintptr_t jmp_list_head;
uintptr_t jmp_list_next[2];
uintptr_t jmp_dest[2];
+
+ /* Pointer to a struct where statistics from the TB is stored */
+ TBStatistics *tb_stats;
};
/* The alignment given to TranslationBlock during allocation. */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 5abdbc3874..68011da95b 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -131,6 +131,7 @@ typedef struct Range Range;
typedef struct ReservedRegion ReservedRegion;
typedef struct SHPCDevice SHPCDevice;
typedef struct SSIBus SSIBus;
+typedef struct TBStatistics TBStatistics;
typedef struct TCGHelperInfo TCGHelperInfo;
typedef struct TranslationBlock TranslationBlock;
typedef struct VirtIODevice VirtIODevice;
diff --git a/include/tcg/tb-stats.h b/include/tcg/tb-stats.h
new file mode 100644
index 0000000000..1ec0d13eff
--- /dev/null
+++ b/include/tcg/tb-stats.h
@@ -0,0 +1,132 @@
+/*
+ * QEMU System Emulator, Code Quality Monitor System
+ *
+ * Copyright (c) 2019 Vanderson M. do Rosario <vandersonmr2@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef TCG_TB_STATS_H
+#define TCG_TB_STATS_H 1
+
+#include "qemu/thread.h"
+#include "exec/translation-block.h"
+
+enum {
+ TB_STATS_EXEC = 1u << 0,
+ TB_STATS_JIT = 1u << 1,
+
+ TB_STATS_NONE = 0,
+ TB_STATS_ALL = TB_STATS_EXEC | TB_STATS_JIT,
+};
+
+extern uint32_t tb_stats_enabled;
+
+/**
+ * tb_stats_init:
+ * @flags: TB_STATS_* flags to enable.
+ *
+ * Initialize translation block statistics, enabling @flags.
+ * If @flags is 0, disable all statistics.
+ */
+void tb_stats_init(uint32_t flags);
+
+/*
+ * This struct stores statistics such as execution count of the
+ * TranslationBlocks. Each sets of TBs for a given phys_pc/pc/flags
+ * has its own TBStatistics which will persist over tb_flush.
+ *
+ * We include additional counters to track number of translations as
+ * well as variants for compile flags.
+ */
+struct TBStatistics {
+ tb_page_addr_t phys_pc;
+ vaddr pc;
+ uint32_t flags;
+ uint64_t flags2;
+
+ /* Execution stats */
+ struct {
+ unsigned long normal;
+ unsigned long atomic;
+ /* filled only when dumping x% cover set */
+ double coverage;
+ } executions;
+
+ /* JIT Stats - protected by lock */
+ QemuMutex jit_stats_lock;
+
+ /* Sum of all operations for all translations */
+ struct {
+ unsigned long num_guest_inst;
+ unsigned long num_tcg_ops;
+ unsigned long num_tcg_ops_opt;
+ unsigned long spills;
+
+ unsigned long temps;
+ unsigned long deleted_ops;
+ unsigned long in_len;
+ unsigned long out_len;
+ unsigned long search_out_len;
+ } code;
+
+ struct {
+ unsigned long total;
+ unsigned long spanning;
+ } translations;
+
+ /*
+ * All persistent (cached) TranslationBlocks using
+ * this TBStats structure. Has to be reset on a tb_flush.
+ */
+ GPtrArray *tbs;
+};
+
+/**
+ * tb_stats_enabled:
+ * @tb: TranslationBlock
+ * @f: flag to check
+ *
+ * Return true if any stats are enabled for @tb and
+ * if @f is enabled globally.
+ */
+static inline bool tb_stats_enabled_for_tb(TranslationBlock *tb, uint32_t f)
+{
+ return unlikely(tb_stats_enabled & f) && tb->tb_stats;
+}
+
+/**
+ * tb_stats_reset_tbs: reset the linked array of TBs
+ *
+ * Reset the list of tbs for a given array. Should be called from
+ * safe work during tb_flush.
+ */
+void tb_stats_reset_tbs(void);
+
+/**
+ * tb_stats_lookup:
+ *
+ * If any tb_stats are enabled, return a new or existing struct
+ * for the tuple (phys_pc, pc, flags, flags2). To be used when
+ * building a new TranslationBlock.
+ */
+TBStatistics *tb_stats_lookup(tb_page_addr_t phys_pc, vaddr pc,
+ uint32_t flags, uint64_t flags2);
+
+#endif /* TCG_TB_STATS_H */
diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index e678d20dc2..9025459fb1 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -27,6 +27,7 @@
#include "exec/translate-all.h"
#include "sysemu/tcg.h"
#include "tcg/tcg.h"
+#include "tcg/tb-stats.h"
#include "tb-hash.h"
#include "tb-context.h"
#include "internal-common.h"
@@ -772,7 +773,7 @@ static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
qht_reset_size(&tb_ctx.htable, CODE_GEN_HTABLE_SIZE);
tb_remove_all();
-
+ tb_stats_reset_tbs();
tcg_region_reset_all();
/* XXX: flush processor icache at this point if cache flush is expensive */
qatomic_inc(&tb_ctx.tb_flush_count);
diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
new file mode 100644
index 0000000000..424c9a90ec
--- /dev/null
+++ b/accel/tcg/tb-stats.c
@@ -0,0 +1,85 @@
+/*
+ * QEMU System Emulator, Code Quality Monitor System
+ *
+ * Copyright (c) 2019 Vanderson M. do Rosario <vandersonmr2@gmail.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/xxhash.h"
+#include "tcg/tb-stats.h"
+#include "tb-context.h"
+
+uint32_t tb_stats_enabled;
+
+static bool tb_stats_cmp(const void *ap, const void *bp)
+{
+ const TBStatistics *a = ap;
+ const TBStatistics *b = bp;
+
+ return a->phys_pc == b->phys_pc &&
+ a->pc == b->pc &&
+ a->flags == b->flags &&
+ a->flags2 == b->flags2;
+
+}
+
+static void tb_stats_free(void *p, uint32_t hash, void *userp)
+{
+ TBStatistics *s = p;
+
+ qemu_mutex_destroy(&s->jit_stats_lock);
+ g_ptr_array_free(s->tbs, true);
+ g_free(s);
+}
+
+void tb_stats_init(uint32_t flags)
+{
+ tb_stats_enabled = flags;
+ if (flags) {
+ if (!tb_ctx.stats.map) {
+ qht_init(&tb_ctx.stats, tb_stats_cmp,
+ CODE_GEN_HTABLE_SIZE, QHT_MODE_AUTO_RESIZE);
+ }
+ } else {
+ qht_iter(&tb_ctx.stats, tb_stats_free, NULL);
+ qht_destroy(&tb_ctx.stats);
+ }
+}
+
+static void tb_stats_reset(void *p, uint32_t hash, void *userp)
+{
+ TBStatistics *s = p;
+ g_ptr_array_set_size(s->tbs, 0);
+}
+
+void tb_stats_reset_tbs(void)
+{
+ if (tb_ctx.stats.map) {
+ qht_iter(&tb_ctx.stats, tb_stats_reset, NULL);
+ }
+}
+
+TBStatistics *tb_stats_lookup(tb_page_addr_t phys_pc, vaddr pc,
+ uint32_t flags, uint64_t flags2)
+{
+ TBStatistics *s;
+ uint32_t h;
+ void *existing;
+
+ s = g_new0(TBStatistics, 1);
+ s->phys_pc = phys_pc;
+ s->pc = pc;
+ s->flags = flags;
+ s->flags2 = flags2;
+ s->tbs = g_ptr_array_new();
+ qemu_mutex_init(&s->jit_stats_lock);
+
+ h = qemu_xxhash7(phys_pc, pc, flags2, flags);
+ if (!qht_insert(&tb_ctx.stats, s, h, &existing)) {
+ tb_stats_free(s, 0, NULL);
+ return existing;
+ }
+ return s;
+}
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index e579b0891d..6e64ae2dbe 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -65,6 +65,7 @@
#include "internal-target.h"
#include "perf.h"
#include "tcg/insn-start-words.h"
+#include "tcg/tb-stats.h"
TBContext tb_ctx;
@@ -353,6 +354,24 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
tcg_ctx->guest_mo = TCG_MO_ALL;
#endif
+ /*
+ * Insert the TB into the corresponding stats structure, if required.
+ * Do this before code generation so that translator_loop can see
+ * the structure address.
+ */
+ tb->tb_stats = NULL;
+ if (unlikely(tb_stats_enabled) && qemu_log_in_addr_range(pc)) {
+ TBStatistics *s = tb_stats_lookup(phys_pc,
+ cflags & CF_PCREL ? 0 : pc,
+ flags, cs_base);
+ if (s) {
+ tb->tb_stats = s;
+ qemu_mutex_lock(&s->jit_stats_lock);
+ g_ptr_array_add(s->tbs, tb);
+ qemu_mutex_unlock(&s->jit_stats_lock);
+ }
+ }
+
restart_translate:
trace_translate_block(tb, pc, tb->tc.ptr);
diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
index 8783edd06e..34312f7a8b 100644
--- a/accel/tcg/meson.build
+++ b/accel/tcg/meson.build
@@ -6,6 +6,7 @@ tcg_ss.add(files(
'tcg-all.c',
'cpu-exec.c',
'tb-maint.c',
+ 'tb-stats.c',
'tcg-runtime-gvec.c',
'tcg-runtime.c',
'translate-all.c',
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 05/16] accel/tcg: Add TBStatistics structure
2023-10-03 18:30 ` [PATCH v17 05/16] accel/tcg: Add TBStatistics structure Richard Henderson
@ 2023-10-16 14:38 ` Alex Bennée
0 siblings, 0 replies; 39+ messages in thread
From: Alex Bennée @ 2023-10-16 14:38 UTC (permalink / raw)
To: Richard Henderson; +Cc: fei2.wu, Vanderson M . do Rosario, qemu-devel
Richard Henderson <richard.henderson@linaro.org> writes:
> Add code to allocate, reset, and free the structures along
> with their corresponding TranslationBlocks. We do not yet
> collect, display, or enable the statistics.
>
> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> [rth: Significantly reorganized.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 06/16] accel/tcg: Collect TB execution statistics
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (4 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 05/16] accel/tcg: Add TBStatistics structure Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-03 18:30 ` [PATCH v17 07/16] accel/tcg: Collect TB jit statistics Richard Henderson
` (10 subsequent siblings)
16 siblings, 0 replies; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu, Vanderson M. do Rosario, Alex Bennée
From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
Collect atomic and normal execution counts for TBs which
have allocated a stats structure.
Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/cpu-exec.c | 6 ++++++
accel/tcg/translator.c | 28 ++++++++++++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 1a5bc90220..1114eae5c4 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -26,6 +26,7 @@
#include "disas/disas.h"
#include "exec/exec-all.h"
#include "tcg/tcg.h"
+#include "tcg/tb-stats.h"
#include "qemu/atomic.h"
#include "qemu/rcu.h"
#include "exec/log.h"
@@ -601,6 +602,11 @@ void cpu_exec_step_atomic(CPUState *cpu)
}
cpu_exec_enter(cpu);
+
+ if (tb_stats_enabled_for_tb(tb, TB_STATS_EXEC)) {
+ tb->tb_stats->executions.atomic++;
+ }
+
/* execute the generated code */
trace_exec_tb(tb, pc);
cpu_tb_exec(cpu, tb, &tb_exit);
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index e7abcd86c1..eac281b229 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -14,6 +14,8 @@
#include "exec/translator.h"
#include "exec/plugin-gen.h"
#include "tcg/tcg-op-common.h"
+#include "tcg/tcg-temp-internal.h"
+#include "tcg/tb-stats.h"
#include "internal-target.h"
static void set_can_do_io(DisasContextBase *db, bool val)
@@ -112,6 +114,31 @@ static void gen_tb_end(const TranslationBlock *tb, uint32_t cflags,
}
}
+static void gen_tb_exec_count(TranslationBlock *tb)
+{
+ if (tb_stats_enabled_for_tb(tb, TB_STATS_EXEC)) {
+ TCGv_ptr ptr = tcg_temp_ebb_new_ptr();
+
+ tcg_gen_movi_ptr(ptr, (intptr_t)&tb->tb_stats->executions.normal);
+ if (sizeof(tb->tb_stats->executions.normal) == 4) {
+ TCGv_i32 t = tcg_temp_ebb_new_i32();
+ tcg_gen_ld_i32(t, ptr, 0);
+ tcg_gen_addi_i32(t, t, 1);
+ tcg_gen_st_i32(t, ptr, 0);
+ tcg_temp_free_i32(t);
+ } else if (sizeof(tb->tb_stats->executions.normal) == 8) {
+ TCGv_i64 t = tcg_temp_ebb_new_i64();
+ tcg_gen_ld_i64(t, ptr, 0);
+ tcg_gen_addi_i64(t, t, 1);
+ tcg_gen_st_i64(t, ptr, 0);
+ tcg_temp_free_i64(t);
+ } else {
+ qemu_build_not_reached_always();
+ }
+ tcg_temp_free_ptr(ptr);
+ }
+}
+
bool translator_use_goto_tb(DisasContextBase *db, vaddr dest)
{
/* Suppress goto_tb if requested. */
@@ -148,6 +175,7 @@ void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
/* Start translating. */
icount_start_insn = gen_tb_start(db, cflags);
+ gen_tb_exec_count(tb);
ops->tb_start(db, cpu);
tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v17 07/16] accel/tcg: Collect TB jit statistics
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (5 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 06/16] accel/tcg: Collect TB execution statistics Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-10 12:13 ` Philippe Mathieu-Daudé
2023-10-03 18:30 ` [PATCH v17 08/16] accel/tcg: Add tb_stats hmp command Richard Henderson
` (9 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu, Vanderson M . do Rosario, Alex Bennée
Collect items like input and output code size, guest instruction count,
intermediate ops, spills, etc.
Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
[rth: Consolidated at the end of translation.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/translate-all.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 6e64ae2dbe..ad4538f169 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -557,6 +557,27 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
return tb;
}
+ /* Record JIT statistics, if required. */
+ if (unlikely(tb_stats_enabled & TB_STATS_JIT)) {
+ TBStatistics *s = tb->tb_stats;
+ if (s) {
+ s->code.num_tcg_ops += tcg_ctx->orig_nb_ops;
+ s->code.num_tcg_ops_opt += tcg_ctx->nb_ops;
+ s->code.temps += tcg_ctx->nb_temps;
+ s->code.deleted_ops += tcg_ctx->nb_deleted_ops;
+ s->code.spills += tcg_ctx->nb_spills;
+
+ s->code.num_guest_inst += tb->icount;
+ s->code.in_len += tb->size;
+ s->code.out_len += tb->tc.size;
+ s->code.search_out_len += search_size;
+ s->translations.total += 1;
+ if (tb_page_addr1(tb) != -1) {
+ s->translations.spanning += 1;
+ }
+ }
+ }
+
/*
* Insert TB into the corresponding region tree before publishing it
* through QHT. Otherwise rewinding happened in the TB might fail to
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 07/16] accel/tcg: Collect TB jit statistics
2023-10-03 18:30 ` [PATCH v17 07/16] accel/tcg: Collect TB jit statistics Richard Henderson
@ 2023-10-10 12:13 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-10 12:13 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: fei2.wu, Vanderson M . do Rosario, Alex Bennée
On 3/10/23 20:30, Richard Henderson wrote:
> Collect items like input and output code size, guest instruction count,
> intermediate ops, spills, etc.
>
> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> [rth: Consolidated at the end of translation.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> accel/tcg/translate-all.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 08/16] accel/tcg: Add tb_stats hmp command
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (6 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 07/16] accel/tcg: Collect TB jit statistics Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-03 18:30 ` [PATCH v17 09/16] util/log: Add Error argument to qemu_str_to_log_mask Richard Henderson
` (8 subsequent siblings)
16 siblings, 0 replies; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel
Cc: fei2.wu, Vanderson M. do Rosario, Dr . David Alan Gilbert,
Alex Bennée
From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
Add tb_stats [start|stop|status] command to hmp.
This allows controlling the collection of statistics.
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 considered worthwhile.
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
[rth: Significantly reorganized.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/monitor.c | 70 ++++++++++++++++++++++++++++++++++++++++++++-
hmp-commands.hx | 15 ++++++++++
2 files changed, 84 insertions(+), 1 deletion(-)
diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
index 093efe9714..370fea883c 100644
--- a/accel/tcg/monitor.c
+++ b/accel/tcg/monitor.c
@@ -12,11 +12,15 @@
#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"
#include "tcg/tcg.h"
+#include "tcg/tb-stats.h"
+#include "exec/tb-flush.h"
#include "internal-common.h"
#include "tb-context.h"
@@ -235,10 +239,74 @@ HumanReadableText *qmp_x_query_opcount(Error **errp)
return human_readable_text_from_str(buf);
}
+static void tb_stats_init_safe(CPUState *cpu, run_on_cpu_data icmd)
+{
+ uint32_t flags = icmd.host_int;
+
+ tb_stats_init(flags);
+ tb_flush(cpu);
+}
+
+static void hmp_tbstats(Monitor *mon, const QDict *qdict)
+{
+ uint32_t flags = TB_STATS_NONE;
+ const char *cmd;
+
+ if (!tcg_enabled()) {
+ monitor_printf(mon, "Only available with accel=tcg\n");
+ return;
+ }
+
+ cmd = qdict_get_try_str(qdict, "command");
+
+ if (strcmp(cmd, "start") == 0) {
+ const char *sflag = qdict_get_try_str(qdict, "flag");
+
+ flags = TB_STATS_ALL;
+ if (sflag) {
+ if (strcmp(sflag, "all") == 0) {
+ flags = TB_STATS_ALL;
+ } else if (strcmp(sflag, "jit") == 0) {
+ flags = TB_STATS_JIT;
+ } else if (strcmp(sflag, "exec") == 0) {
+ flags = TB_STATS_EXEC;
+ } else {
+ monitor_printf(mon, "Invalid argument to tb_stats start\n");
+ return;
+ }
+ }
+
+ if (tb_stats_enabled) {
+ monitor_printf(mon, "TB statistics already being recorded\n");
+ return;
+ }
+ } else if (strcmp(cmd, "stop") == 0) {
+ if (!tb_stats_enabled) {
+ monitor_printf(mon, "TB statistics not being recorded\n");
+ return;
+ }
+ } else if (strcmp(cmd, "status") == 0) {
+ if (tb_stats_enabled) {
+ monitor_printf(mon, "TB statistics are enabled:%s%s\n",
+ tb_stats_enabled & TB_STATS_EXEC ? " EXEC" : "",
+ tb_stats_enabled & TB_STATS_JIT ? " JIT" : "");
+ } else {
+ monitor_printf(mon, "TB statistics are disabled\n");
+ }
+ return;
+ } else {
+ monitor_printf(mon, "Invalid command\n");
+ return;
+ }
+
+ async_safe_run_on_cpu(first_cpu, tb_stats_init_safe,
+ RUN_ON_CPU_HOST_INT(flags));
+}
+
static void hmp_tcg_register(void)
{
monitor_register_hmp_info_hrt("jit", qmp_x_query_jit);
monitor_register_hmp_info_hrt("opcount", qmp_x_query_opcount);
+ monitor_register_hmp("tb_stats", false, hmp_tbstats);
}
-
type_init(hmp_tcg_register);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 63eac22734..e1d78ab69d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1673,6 +1673,21 @@ SRST
Executes a qemu-io command on the given block device.
ERST
+#if defined(CONFIG_TCG)
+ {
+ .name = "tb_stats",
+ .args_type = "command:s,flag:s?",
+ .params = "command [flag]",
+ .help = "Control tb statistics collection:"
+ "tb_stats (start|stop|status) [all|jit|exec]",
+ },
+#endif
+
+SRST
+``tb_stats`` *command* *flag*
+ Control recording tb statistics
+ERST
+
{
.name = "qom-list",
.args_type = "path:s?",
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v17 09/16] util/log: Add Error argument to qemu_str_to_log_mask
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (7 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 08/16] accel/tcg: Add tb_stats hmp command Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-10 12:55 ` Markus Armbruster
2023-10-03 18:30 ` [PATCH v17 10/16] util/log: Add -d tb_stats Richard Henderson
` (7 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu
Do not rely on return value of 0 to indicate error,
pass along an Error pointer to be set.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/qemu/log.h | 2 +-
bsd-user/main.c | 6 ++++--
linux-user/main.c | 7 +++++--
monitor/hmp-cmds.c | 5 +++--
softmmu/vl.c | 7 +++++--
util/log.c | 5 +++--
6 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/include/qemu/log.h b/include/qemu/log.h
index df59bfabcd..9b92d2663e 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -87,7 +87,7 @@ bool qemu_set_log_filename(const char *filename, Error **errp);
bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp);
void qemu_set_dfilter_ranges(const char *ranges, Error **errp);
bool qemu_log_in_addr_range(uint64_t addr);
-int qemu_str_to_log_mask(const char *str);
+int qemu_str_to_log_mask(const char *str, Error **errp);
/* Print a usage message listing all the valid logging categories
* to the specified FILE*.
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 703f3e2c41..a981239a0b 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -411,8 +411,10 @@ int main(int argc, char **argv)
{
int mask = 0;
if (log_mask) {
- mask = qemu_str_to_log_mask(log_mask);
- if (!mask) {
+ Error *err = NULL;
+ mask = qemu_str_to_log_mask(log_mask, &err);
+ if (err) {
+ error_report_err(err);
qemu_print_log_usage(stdout);
exit(1);
}
diff --git a/linux-user/main.c b/linux-user/main.c
index 0c23584a96..d0464736cc 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -264,8 +264,11 @@ static void handle_arg_help(const char *arg)
static void handle_arg_log(const char *arg)
{
- last_log_mask = qemu_str_to_log_mask(arg);
- if (!last_log_mask) {
+ Error *err = NULL;
+
+ last_log_mask = qemu_str_to_log_mask(arg, &err);
+ if (err) {
+ error_report_err(err);
qemu_print_log_usage(stdout);
exit(EXIT_FAILURE);
}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 6c559b48c8..c4bd97d467 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -291,8 +291,9 @@ void hmp_log(Monitor *mon, const QDict *qdict)
if (!strcmp(items, "none")) {
mask = 0;
} else {
- mask = qemu_str_to_log_mask(items);
- if (!mask) {
+ mask = qemu_str_to_log_mask(items, &err);
+ if (err) {
+ error_free(err);
hmp_help_cmd(mon, "log");
return;
}
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 98e071e63b..02193696b9 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2486,8 +2486,11 @@ static void qemu_process_early_options(void)
{
int mask = 0;
if (log_mask) {
- mask = qemu_str_to_log_mask(log_mask);
- if (!mask) {
+ Error *err = NULL;
+
+ mask = qemu_str_to_log_mask(log_mask, &err);
+ if (err) {
+ error_report_err(err);
qemu_print_log_usage(stdout);
exit(1);
}
diff --git a/util/log.c b/util/log.c
index def88a9402..b5f08db202 100644
--- a/util/log.c
+++ b/util/log.c
@@ -500,8 +500,8 @@ const QEMULogItem qemu_log_items[] = {
{ 0, NULL, NULL },
};
-/* takes a comma separated list of log masks. Return 0 if error. */
-int qemu_str_to_log_mask(const char *str)
+/* Takes a comma separated list of log masks. */
+int qemu_str_to_log_mask(const char *str, Error **errp)
{
const QEMULogItem *item;
int mask = 0;
@@ -524,6 +524,7 @@ int qemu_str_to_log_mask(const char *str)
goto found;
}
}
+ error_setg(errp, "Invalid -d option \"%s\"", *tmp);
goto error;
found:
mask |= item->mask;
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 09/16] util/log: Add Error argument to qemu_str_to_log_mask
2023-10-03 18:30 ` [PATCH v17 09/16] util/log: Add Error argument to qemu_str_to_log_mask Richard Henderson
@ 2023-10-10 12:55 ` Markus Armbruster
2023-10-15 18:55 ` Richard Henderson
0 siblings, 1 reply; 39+ messages in thread
From: Markus Armbruster @ 2023-10-10 12:55 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, fei2.wu, Philippe Mathieu-Daudé
Richard Henderson <richard.henderson@linaro.org> writes:
> Do not rely on return value of 0 to indicate error,
> pass along an Error pointer to be set.
Not wrong, but goes against error.h's recommendation
* - Whenever practical, also return a value that indicates success /
* failure. This can make the error checking more concise, and can
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> include/qemu/log.h | 2 +-
> bsd-user/main.c | 6 ++++--
> linux-user/main.c | 7 +++++--
> monitor/hmp-cmds.c | 5 +++--
> softmmu/vl.c | 7 +++++--
> util/log.c | 5 +++--
> 6 files changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index df59bfabcd..9b92d2663e 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -87,7 +87,7 @@ bool qemu_set_log_filename(const char *filename, Error **errp);
> bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp);
> void qemu_set_dfilter_ranges(const char *ranges, Error **errp);
> bool qemu_log_in_addr_range(uint64_t addr);
> -int qemu_str_to_log_mask(const char *str);
> +int qemu_str_to_log_mask(const char *str, Error **errp);
>
> /* Print a usage message listing all the valid logging categories
> * to the specified FILE*.
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 703f3e2c41..a981239a0b 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -411,8 +411,10 @@ int main(int argc, char **argv)
> {
> int mask = 0;
> if (log_mask) {
> - mask = qemu_str_to_log_mask(log_mask);
> - if (!mask) {
> + Error *err = NULL;
> + mask = qemu_str_to_log_mask(log_mask, &err);
> + if (err) {
> + error_report_err(err);
> qemu_print_log_usage(stdout);
> exit(1);
> }
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 0c23584a96..d0464736cc 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -264,8 +264,11 @@ static void handle_arg_help(const char *arg)
>
> static void handle_arg_log(const char *arg)
> {
> - last_log_mask = qemu_str_to_log_mask(arg);
> - if (!last_log_mask) {
> + Error *err = NULL;
> +
> + last_log_mask = qemu_str_to_log_mask(arg, &err);
> + if (err) {
> + error_report_err(err);
> qemu_print_log_usage(stdout);
> exit(EXIT_FAILURE);
> }
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 6c559b48c8..c4bd97d467 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -291,8 +291,9 @@ void hmp_log(Monitor *mon, const QDict *qdict)
> if (!strcmp(items, "none")) {
> mask = 0;
> } else {
> - mask = qemu_str_to_log_mask(items);
> - if (!mask) {
> + mask = qemu_str_to_log_mask(items, &err);
> + if (err) {
> + error_free(err);
> hmp_help_cmd(mon, "log");
> return;
> }
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 98e071e63b..02193696b9 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2486,8 +2486,11 @@ static void qemu_process_early_options(void)
> {
> int mask = 0;
> if (log_mask) {
> - mask = qemu_str_to_log_mask(log_mask);
> - if (!mask) {
> + Error *err = NULL;
> +
> + mask = qemu_str_to_log_mask(log_mask, &err);
> + if (err) {
> + error_report_err(err);
> qemu_print_log_usage(stdout);
> exit(1);
> }
> diff --git a/util/log.c b/util/log.c
> index def88a9402..b5f08db202 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -500,8 +500,8 @@ const QEMULogItem qemu_log_items[] = {
> { 0, NULL, NULL },
> };
>
> -/* takes a comma separated list of log masks. Return 0 if error. */
> -int qemu_str_to_log_mask(const char *str)
> +/* Takes a comma separated list of log masks. */
> +int qemu_str_to_log_mask(const char *str, Error **errp)
> {
> const QEMULogItem *item;
> int mask = 0;
> @@ -524,6 +524,7 @@ int qemu_str_to_log_mask(const char *str)
char **parts = g_strsplit(str, ",", 0);
char **tmp;
When @parts is null or "", the loop runs zero times, and ...
for (tmp = parts; tmp && *tmp; tmp++) {
if (g_str_equal(*tmp, "all")) {
for (item = qemu_log_items; item->mask != 0; item++) {
mask |= item->mask;
}
#ifdef CONFIG_TRACE_LOG
} else if (g_str_has_prefix(*tmp, "trace:") && (*tmp)[6] != '\0') {
trace_enable_events((*tmp) + 6);
mask |= LOG_TRACE;
#endif
} else {
for (item = qemu_log_items; item->mask != 0; item++) {
if (g_str_equal(*tmp, item->name)) {
> goto found;
> }
> }
> + error_setg(errp, "Invalid -d option \"%s\"", *tmp);
> goto error;
> found:
> mask |= item->mask;
}
}
... we end up here with mask zero and no error set.
g_strfreev(parts);
return mask;
Before the patch, this counted as error. Afterwards, it doesn't.
Impact? Worth mentioning in the commit message?
error:
g_strfreev(parts);
return 0;
Returning -1 here would stick to error.h's recommendation.
}
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v17 09/16] util/log: Add Error argument to qemu_str_to_log_mask
2023-10-10 12:55 ` Markus Armbruster
@ 2023-10-15 18:55 ` Richard Henderson
0 siblings, 0 replies; 39+ messages in thread
From: Richard Henderson @ 2023-10-15 18:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, fei2.wu, Philippe Mathieu-Daudé
On 10/10/23 05:55, Markus Armbruster wrote:
> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> Do not rely on return value of 0 to indicate error,
>> pass along an Error pointer to be set.
>
> Not wrong, but goes against error.h's recommendation
>
> * - Whenever practical, also return a value that indicates success /
> * failure. This can make the error checking more concise, and can
> * avoid useless error object creation and destruction. Note that
> * we still have many functions returning void. We recommend
> * • bool-valued functions return true on success / false on failure,
> * • pointer-valued functions return non-null / null pointer, and
> * • integer-valued functions return non-negative / negative.
>
...
> Returning -1 here would stick to error.h's recommendation.
The problem had been that 0 would become a valid result in the next patch.
But using -1 would work, particularly with bit 31 of the log mask as yet unused.
r~
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 10/16] util/log: Add -d tb_stats
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (8 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 09/16] util/log: Add Error argument to qemu_str_to_log_mask Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-10 12:34 ` Philippe Mathieu-Daudé
2023-10-03 18:30 ` [PATCH v17 11/16] accel/tcg: Add tb_stats_collect and tb_stats_dump Richard Henderson
` (6 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu, Vanderson M . do Rosario, Alex Bennée
From: Fei Wu <fei2.wu@intel.com>
Enable TBStatistics collection from startup.
Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
[rth: Change "tb_stats_foo" to "tb_stats:foo"]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
stubs/tb-stats.c | 16 ++++++++++++++++
util/log.c | 36 +++++++++++++++++++++++++++++++-----
stubs/meson.build | 1 +
3 files changed, 48 insertions(+), 5 deletions(-)
create mode 100644 stubs/tb-stats.c
diff --git a/stubs/tb-stats.c b/stubs/tb-stats.c
new file mode 100644
index 0000000000..ceaa1622ce
--- /dev/null
+++ b/stubs/tb-stats.c
@@ -0,0 +1,16 @@
+/*
+ * TB Stats Stubs
+ *
+ * Copyright (c) 2019
+ * Written by Alex Bennée <alex.bennee@linaro.org>
+ *
+ * This code is licensed under the GNU GPL v2, or later.
+ */
+
+
+#include "qemu/osdep.h"
+#include "tcg/tb-stats.h"
+
+void tb_stats_init(uint32_t flags)
+{
+}
diff --git a/util/log.c b/util/log.c
index b5f08db202..0cb987fb74 100644
--- a/util/log.c
+++ b/util/log.c
@@ -30,6 +30,9 @@
#ifdef CONFIG_LINUX
#include <sys/syscall.h>
#endif
+#ifdef CONFIG_TCG
+#include "tcg/tb-stats.h"
+#endif
typedef struct RCUCloseFILE {
@@ -509,22 +512,41 @@ int qemu_str_to_log_mask(const char *str, Error **errp)
char **tmp;
for (tmp = parts; tmp && *tmp; tmp++) {
- if (g_str_equal(*tmp, "all")) {
+ char *t = *tmp;
+
+ if (g_str_equal(t, "all")) {
for (item = qemu_log_items; item->mask != 0; item++) {
mask |= item->mask;
}
#ifdef CONFIG_TRACE_LOG
- } else if (g_str_has_prefix(*tmp, "trace:") && (*tmp)[6] != '\0') {
- trace_enable_events((*tmp) + 6);
+ } else if (g_str_has_prefix(t, "trace:") && t[6] != '\0') {
+ trace_enable_events(t + 6);
mask |= LOG_TRACE;
+#endif
+#ifdef CONFIG_TCG
+ } else if (g_str_has_prefix(t, "tb_stats:") && t[9] != '\0') {
+ int flags = TB_STATS_NONE;
+ char *v = t + 9;
+
+ if (g_str_equal(v, "all")) {
+ flags = TB_STATS_ALL;
+ } else if (g_str_equal(v, "jit")) {
+ flags = TB_STATS_JIT;
+ } else if (g_str_equal(v, "exec")) {
+ flags = TB_STATS_EXEC;
+ } else {
+ error_setg(errp, "Invalid -d option \"%s\"", t);
+ goto error;
+ }
+ tb_stats_init(flags);
#endif
} else {
for (item = qemu_log_items; item->mask != 0; item++) {
- if (g_str_equal(*tmp, item->name)) {
+ if (g_str_equal(t, item->name)) {
goto found;
}
}
- error_setg(errp, "Invalid -d option \"%s\"", *tmp);
+ error_setg(errp, "Invalid -d option \"%s\"", t);
goto error;
found:
mask |= item->mask;
@@ -546,6 +568,10 @@ void qemu_print_log_usage(FILE *f)
for (item = qemu_log_items; item->mask != 0; item++) {
fprintf(f, "%-15s %s\n", item->name, item->help);
}
+#ifdef CONFIG_TCG
+ fprintf(f, "tb_stats:WHICH enable translation block statistics"
+ " (all, exec, jit)\n");
+#endif
#ifdef CONFIG_TRACE_LOG
fprintf(f, "trace:PATTERN enable trace events\n");
fprintf(f, "\nUse \"-d trace:help\" to get a list of trace events.\n\n");
diff --git a/stubs/meson.build b/stubs/meson.build
index ef6e39a64d..37ca25ea01 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -65,4 +65,5 @@ else
stub_ss.add(files('qdev.c'))
endif
stub_ss.add(files('semihost-all.c'))
+stub_ss.add(files('tb-stats.c'))
stub_ss.add(when: 'CONFIG_VFIO_USER_SERVER', if_false: files('vfio-user-obj.c'))
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 10/16] util/log: Add -d tb_stats
2023-10-03 18:30 ` [PATCH v17 10/16] util/log: Add -d tb_stats Richard Henderson
@ 2023-10-10 12:34 ` Philippe Mathieu-Daudé
2023-10-15 19:53 ` Richard Henderson
0 siblings, 1 reply; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-10 12:34 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: fei2.wu, Vanderson M . do Rosario, Alex Bennée
On 3/10/23 20:30, Richard Henderson wrote:
> From: Fei Wu <fei2.wu@intel.com>
>
> Enable TBStatistics collection from startup.
>
> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> [rth: Change "tb_stats_foo" to "tb_stats:foo"]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> stubs/tb-stats.c | 16 ++++++++++++++++
> util/log.c | 36 +++++++++++++++++++++++++++++++-----
> stubs/meson.build | 1 +
> 3 files changed, 48 insertions(+), 5 deletions(-)
> create mode 100644 stubs/tb-stats.c
>
> diff --git a/stubs/tb-stats.c b/stubs/tb-stats.c
> new file mode 100644
> index 0000000000..ceaa1622ce
> --- /dev/null
> +++ b/stubs/tb-stats.c
> @@ -0,0 +1,16 @@
> +/*
> + * TB Stats Stubs
> + *
> + * Copyright (c) 2019
> + * Written by Alex Bennée <alex.bennee@linaro.org>
> + *
> + * This code is licensed under the GNU GPL v2, or later.
> + */
> +
> +
> +#include "qemu/osdep.h"
> +#include "tcg/tb-stats.h"
> +
> +void tb_stats_init(uint32_t flags)
> +{
> +}
We don't need this stub anymore.
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v17 10/16] util/log: Add -d tb_stats
2023-10-10 12:34 ` Philippe Mathieu-Daudé
@ 2023-10-15 19:53 ` Richard Henderson
0 siblings, 0 replies; 39+ messages in thread
From: Richard Henderson @ 2023-10-15 19:53 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: fei2.wu, Vanderson M . do Rosario, Alex Bennée
On 10/10/23 05:34, Philippe Mathieu-Daudé wrote:
> On 3/10/23 20:30, Richard Henderson wrote:
>> From: Fei Wu <fei2.wu@intel.com>
>>
>> Enable TBStatistics collection from startup.
>>
>> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>> [rth: Change "tb_stats_foo" to "tb_stats:foo"]
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> stubs/tb-stats.c | 16 ++++++++++++++++
>> util/log.c | 36 +++++++++++++++++++++++++++++++-----
>> stubs/meson.build | 1 +
>> 3 files changed, 48 insertions(+), 5 deletions(-)
>> create mode 100644 stubs/tb-stats.c
>>
>> diff --git a/stubs/tb-stats.c b/stubs/tb-stats.c
>> new file mode 100644
>> index 0000000000..ceaa1622ce
>> --- /dev/null
>> +++ b/stubs/tb-stats.c
>> @@ -0,0 +1,16 @@
>> +/*
>> + * TB Stats Stubs
>> + *
>> + * Copyright (c) 2019
>> + * Written by Alex Bennée <alex.bennee@linaro.org>
>> + *
>> + * This code is licensed under the GNU GPL v2, or later.
>> + */
>> +
>> +
>> +#include "qemu/osdep.h"
>> +#include "tcg/tb-stats.h"
>> +
>> +void tb_stats_init(uint32_t flags)
>> +{
>> +}
>
> We don't need this stub anymore.
Certainly we do, within the tools and tests.
r~
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 11/16] accel/tcg: Add tb_stats_collect and tb_stats_dump
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (9 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 10/16] util/log: Add -d tb_stats Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-16 14:48 ` Alex Bennée
2023-10-03 18:30 ` [PATCH v17 12/16] softmmu: Export qemu_ram_ptr_length Richard Henderson
` (5 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu, Vanderson M. do Rosario, Alex Bennée
From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
These functions will be used together to output statistics.
Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
[rth: Split out of a larger patch]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/tcg/tb-stats.h | 25 +++++++++
accel/tcg/tb-stats.c | 119 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 144 insertions(+)
diff --git a/include/tcg/tb-stats.h b/include/tcg/tb-stats.h
index 1ec0d13eff..edee73b63b 100644
--- a/include/tcg/tb-stats.h
+++ b/include/tcg/tb-stats.h
@@ -129,4 +129,29 @@ void tb_stats_reset_tbs(void);
TBStatistics *tb_stats_lookup(tb_page_addr_t phys_pc, vaddr pc,
uint32_t flags, uint64_t flags2);
+/**
+ * tb_stats_collect:
+ * @max: maximum number of results
+ * @sort: sort function
+ *
+ * Collect all TBStatistics and return the first @max items,
+ * as dictated by the sort criteria.
+ */
+GPtrArray *tb_stats_collect(unsigned max, GCompareFunc sort);
+
+/* Sort functions for tb_stats_collect. */
+gint tb_stats_sort_by_spills(gconstpointer, gconstpointer);
+gint tb_stats_sort_by_coverage(gconstpointer, gconstpointer);
+gint tb_stats_sort_by_hg(gconstpointer, gconstpointer);
+
+/**
+ * tb_stats_dump:
+ * @s: structure to dump
+ * @index: label to emit
+ *
+ * Return a string with the rendering of the data in @s;
+ * @index is included in the output.
+ */
+GString *tb_stats_dump(TBStatistics *s, unsigned index);
+
#endif /* TCG_TB_STATS_H */
diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
index 424c9a90ec..b2c9445b67 100644
--- a/accel/tcg/tb-stats.c
+++ b/accel/tcg/tb-stats.c
@@ -83,3 +83,122 @@ TBStatistics *tb_stats_lookup(tb_page_addr_t phys_pc, vaddr pc,
}
return s;
}
+
+static void tb_stats_collect_iter(void *p, uint32_t hash, void *u)
+{
+ g_ptr_array_add(u, p);
+}
+
+static void calculate_coverages(GPtrArray *array)
+{
+ double total_exec_count = 0;
+ guint i, n = array->len;
+
+ for (i = 0; i < n; ++i) {
+ TBStatistics *s = g_ptr_array_index(array, i);
+ double avg_insns = 1;
+ double exec_count;
+
+ if (s->translations.total) {
+ avg_insns = s->code.num_guest_inst / (double)s->translations.total;
+ }
+ exec_count = ((double)s->executions.atomic + s->executions.normal)
+ / avg_insns;
+ s->executions.coverage = exec_count;
+ total_exec_count += exec_count;
+ }
+
+ for (i = 0; i < n; ++i) {
+ TBStatistics *s = g_ptr_array_index(array, i);
+ s->executions.coverage /= total_exec_count;
+ }
+}
+
+GPtrArray *tb_stats_collect(unsigned max, GCompareFunc sort)
+{
+ GPtrArray *array = g_ptr_array_new();
+
+ /*
+ * Collect all TBStatistics and sort.
+ * Note that coverage data requires both execution and jit collection.
+ */
+ qht_iter(&tb_ctx.stats, tb_stats_collect_iter, array);
+ calculate_coverages(array);
+ g_ptr_array_sort(array, sort);
+
+ /* Truncate to the first MAX entries. */
+ if (max < array->len) {
+ g_ptr_array_set_size(array, max);
+ }
+ return array;
+}
+
+gint tb_stats_sort_by_spills(gconstpointer p1, gconstpointer p2)
+{
+ const TBStatistics *s1 = *(TBStatistics **)p1;
+ const TBStatistics *s2 = *(TBStatistics **)p2;
+ double c1 = (double)s1->code.spills / s1->translations.total;
+ double c2 = (double)s2->code.spills / s2->translations.total;
+
+ return c1 < c2 ? 1 : c1 == c2 ? 0 : -1;
+}
+
+gint tb_stats_sort_by_coverage(gconstpointer p1, gconstpointer p2)
+{
+ const TBStatistics *s1 = *(TBStatistics **)p1;
+ const TBStatistics *s2 = *(TBStatistics **)p2;
+ double c1 = s1->executions.coverage;
+ double c2 = s2->executions.coverage;
+
+ return c1 < c2 ? 1 : c1 == c2 ? 0 : -1;
+}
+
+gint tb_stats_sort_by_hg(gconstpointer p1, gconstpointer p2)
+{
+ const TBStatistics *s1 = *(TBStatistics **)p1;
+ const TBStatistics *s2 = *(TBStatistics **)p2;
+ double c1 = (double)s1->code.out_len / s1->code.num_guest_inst;
+ double c2 = (double)s2->code.out_len / s2->code.num_guest_inst;
+
+ return c1 < c2 ? 1 : c1 == c2 ? 0 : -1;
+}
+
+GString *tb_stats_dump(TBStatistics *s, unsigned index)
+{
+ unsigned n = s->tbs->len;
+ unsigned invalid = 0;
+ GString *buf;
+
+ for (unsigned i = 0; i < n; ++i) {
+ TranslationBlock *tb = g_ptr_array_index(s->tbs, i);
+ if (tb->cflags & CF_INVALID) {
+ invalid += 1;
+ }
+ }
+
+ buf = g_string_new("");
+ g_string_append_printf(buf,
+ "TB id:%u | phys:0x" TB_PAGE_ADDR_FMT " virt=%" VADDR_PRIx
+ " flags:0x%08x invalid:%u/%u\n",
+ index, s->phys_pc, s->pc, s->flags, invalid, n - invalid);
+
+ if (tb_stats_enabled & TB_STATS_EXEC) {
+ g_string_append_printf(buf,
+ "\t| exec:%lu/%lu coverage:%.2f%%\n",
+ s->executions.normal, s->executions.atomic,
+ s->executions.coverage * 100);
+ }
+
+ if (tb_stats_enabled & TB_STATS_JIT) {
+ g_string_append_printf(buf,
+ "\t| trans:%lu inst: g:%lu op:%lu op_opt:%lu spills:%ld\n"
+ "\t| h/g (host bytes / guest insts): %f\n",
+ s->translations.total,
+ s->code.num_guest_inst / s->translations.total,
+ s->code.num_tcg_ops / s->translations.total,
+ s->code.num_tcg_ops_opt / s->translations.total,
+ s->code.spills / s->translations.total,
+ (double)s->code.out_len / s->code.num_guest_inst);
+ }
+ return buf;
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 11/16] accel/tcg: Add tb_stats_collect and tb_stats_dump
2023-10-03 18:30 ` [PATCH v17 11/16] accel/tcg: Add tb_stats_collect and tb_stats_dump Richard Henderson
@ 2023-10-16 14:48 ` Alex Bennée
0 siblings, 0 replies; 39+ messages in thread
From: Alex Bennée @ 2023-10-16 14:48 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, fei2.wu, Vanderson M. do Rosario
Richard Henderson <richard.henderson@linaro.org> writes:
> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
>
> These functions will be used together to output statistics.
>
> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> [rth: Split out of a larger patch]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> include/tcg/tb-stats.h | 25 +++++++++
> accel/tcg/tb-stats.c | 119 +++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 144 insertions(+)
>
> diff --git a/include/tcg/tb-stats.h b/include/tcg/tb-stats.h
> index 1ec0d13eff..edee73b63b 100644
> --- a/include/tcg/tb-stats.h
> +++ b/include/tcg/tb-stats.h
> @@ -129,4 +129,29 @@ void tb_stats_reset_tbs(void);
> TBStatistics *tb_stats_lookup(tb_page_addr_t phys_pc, vaddr pc,
> uint32_t flags, uint64_t flags2);
>
> +/**
> + * tb_stats_collect:
> + * @max: maximum number of results
> + * @sort: sort function
> + *
> + * Collect all TBStatistics and return the first @max items,
> + * as dictated by the sort criteria.
> + */
> +GPtrArray *tb_stats_collect(unsigned max, GCompareFunc sort);
> +
> +/* Sort functions for tb_stats_collect. */
> +gint tb_stats_sort_by_spills(gconstpointer, gconstpointer);
> +gint tb_stats_sort_by_coverage(gconstpointer, gconstpointer);
> +gint tb_stats_sort_by_hg(gconstpointer, gconstpointer);
> +
> +/**
> + * tb_stats_dump:
> + * @s: structure to dump
> + * @index: label to emit
> + *
> + * Return a string with the rendering of the data in @s;
> + * @index is included in the output.
> + */
> +GString *tb_stats_dump(TBStatistics *s, unsigned index);
> +
> #endif /* TCG_TB_STATS_H */
> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
> index 424c9a90ec..b2c9445b67 100644
> --- a/accel/tcg/tb-stats.c
> +++ b/accel/tcg/tb-stats.c
> @@ -83,3 +83,122 @@ TBStatistics *tb_stats_lookup(tb_page_addr_t phys_pc, vaddr pc,
> }
> return s;
> }
> +
> +static void tb_stats_collect_iter(void *p, uint32_t hash, void *u)
> +{
> + g_ptr_array_add(u, p);
> +}
> +
> +static void calculate_coverages(GPtrArray *array)
> +{
> + double total_exec_count = 0;
> + guint i, n = array->len;
> +
> + for (i = 0; i < n; ++i) {
> + TBStatistics *s = g_ptr_array_index(array, i);
> + double avg_insns = 1;
> + double exec_count;
> +
> + if (s->translations.total) {
> + avg_insns = s->code.num_guest_inst / (double)s->translations.total;
> + }
> + exec_count = ((double)s->executions.atomic + s->executions.normal)
> + / avg_insns;
> + s->executions.coverage = exec_count;
> + total_exec_count += exec_count;
> + }
> +
> + for (i = 0; i < n; ++i) {
> + TBStatistics *s = g_ptr_array_index(array, i);
> + s->executions.coverage /= total_exec_count;
> + }
> +}
> +
> +GPtrArray *tb_stats_collect(unsigned max, GCompareFunc sort)
> +{
> + GPtrArray *array = g_ptr_array_new();
> +
> + /*
> + * Collect all TBStatistics and sort.
> + * Note that coverage data requires both execution and jit collection.
> + */
> + qht_iter(&tb_ctx.stats, tb_stats_collect_iter, array);
> + calculate_coverages(array);
> + g_ptr_array_sort(array, sort);
> +
> + /* Truncate to the first MAX entries. */
> + if (max < array->len) {
> + g_ptr_array_set_size(array, max);
> + }
> + return array;
> +}
> +
> +gint tb_stats_sort_by_spills(gconstpointer p1, gconstpointer p2)
> +{
> + const TBStatistics *s1 = *(TBStatistics **)p1;
> + const TBStatistics *s2 = *(TBStatistics **)p2;
> + double c1 = (double)s1->code.spills / s1->translations.total;
> + double c2 = (double)s2->code.spills / s2->translations.total;
> +
> + return c1 < c2 ? 1 : c1 == c2 ? 0 : -1;
> +}
> +
> +gint tb_stats_sort_by_coverage(gconstpointer p1, gconstpointer p2)
> +{
> + const TBStatistics *s1 = *(TBStatistics **)p1;
> + const TBStatistics *s2 = *(TBStatistics **)p2;
> + double c1 = s1->executions.coverage;
> + double c2 = s2->executions.coverage;
> +
> + return c1 < c2 ? 1 : c1 == c2 ? 0 : -1;
> +}
> +
> +gint tb_stats_sort_by_hg(gconstpointer p1, gconstpointer p2)
> +{
> + const TBStatistics *s1 = *(TBStatistics **)p1;
> + const TBStatistics *s2 = *(TBStatistics **)p2;
> + double c1 = (double)s1->code.out_len / s1->code.num_guest_inst;
> + double c2 = (double)s2->code.out_len / s2->code.num_guest_inst;
> +
> + return c1 < c2 ? 1 : c1 == c2 ? 0 : -1;
> +}
> +
> +GString *tb_stats_dump(TBStatistics *s, unsigned index)
> +{
> + unsigned n = s->tbs->len;
> + unsigned invalid = 0;
> + GString *buf;
> +
> + for (unsigned i = 0; i < n; ++i) {
> + TranslationBlock *tb = g_ptr_array_index(s->tbs, i);
> + if (tb->cflags & CF_INVALID) {
> + invalid += 1;
> + }
> + }
> +
> + buf = g_string_new("");
> + g_string_append_printf(buf,
> + "TB id:%u | phys:0x" TB_PAGE_ADDR_FMT " virt=%" VADDR_PRIx
> + " flags:0x%08x invalid:%u/%u\n",
> + index, s->phys_pc, s->pc, s->flags, invalid, n - invalid);
This is a little unclear in the output maybe:
flags:0x%08x translations:%u (of which %u invalidated)
and adjust the names appropriately?
> +
> + if (tb_stats_enabled & TB_STATS_EXEC) {
> + g_string_append_printf(buf,
> + "\t| exec:%lu/%lu coverage:%.2f%%\n",
> + s->executions.normal, s->executions.atomic,
> + s->executions.coverage * 100);
> + }
> +
> + if (tb_stats_enabled & TB_STATS_JIT) {
> + g_string_append_printf(buf,
> + "\t| trans:%lu inst: g:%lu op:%lu op_opt:%lu spills:%ld\n"
> + "\t| h/g (host bytes / guest insts): %f\n",
> + s->translations.total,
> + s->code.num_guest_inst / s->translations.total,
> + s->code.num_tcg_ops / s->translations.total,
> + s->code.num_tcg_ops_opt / s->translations.total,
> + s->code.spills / s->translations.total,
> + (double)s->code.out_len / s->code.num_guest_inst);
> + }
> + return buf;
> +}
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 12/16] softmmu: Export qemu_ram_ptr_length
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (10 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 11/16] accel/tcg: Add tb_stats_collect and tb_stats_dump Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-10 12:31 ` Philippe Mathieu-Daudé
2023-10-03 18:30 ` [PATCH v17 13/16] disas: Allow monitor_disas to read from ram_addr_t Richard Henderson
` (4 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/exec/memory.h | 2 ++
softmmu/physmem.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index ef23d65afc..ebdecf64a6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2874,6 +2874,8 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
hwaddr len, hwaddr addr1, hwaddr l,
MemoryRegion *mr);
void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
+void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
+ hwaddr *size, bool lock);
/* Internal functions, part of the implementation of address_space_read_cached
* and address_space_write_cached. */
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 309653c722..69a853c27a 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2182,8 +2182,8 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
*
* Called within RCU critical section.
*/
-static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
- hwaddr *size, bool lock)
+void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
+ hwaddr *size, bool lock)
{
RAMBlock *block = ram_block;
if (*size == 0) {
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 12/16] softmmu: Export qemu_ram_ptr_length
2023-10-03 18:30 ` [PATCH v17 12/16] softmmu: Export qemu_ram_ptr_length Richard Henderson
@ 2023-10-10 12:31 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-10 12:31 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: fei2.wu
On 3/10/23 20:30, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> include/exec/memory.h | 2 ++
> softmmu/physmem.c | 4 ++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index ef23d65afc..ebdecf64a6 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2874,6 +2874,8 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
> hwaddr len, hwaddr addr1, hwaddr l,
> MemoryRegion *mr);
> void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
> +void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> + hwaddr *size, bool lock);
>
> /* Internal functions, part of the implementation of address_space_read_cached
> * and address_space_write_cached. */
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 309653c722..69a853c27a 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2182,8 +2182,8 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
> *
> * Called within RCU critical section.
> */
> -static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> - hwaddr *size, bool lock)
> +void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> + hwaddr *size, bool lock)
> {
> RAMBlock *block = ram_block;
> if (*size == 0) {
Pre-existing, function named with '_length' suffix but takes a 'size'
parameter.
Preferably moving the docstring along with the public declaration:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 13/16] disas: Allow monitor_disas to read from ram_addr_t
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (11 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 12/16] softmmu: Export qemu_ram_ptr_length Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-10 12:46 ` Philippe Mathieu-Daudé
2023-10-03 18:30 ` [PATCH v17 14/16] monitor: Change MonitorDec.get_value return type to int64_t Richard Henderson
` (3 subsequent siblings)
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu, Vanderson M. do Rosario, Alex Bennée
From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
Introduce a MonitorDisasSpace to replace the current is_physical
boolean argument to monitor_disas. Generate an error if we attempt
to read past the end of a single RAMBlock.
Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
[rth: Split out of a larger patch; validate the RAMBlock size]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/disas/disas.h | 8 +++++++-
disas/disas-mon.c | 32 ++++++++++++++++++++++++++++++--
monitor/hmp-cmds-target.c | 27 ++++++++++++++++-----------
3 files changed, 53 insertions(+), 14 deletions(-)
diff --git a/include/disas/disas.h b/include/disas/disas.h
index 176775eff7..cd99b0ccd0 100644
--- a/include/disas/disas.h
+++ b/include/disas/disas.h
@@ -5,8 +5,14 @@
void disas(FILE *out, const void *code, size_t size);
void target_disas(FILE *out, CPUState *cpu, uint64_t code, size_t size);
+typedef enum {
+ MON_DISAS_GVA, /* virtual */
+ MON_DISAS_GPA, /* physical */
+ MON_DISAS_GRA, /* ram_addr_t */
+} MonitorDisasSpace;
+
void monitor_disas(Monitor *mon, CPUState *cpu, uint64_t pc,
- int nb_insn, bool is_physical);
+ int nb_insn, MonitorDisasSpace space);
char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size);
diff --git a/disas/disas-mon.c b/disas/disas-mon.c
index 48ac492c6c..d486f64c92 100644
--- a/disas/disas-mon.c
+++ b/disas/disas-mon.c
@@ -23,9 +23,27 @@ physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
return res == MEMTX_OK ? 0 : EIO;
}
+static int
+ram_addr_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
+ struct disassemble_info *info)
+{
+ hwaddr hw_length;
+ void *p;
+
+ RCU_READ_LOCK_GUARD();
+
+ hw_length = length;
+ p = qemu_ram_ptr_length(NULL, memaddr, &hw_length, false);
+ if (hw_length < length) {
+ return EIO;
+ }
+ memcpy(myaddr, p, length);
+ return 0;
+}
+
/* Disassembler for the monitor. */
void monitor_disas(Monitor *mon, CPUState *cpu, uint64_t pc,
- int nb_insn, bool is_physical)
+ int nb_insn, MonitorDisasSpace space)
{
int count, i;
CPUDebug s;
@@ -35,8 +53,18 @@ void monitor_disas(Monitor *mon, CPUState *cpu, uint64_t pc,
s.info.fprintf_func = disas_gstring_printf;
s.info.stream = (FILE *)ds; /* abuse this slot */
- if (is_physical) {
+ switch (space) {
+ case MON_DISAS_GVA:
+ /* target_read_memory set in disas_initialize_debug_target */
+ break;
+ case MON_DISAS_GPA:
s.info.read_memory_func = physical_read_memory;
+ break;
+ case MON_DISAS_GRA:
+ s.info.read_memory_func = ram_addr_read_memory;
+ break;
+ default:
+ g_assert_not_reached();
}
s.info.buffer_vma = pc;
diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
index d9fbcac08d..476cf68e81 100644
--- a/monitor/hmp-cmds-target.c
+++ b/monitor/hmp-cmds-target.c
@@ -120,21 +120,20 @@ void hmp_info_registers(Monitor *mon, const QDict *qdict)
}
static void memory_dump(Monitor *mon, int count, int format, int wsize,
- hwaddr addr, int is_physical)
+ hwaddr addr, MonitorDisasSpace space)
{
int l, line_size, i, max_digits, len;
uint8_t buf[16];
uint64_t v;
CPUState *cs = mon_get_cpu(mon);
- if (!cs && (format == 'i' || !is_physical)) {
+ if (space == MON_DISAS_GVA || format == 'i') {
monitor_printf(mon, "Can not dump without CPU\n");
return;
}
if (format == 'i') {
- monitor_disas(mon, cs, addr, count, is_physical);
- return;
+ monitor_disas(mon, cs, addr, count, space);
}
len = wsize * count;
@@ -163,15 +162,21 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
}
while (len > 0) {
- if (is_physical) {
- monitor_printf(mon, HWADDR_FMT_plx ":", addr);
- } else {
+ switch (space) {
+ case MON_DISAS_GVA:
monitor_printf(mon, TARGET_FMT_lx ":", (target_ulong)addr);
+ break;
+ case MON_DISAS_GPA:
+ monitor_printf(mon, HWADDR_FMT_plx ":", addr);
+ break;
+ default:
+ g_assert_not_reached();
}
l = len;
- if (l > line_size)
+ if (l > line_size) {
l = line_size;
- if (is_physical) {
+ }
+ if (space == MON_DISAS_GPA) {
AddressSpace *as = cs ? cs->as : &address_space_memory;
MemTxResult r = address_space_read(as, addr,
MEMTXATTRS_UNSPECIFIED, buf, l);
@@ -235,7 +240,7 @@ void hmp_memory_dump(Monitor *mon, const QDict *qdict)
int size = qdict_get_int(qdict, "size");
target_long addr = qdict_get_int(qdict, "addr");
- memory_dump(mon, count, format, size, addr, 0);
+ memory_dump(mon, count, format, size, addr, MON_DISAS_GVA);
}
void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict)
@@ -245,7 +250,7 @@ void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict)
int size = qdict_get_int(qdict, "size");
hwaddr addr = qdict_get_int(qdict, "addr");
- memory_dump(mon, count, format, size, addr, 1);
+ memory_dump(mon, count, format, size, addr, MON_DISAS_GPA);
}
void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp)
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 13/16] disas: Allow monitor_disas to read from ram_addr_t
2023-10-03 18:30 ` [PATCH v17 13/16] disas: Allow monitor_disas to read from ram_addr_t Richard Henderson
@ 2023-10-10 12:46 ` Philippe Mathieu-Daudé
2023-10-15 19:21 ` Alex Bennée
0 siblings, 1 reply; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-10 12:46 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: fei2.wu, Vanderson M. do Rosario, Alex Bennée
On 3/10/23 20:30, Richard Henderson wrote:
> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
>
> Introduce a MonitorDisasSpace to replace the current is_physical
> boolean argument to monitor_disas. Generate an error if we attempt
> to read past the end of a single RAMBlock.
>
> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> [rth: Split out of a larger patch; validate the RAMBlock size]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> include/disas/disas.h | 8 +++++++-
> disas/disas-mon.c | 32 ++++++++++++++++++++++++++++++--
> monitor/hmp-cmds-target.c | 27 ++++++++++++++++-----------
> 3 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/include/disas/disas.h b/include/disas/disas.h
> index 176775eff7..cd99b0ccd0 100644
> --- a/include/disas/disas.h
> +++ b/include/disas/disas.h
> @@ -5,8 +5,14 @@
> void disas(FILE *out, const void *code, size_t size);
> void target_disas(FILE *out, CPUState *cpu, uint64_t code, size_t size);
>
> +typedef enum {
> + MON_DISAS_GVA, /* virtual */
> + MON_DISAS_GPA, /* physical */
> + MON_DISAS_GRA, /* ram_addr_t */
> +} MonitorDisasSpace;
Obviously I'd rather see MonitorDisasSpace = {MON_DISAS_GVA/GPA}
introduced in a preliminary patch, but just saying.
> static void memory_dump(Monitor *mon, int count, int format, int wsize,
> - hwaddr addr, int is_physical)
> + hwaddr addr, MonitorDisasSpace space)
> {
> int l, line_size, i, max_digits, len;
> uint8_t buf[16];
> uint64_t v;
> CPUState *cs = mon_get_cpu(mon);
>
> - if (!cs && (format == 'i' || !is_physical)) {
Why is the '!cs' check removed? Otherwise LGTM.
> + if (space == MON_DISAS_GVA || format == 'i') {
> monitor_printf(mon, "Can not dump without CPU\n");
> return;
> }
>
> if (format == 'i') {
> - monitor_disas(mon, cs, addr, count, is_physical);
> - return;
> + monitor_disas(mon, cs, addr, count, space);
> }
>
> len = wsize * count;
> @@ -163,15 +162,21 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
> }
>
> while (len > 0) {
> - if (is_physical) {
> - monitor_printf(mon, HWADDR_FMT_plx ":", addr);
> - } else {
> + switch (space) {
> + case MON_DISAS_GVA:
> monitor_printf(mon, TARGET_FMT_lx ":", (target_ulong)addr);
> + break;
> + case MON_DISAS_GPA:
> + monitor_printf(mon, HWADDR_FMT_plx ":", addr);
> + break;
> + default:
> + g_assert_not_reached();
> }
> l = len;
> - if (l > line_size)
> + if (l > line_size) {
> l = line_size;
> - if (is_physical) {
> + }
> + if (space == MON_DISAS_GPA) {
> AddressSpace *as = cs ? cs->as : &address_space_memory;
> MemTxResult r = address_space_read(as, addr,
> MEMTXATTRS_UNSPECIFIED, buf, l);
> @@ -235,7 +240,7 @@ void hmp_memory_dump(Monitor *mon, const QDict *qdict)
> int size = qdict_get_int(qdict, "size");
> target_long addr = qdict_get_int(qdict, "addr");
>
> - memory_dump(mon, count, format, size, addr, 0);
> + memory_dump(mon, count, format, size, addr, MON_DISAS_GVA);
> }
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v17 13/16] disas: Allow monitor_disas to read from ram_addr_t
2023-10-10 12:46 ` Philippe Mathieu-Daudé
@ 2023-10-15 19:21 ` Alex Bennée
0 siblings, 0 replies; 39+ messages in thread
From: Alex Bennée @ 2023-10-15 19:21 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Richard Henderson, qemu-devel, fei2.wu, Vanderson M. do Rosario
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 3/10/23 20:30, Richard Henderson wrote:
>> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
>> Introduce a MonitorDisasSpace to replace the current is_physical
>> boolean argument to monitor_disas. Generate an error if we attempt
>> to read past the end of a single RAMBlock.
>> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>> [rth: Split out of a larger patch; validate the RAMBlock size]
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> include/disas/disas.h | 8 +++++++-
>> disas/disas-mon.c | 32 ++++++++++++++++++++++++++++++--
>> monitor/hmp-cmds-target.c | 27 ++++++++++++++++-----------
>> 3 files changed, 53 insertions(+), 14 deletions(-)
>> diff --git a/include/disas/disas.h b/include/disas/disas.h
>> index 176775eff7..cd99b0ccd0 100644
>> --- a/include/disas/disas.h
>> +++ b/include/disas/disas.h
>> @@ -5,8 +5,14 @@
>> void disas(FILE *out, const void *code, size_t size);
>> void target_disas(FILE *out, CPUState *cpu, uint64_t code, size_t size);
>> +typedef enum {
>> + MON_DISAS_GVA, /* virtual */
>> + MON_DISAS_GPA, /* physical */
>> + MON_DISAS_GRA, /* ram_addr_t */
>> +} MonitorDisasSpace;
>
>
> Obviously I'd rather see MonitorDisasSpace = {MON_DISAS_GVA/GPA}
> introduced in a preliminary patch, but just saying.
>
>> static void memory_dump(Monitor *mon, int count, int format, int wsize,
>> - hwaddr addr, int is_physical)
>> + hwaddr addr, MonitorDisasSpace space)
>> {
>> int l, line_size, i, max_digits, len;
>> uint8_t buf[16];
>> uint64_t v;
>> CPUState *cs = mon_get_cpu(mon);
>> - if (!cs && (format == 'i' || !is_physical)) {
>
> Why is the '!cs' check removed? Otherwise LGTM.
Yeah it breaks the monitor I think with should be:
if (!cs && (space == MON_DISAS_GVA || format == 'i'))
>
>> + if (space == MON_DISAS_GVA || format == 'i') {
>> monitor_printf(mon, "Can not dump without CPU\n");
>> return;
>> }
>> if (format == 'i') {
>> - monitor_disas(mon, cs, addr, count, is_physical);
>> - return;
>> + monitor_disas(mon, cs, addr, count, space);
>> }
>> len = wsize * count;
>> @@ -163,15 +162,21 @@ static void memory_dump(Monitor *mon, int count, int format, int wsize,
>> }
>> while (len > 0) {
>> - if (is_physical) {
>> - monitor_printf(mon, HWADDR_FMT_plx ":", addr);
>> - } else {
>> + switch (space) {
>> + case MON_DISAS_GVA:
>> monitor_printf(mon, TARGET_FMT_lx ":", (target_ulong)addr);
>> + break;
>> + case MON_DISAS_GPA:
>> + monitor_printf(mon, HWADDR_FMT_plx ":", addr);
>> + break;
>> + default:
>> + g_assert_not_reached();
>> }
>> l = len;
>> - if (l > line_size)
>> + if (l > line_size) {
>> l = line_size;
>> - if (is_physical) {
>> + }
>> + if (space == MON_DISAS_GPA) {
>> AddressSpace *as = cs ? cs->as : &address_space_memory;
>> MemTxResult r = address_space_read(as, addr,
>> MEMTXATTRS_UNSPECIFIED, buf, l);
>> @@ -235,7 +240,7 @@ void hmp_memory_dump(Monitor *mon, const QDict *qdict)
>> int size = qdict_get_int(qdict, "size");
>> target_long addr = qdict_get_int(qdict, "addr");
>> - memory_dump(mon, count, format, size, addr, 0);
>> + memory_dump(mon, count, format, size, addr, MON_DISAS_GVA);
>> }
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 14/16] monitor: Change MonitorDec.get_value return type to int64_t
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (12 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 13/16] disas: Allow monitor_disas to read from ram_addr_t Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-16 14:59 ` Alex Bennée
2023-10-16 15:43 ` Alex Bennée
2023-10-03 18:30 ` [PATCH v17 15/16] accel/tcg: Add info [tb-list|tb] commands to HMP Richard Henderson
` (2 subsequent siblings)
16 siblings, 2 replies; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu
This matches the type of the pval parameter to get_monitor_def.
This means that "monitor/hmp-target.h" itself is now target
independent, even if monitor/hmp-target.c isn't.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
include/monitor/hmp-target.h | 5 +----
monitor/hmp-target.c | 2 ++
target/i386/monitor.c | 4 ++--
target/ppc/ppc-qmp-cmds.c | 20 ++++++++++----------
target/sparc/monitor.c | 8 ++++----
5 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index d78e979f05..730507bd65 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -25,16 +25,13 @@
#ifndef MONITOR_HMP_TARGET_H
#define MONITOR_HMP_TARGET_H
-#include "cpu.h"
-
#define MD_TLONG 0
#define MD_I32 1
struct MonitorDef {
const char *name;
int offset;
- target_long (*get_value)(Monitor *mon, const struct MonitorDef *md,
- int val);
+ int64_t (*get_value)(Monitor *mon, const struct MonitorDef *md, int val);
int type;
};
diff --git a/monitor/hmp-target.c b/monitor/hmp-target.c
index 1eb72ac1bf..ed7149b5ff 100644
--- a/monitor/hmp-target.c
+++ b/monitor/hmp-target.c
@@ -35,6 +35,8 @@
#include "qapi/qapi-commands-machine.h"
#include "qapi/error.h"
#include "qemu/cutils.h"
+#include "cpu-param.h"
+#include "exec/target_long.h"
#if defined(TARGET_S390X)
#include "hw/s390x/storage-keys.h"
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 6512846327..6759ec7ca0 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -600,8 +600,8 @@ void hmp_mce(Monitor *mon, const QDict *qdict)
}
}
-static target_long monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
- int val)
+static int64_t monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env(mon);
return env->eip + env->segs[R_CS].base;
diff --git a/target/ppc/ppc-qmp-cmds.c b/target/ppc/ppc-qmp-cmds.c
index f9acc21056..ea2c152228 100644
--- a/target/ppc/ppc-qmp-cmds.c
+++ b/target/ppc/ppc-qmp-cmds.c
@@ -32,8 +32,8 @@
#include "cpu-models.h"
#include "cpu-qom.h"
-static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
- int val)
+static int64_t monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env(mon);
unsigned int u;
@@ -43,15 +43,15 @@ static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
return u;
}
-static target_long monitor_get_xer(Monitor *mon, const struct MonitorDef *md,
- int val)
+static int64_t monitor_get_xer(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env(mon);
return cpu_read_xer(env);
}
-static target_long monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
- int val)
+static int64_t monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env(mon);
if (!env->tb_env) {
@@ -60,8 +60,8 @@ static target_long monitor_get_decr(Monitor *mon, const struct MonitorDef *md,
return cpu_ppc_load_decr(env);
}
-static target_long monitor_get_tbu(Monitor *mon, const struct MonitorDef *md,
- int val)
+static int64_t monitor_get_tbu(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env(mon);
if (!env->tb_env) {
@@ -70,8 +70,8 @@ static target_long monitor_get_tbu(Monitor *mon, const struct MonitorDef *md,
return cpu_ppc_load_tbu(env);
}
-static target_long monitor_get_tbl(Monitor *mon, const struct MonitorDef *md,
- int val)
+static int64_t monitor_get_tbl(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env(mon);
if (!env->tb_env) {
diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
index 73f15aa272..24cc3dbf68 100644
--- a/target/sparc/monitor.c
+++ b/target/sparc/monitor.c
@@ -40,8 +40,8 @@ void hmp_info_tlb(Monitor *mon, const QDict *qdict)
}
#ifndef TARGET_SPARC64
-static target_long monitor_get_psr(Monitor *mon, const struct MonitorDef *md,
- int val)
+static int64_t monitor_get_psr(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env(mon);
@@ -49,8 +49,8 @@ static target_long monitor_get_psr(Monitor *mon, const struct MonitorDef *md,
}
#endif
-static target_long monitor_get_reg(Monitor *mon, const struct MonitorDef *md,
- int val)
+static int64_t monitor_get_reg(Monitor *mon, const struct MonitorDef *md,
+ int val)
{
CPUArchState *env = mon_get_cpu_env(mon);
return env->regwptr[val];
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v17 15/16] accel/tcg: Add info [tb-list|tb] commands to HMP
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (13 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 14/16] monitor: Change MonitorDec.get_value return type to int64_t Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-16 15:02 ` Alex Bennée
2023-10-03 18:30 ` [PATCH v17 16/16] accel/tcg: Dump hot TBs at the end of the execution Richard Henderson
2024-11-14 9:28 ` [PATCH v17 00/16] TCG code quality tracking Nikita Shubin
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu, Vanderson M. do Rosario, Alex Bennée
From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
These commands allow the exploration of TBs generated by the TCG.
Understand which one hotter, with more guest/host instructions,
and examine the guest code.
The goal of this command is to allow the dynamic exploration of
TCG behavior and code quality. Therefore, for now, a corresponding
QMP command is not worthwhile.
Example of output:
------------------------------
TB id:0 | phys:0xa21f562e virt:0x0000000000000000 flags:0x00028010 0 inv/1
| exec:6171503732/0 guest inst cov:94.77%
| trans:1 ints: g:8 op:28 op_opt:24 spills:0
| h/g (host bytes / guest insts): 37.000000
0xa21f562e: 00002797 auipc a5,8192 # 0xa21f762e
0xa21f5632: a2278793 addi a5,a5,-1502
0xa21f5636: 639c ld a5,0(a5)
0xa21f5638: 00178713 addi a4,a5,1
0xa21f563c: 00002797 auipc a5,8192 # 0xa21f763c
0xa21f5640: a1478793 addi a5,a5,-1516
0xa21f5644: e398 sd a4,0(a5)
0xa21f5646: b7e5 j -24 # 0xa21f562e
Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
[rth: Split out of a larger patch]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
accel/tcg/tb-context.h | 2 +
accel/tcg/monitor.c | 91 ++++++++++++++++++++++++++++++++++++++++++
accel/tcg/tb-stats.c | 2 +
hmp-commands-info.hx | 14 +++++++
4 files changed, 109 insertions(+)
diff --git a/accel/tcg/tb-context.h b/accel/tcg/tb-context.h
index 4b1abe392b..29d87200b6 100644
--- a/accel/tcg/tb-context.h
+++ b/accel/tcg/tb-context.h
@@ -35,6 +35,8 @@ struct TBContext {
/* statistics */
unsigned tb_flush_count;
unsigned tb_phys_invalidate_count;
+
+ GPtrArray *last_search;
};
extern TBContext tb_ctx;
diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
index 370fea883c..1be3218715 100644
--- a/accel/tcg/monitor.c
+++ b/accel/tcg/monitor.c
@@ -15,12 +15,14 @@
#include "qapi/qmp/qdict.h"
#include "monitor/monitor.h"
#include "monitor/hmp.h"
+#include "monitor/hmp-target.h"
#include "sysemu/cpus.h"
#include "sysemu/cpu-timers.h"
#include "sysemu/tcg.h"
#include "tcg/tcg.h"
#include "tcg/tb-stats.h"
#include "exec/tb-flush.h"
+#include "disas/disas.h"
#include "internal-common.h"
#include "tb-context.h"
@@ -303,10 +305,99 @@ static void hmp_tbstats(Monitor *mon, const QDict *qdict)
RUN_ON_CPU_HOST_INT(flags));
}
+static void hmp_info_tblist(Monitor *mon, const QDict *qdict)
+{
+ int max;
+ const char *sortedby_str;
+ GCompareFunc sort;
+ GPtrArray *array;
+
+ if (!tcg_enabled()) {
+ monitor_printf(mon, "Only available with accel=tcg\n");
+ return;
+ }
+ if (!tb_stats_enabled) {
+ monitor_printf(mon, "TB statistics not being recorded\n");
+ return;
+ }
+
+ max = qdict_get_try_int(qdict, "number", 10);
+ sortedby_str = qdict_get_try_str(qdict, "sortedby");
+
+ if (sortedby_str == NULL || g_str_equal(sortedby_str, "hotness")) {
+ sort = tb_stats_sort_by_coverage;
+ } else if (g_str_equal(sortedby_str, "hg")) {
+ sort = tb_stats_sort_by_hg;
+ } else if (g_str_equal(sortedby_str, "spills")) {
+ sort = tb_stats_sort_by_spills;
+ } else {
+ monitor_printf(mon, "Sort options are: hotness, hg, spills\n");
+ return;
+ }
+
+ g_ptr_array_unref(tb_ctx.last_search);
+ tb_ctx.last_search = NULL;
+
+ array = tb_stats_collect(max, sort);
+ max = array->len;
+ if (max == 0) {
+ monitor_printf(mon, "No TB statistics collected\n");
+ g_ptr_array_free(array, true);
+ return;
+ }
+
+ for (int i = 0; i < max; ++i) {
+ TBStatistics *s = g_ptr_array_index(array, i);
+ g_autoptr(GString) buf = tb_stats_dump(s, i);
+ monitor_puts(mon, buf->str);
+ }
+
+ /* Remember for the next "info tb" */
+ tb_ctx.last_search = array;
+}
+
+static void hmp_info_tb(Monitor *mon, const QDict *qdict)
+{
+ GPtrArray *array;
+ int id;
+
+ if (!tcg_enabled()) {
+ monitor_printf(mon, "Only available with accel=tcg\n");
+ return;
+ }
+
+ array = g_ptr_array_ref(tb_ctx.last_search);
+ if (!array) {
+ monitor_printf(mon, "No TB statistics collected\n");
+ return;
+ }
+
+ id = qdict_get_int(qdict, "id");
+ if (id < array->len) {
+ TBStatistics *s = g_ptr_array_index(array, id);
+ g_autoptr(GString) buf = tb_stats_dump(s, id);
+ monitor_puts(mon, buf->str);
+
+ for (int i = s->tbs->len - 1; i >= 0; --i) {
+ TranslationBlock *tb = g_ptr_array_index(s->tbs, i);
+ if (!(tb->cflags & CF_INVALID)) {
+ monitor_disas(mon, mon_get_cpu(mon), s->phys_pc,
+ tb->icount, MON_DISAS_GRA);
+ }
+ }
+ } else {
+ monitor_printf(mon, "TB %d information not recorded\n", id);
+ }
+
+ g_ptr_array_unref(array);
+}
+
static void hmp_tcg_register(void)
{
monitor_register_hmp_info_hrt("jit", qmp_x_query_jit);
monitor_register_hmp_info_hrt("opcount", qmp_x_query_opcount);
monitor_register_hmp("tb_stats", false, hmp_tbstats);
+ monitor_register_hmp("tb-list", true, hmp_info_tblist);
+ monitor_register_hmp("tb", true, hmp_info_tb);
}
type_init(hmp_tcg_register);
diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
index b2c9445b67..0f84c14a88 100644
--- a/accel/tcg/tb-stats.c
+++ b/accel/tcg/tb-stats.c
@@ -43,6 +43,8 @@ void tb_stats_init(uint32_t flags)
CODE_GEN_HTABLE_SIZE, QHT_MODE_AUTO_RESIZE);
}
} else {
+ g_ptr_array_unref(tb_ctx.last_search);
+ tb_ctx.last_search = NULL;
qht_iter(&tb_ctx.stats, tb_stats_free, NULL);
qht_destroy(&tb_ctx.stats);
}
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index f5b37eb74a..8e9b64cf7f 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -262,6 +262,20 @@ ERST
.params = "",
.help = "show dynamic compiler info",
},
+ {
+ .name = "tb-list",
+ .args_type = "number:i?,sortedby:s?",
+ .params = "[number sortedby]",
+ .help = "show a [number] translated blocks sorted by [sortedby]"
+ "sortedby opts: hotness hg spills",
+ },
+ {
+ .name = "tb",
+ .args_type = "id:i",
+ .params = "id",
+ .help = "show information about one translated block by id,"
+ "from the result of a previous \"info tb-list\"",
+ },
#endif
SRST
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 15/16] accel/tcg: Add info [tb-list|tb] commands to HMP
2023-10-03 18:30 ` [PATCH v17 15/16] accel/tcg: Add info [tb-list|tb] commands to HMP Richard Henderson
@ 2023-10-16 15:02 ` Alex Bennée
0 siblings, 0 replies; 39+ messages in thread
From: Alex Bennée @ 2023-10-16 15:02 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, fei2.wu, Vanderson M. do Rosario
Richard Henderson <richard.henderson@linaro.org> writes:
> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
>
> These commands allow the exploration of TBs generated by the TCG.
> Understand which one hotter, with more guest/host instructions,
> and examine the guest code.
>
> The goal of this command is to allow the dynamic exploration of
> TCG behavior and code quality. Therefore, for now, a corresponding
> QMP command is not worthwhile.
>
> Example of output:
>
> ------------------------------
>
> TB id:0 | phys:0xa21f562e virt:0x0000000000000000 flags:0x00028010 0 inv/1
> | exec:6171503732/0 guest inst cov:94.77%
> | trans:1 ints: g:8 op:28 op_opt:24 spills:0
> | h/g (host bytes / guest insts): 37.000000
>
> 0xa21f562e: 00002797 auipc a5,8192 # 0xa21f762e
> 0xa21f5632: a2278793 addi a5,a5,-1502
> 0xa21f5636: 639c ld a5,0(a5)
> 0xa21f5638: 00178713 addi a4,a5,1
> 0xa21f563c: 00002797 auipc a5,8192 # 0xa21f763c
> 0xa21f5640: a1478793 addi a5,a5,-1516
> 0xa21f5644: e398 sd a4,0(a5)
> 0xa21f5646: b7e5 j -24 # 0xa21f562e
>
> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> [rth: Split out of a larger patch]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> accel/tcg/tb-context.h | 2 +
> accel/tcg/monitor.c | 91 ++++++++++++++++++++++++++++++++++++++++++
> accel/tcg/tb-stats.c | 2 +
> hmp-commands-info.hx | 14 +++++++
> 4 files changed, 109 insertions(+)
>
> diff --git a/accel/tcg/tb-context.h b/accel/tcg/tb-context.h
> index 4b1abe392b..29d87200b6 100644
> --- a/accel/tcg/tb-context.h
> +++ b/accel/tcg/tb-context.h
> @@ -35,6 +35,8 @@ struct TBContext {
> /* statistics */
> unsigned tb_flush_count;
> unsigned tb_phys_invalidate_count;
> +
> + GPtrArray *last_search;
> };
>
> extern TBContext tb_ctx;
> diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
> index 370fea883c..1be3218715 100644
> --- a/accel/tcg/monitor.c
> +++ b/accel/tcg/monitor.c
> @@ -15,12 +15,14 @@
> #include "qapi/qmp/qdict.h"
> #include "monitor/monitor.h"
> #include "monitor/hmp.h"
> +#include "monitor/hmp-target.h"
> #include "sysemu/cpus.h"
> #include "sysemu/cpu-timers.h"
> #include "sysemu/tcg.h"
> #include "tcg/tcg.h"
> #include "tcg/tb-stats.h"
> #include "exec/tb-flush.h"
> +#include "disas/disas.h"
> #include "internal-common.h"
> #include "tb-context.h"
>
> @@ -303,10 +305,99 @@ static void hmp_tbstats(Monitor *mon, const QDict *qdict)
> RUN_ON_CPU_HOST_INT(flags));
> }
>
> +static void hmp_info_tblist(Monitor *mon, const QDict *qdict)
> +{
> + int max;
> + const char *sortedby_str;
> + GCompareFunc sort;
> + GPtrArray *array;
> +
> + if (!tcg_enabled()) {
> + monitor_printf(mon, "Only available with accel=tcg\n");
> + return;
> + }
> + if (!tb_stats_enabled) {
> + monitor_printf(mon, "TB statistics not being recorded\n");
> + return;
> + }
> +
> + max = qdict_get_try_int(qdict, "number", 10);
> + sortedby_str = qdict_get_try_str(qdict, "sortedby");
> +
> + if (sortedby_str == NULL || g_str_equal(sortedby_str, "hotness")) {
> + sort = tb_stats_sort_by_coverage;
> + } else if (g_str_equal(sortedby_str, "hg")) {
> + sort = tb_stats_sort_by_hg;
> + } else if (g_str_equal(sortedby_str, "spills")) {
> + sort = tb_stats_sort_by_spills;
> + } else {
> + monitor_printf(mon, "Sort options are: hotness, hg, spills\n");
> + return;
> + }
> +
> + g_ptr_array_unref(tb_ctx.last_search);
> + tb_ctx.last_search = NULL;
> +
> + array = tb_stats_collect(max, sort);
> + max = array->len;
> + if (max == 0) {
> + monitor_printf(mon, "No TB statistics collected\n");
> + g_ptr_array_free(array, true);
> + return;
> + }
> +
> + for (int i = 0; i < max; ++i) {
> + TBStatistics *s = g_ptr_array_index(array, i);
> + g_autoptr(GString) buf = tb_stats_dump(s, i);
> + monitor_puts(mon, buf->str);
> + }
> +
> + /* Remember for the next "info tb" */
> + tb_ctx.last_search = array;
> +}
> +
> +static void hmp_info_tb(Monitor *mon, const QDict *qdict)
> +{
> + GPtrArray *array;
> + int id;
> +
> + if (!tcg_enabled()) {
> + monitor_printf(mon, "Only available with accel=tcg\n");
> + return;
> + }
> +
> + array = g_ptr_array_ref(tb_ctx.last_search);
> + if (!array) {
> + monitor_printf(mon, "No TB statistics collected\n");
> + return;
> + }
> +
> + id = qdict_get_int(qdict, "id");
> + if (id < array->len) {
> + TBStatistics *s = g_ptr_array_index(array, id);
> + g_autoptr(GString) buf = tb_stats_dump(s, id);
> + monitor_puts(mon, buf->str);
> +
> + for (int i = s->tbs->len - 1; i >= 0; --i) {
> + TranslationBlock *tb = g_ptr_array_index(s->tbs, i);
> + if (!(tb->cflags & CF_INVALID)) {
> + monitor_disas(mon, mon_get_cpu(mon), s->phys_pc,
> + tb->icount, MON_DISAS_GRA);
> + }
I'm confused by the state I've got to:
(qemu) info tb 0
TB id:0 | phys:0x1f59d5918 virt=0 flags:0x00000051 invalid:0/1
| exec:57340981/0 coverage:20.73%
| trans:1 inst: g:1 op:19 op_opt:18 spills:0
| h/g (host bytes / guest insts): 136.000000
0x1f59d5918: 35000354 cbnz w20, #0x1f59d5980
(qemu) xp/5i 0x1f59d5918
0x1f59d5918: 00000000 .byte 0x00, 0x00, 0x00, 0x00
0x1f59d591c: 00000000 .byte 0x00, 0x00, 0x00, 0x00
0x1f59d5920: 00000000 .byte 0x00, 0x00, 0x00, 0x00
0x1f59d5924: 00000000 .byte 0x00, 0x00, 0x00, 0x00
0x1f59d5928: 00000000 .byte 0x00, 0x00, 0x00, 0x00
It seems this is the kernels busy loop (so I assume resident in memory)
but trying to dump the instructions directly it fails. I assume the
physical memory address in each case is the same right?
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v17 16/16] accel/tcg: Dump hot TBs at the end of the execution
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (14 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 15/16] accel/tcg: Add info [tb-list|tb] commands to HMP Richard Henderson
@ 2023-10-03 18:30 ` Richard Henderson
2023-10-10 12:36 ` Philippe Mathieu-Daudé
2024-11-14 9:28 ` [PATCH v17 00/16] TCG code quality tracking Nikita Shubin
16 siblings, 1 reply; 39+ messages in thread
From: Richard Henderson @ 2023-10-03 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: fei2.wu
From: Fei Wu <fei2.wu@intel.com>
Dump the hottest TBs if -d tb_stats:{all,jit,exec}[:dump_num_at_exit]
Signed-off-by: Fei Wu <fei2.wu@intel.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
bsd-user/bsd-proc.h | 2 ++
include/tcg/tb-stats.h | 10 +++++++++-
accel/tcg/monitor.c | 8 +++++---
accel/tcg/tb-stats.c | 27 ++++++++++++++++++++++++++-
linux-user/exit.c | 10 ++++++----
softmmu/runstate.c | 2 ++
stubs/tb-stats.c | 6 +++++-
util/log.c | 20 ++++++++++++++++----
8 files changed, 71 insertions(+), 14 deletions(-)
diff --git a/bsd-user/bsd-proc.h b/bsd-user/bsd-proc.h
index 0e1d461c4c..84b52b399a 100644
--- a/bsd-user/bsd-proc.h
+++ b/bsd-user/bsd-proc.h
@@ -21,12 +21,14 @@
#define BSD_PROC_H_
#include <sys/resource.h>
+#include "tcg/tb-stats.h"
/* exit(2) */
static inline abi_long do_bsd_exit(void *cpu_env, abi_long arg1)
{
gdb_exit(arg1);
qemu_plugin_user_exit();
+ tb_stats_dump_atexit();
_exit(arg1);
return 0;
diff --git a/include/tcg/tb-stats.h b/include/tcg/tb-stats.h
index edee73b63b..d3cca94f84 100644
--- a/include/tcg/tb-stats.h
+++ b/include/tcg/tb-stats.h
@@ -41,11 +41,12 @@ extern uint32_t tb_stats_enabled;
/**
* tb_stats_init:
* @flags: TB_STATS_* flags to enable.
+ * @atexit: count of hottest tbs to log.
*
* Initialize translation block statistics, enabling @flags.
* If @flags is 0, disable all statistics.
*/
-void tb_stats_init(uint32_t flags);
+void tb_stats_init(uint32_t flags, uint32_t atexit);
/*
* This struct stores statistics such as execution count of the
@@ -154,4 +155,11 @@ gint tb_stats_sort_by_hg(gconstpointer, gconstpointer);
*/
GString *tb_stats_dump(TBStatistics *s, unsigned index);
+/**
+ * tb_stats_dump_atexit:
+ *
+ * Log any requested TBs at end of execution.
+ */
+void tb_stats_dump_atexit(void);
+
#endif /* TCG_TB_STATS_H */
diff --git a/accel/tcg/monitor.c b/accel/tcg/monitor.c
index 1be3218715..7719583654 100644
--- a/accel/tcg/monitor.c
+++ b/accel/tcg/monitor.c
@@ -245,7 +245,7 @@ static void tb_stats_init_safe(CPUState *cpu, run_on_cpu_data icmd)
{
uint32_t flags = icmd.host_int;
- tb_stats_init(flags);
+ tb_stats_init(flags, 0);
tb_flush(cpu);
}
@@ -335,8 +335,10 @@ static void hmp_info_tblist(Monitor *mon, const QDict *qdict)
return;
}
- g_ptr_array_unref(tb_ctx.last_search);
- tb_ctx.last_search = NULL;
+ if (tb_ctx.last_search) {
+ g_ptr_array_unref(tb_ctx.last_search);
+ tb_ctx.last_search = NULL;
+ }
array = tb_stats_collect(max, sort);
max = array->len;
diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
index 0f84c14a88..62a6228799 100644
--- a/accel/tcg/tb-stats.c
+++ b/accel/tcg/tb-stats.c
@@ -8,10 +8,12 @@
#include "qemu/osdep.h"
#include "qemu/xxhash.h"
+#include "qemu/log.h"
#include "tcg/tb-stats.h"
#include "tb-context.h"
uint32_t tb_stats_enabled;
+static uint32_t tb_stats_atexit;
static bool tb_stats_cmp(const void *ap, const void *bp)
{
@@ -34,7 +36,7 @@ static void tb_stats_free(void *p, uint32_t hash, void *userp)
g_free(s);
}
-void tb_stats_init(uint32_t flags)
+void tb_stats_init(uint32_t flags, uint32_t atexit)
{
tb_stats_enabled = flags;
if (flags) {
@@ -48,6 +50,14 @@ void tb_stats_init(uint32_t flags)
qht_iter(&tb_ctx.stats, tb_stats_free, NULL);
qht_destroy(&tb_ctx.stats);
}
+
+ /*
+ * This function is also used by HMP, when atexit is 0.
+ * Preserve the value set from the command-line.
+ */
+ if (atexit) {
+ tb_stats_atexit = atexit;
+ }
}
static void tb_stats_reset(void *p, uint32_t hash, void *userp)
@@ -204,3 +214,18 @@ GString *tb_stats_dump(TBStatistics *s, unsigned index)
}
return buf;
}
+
+void tb_stats_dump_atexit(void)
+{
+ if (tb_stats_enabled && tb_stats_atexit) {
+ g_autoptr(GPtrArray) array =
+ tb_stats_collect(tb_stats_atexit, tb_stats_sort_by_coverage);
+
+ for (uint32_t i = 0, n = array->len; i < n; ++i) {
+ TBStatistics *s = g_ptr_array_index(array, i);
+ g_autoptr(GString) str = tb_stats_dump(s, i);
+
+ qemu_log("%s\n", str->str);
+ }
+ }
+}
diff --git a/linux-user/exit.c b/linux-user/exit.c
index 50266314e0..4487aaac7e 100644
--- a/linux-user/exit.c
+++ b/linux-user/exit.c
@@ -22,6 +22,7 @@
#include "qemu.h"
#include "user-internals.h"
#include "qemu/plugin.h"
+#include "tcg/tb-stats.h"
#ifdef CONFIG_GCOV
extern void __gcov_dump(void);
@@ -30,9 +31,10 @@ extern void __gcov_dump(void);
void preexit_cleanup(CPUArchState *env, int code)
{
#ifdef CONFIG_GCOV
- __gcov_dump();
+ __gcov_dump();
#endif
- gdb_exit(code);
- qemu_plugin_user_exit();
- perf_exit();
+ gdb_exit(code);
+ qemu_plugin_user_exit();
+ perf_exit();
+ tb_stats_dump_atexit();
}
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 1652ed0439..2c6fb9bff1 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -59,6 +59,7 @@
#include "sysemu/runstate-action.h"
#include "sysemu/sysemu.h"
#include "sysemu/tpm.h"
+#include "tcg/tb-stats.h"
#include "trace.h"
static NotifierList exit_notifiers =
@@ -846,6 +847,7 @@ void qemu_cleanup(void)
/* No more vcpu or device emulation activity beyond this point */
vm_shutdown();
replay_finish();
+ tb_stats_dump_atexit();
/*
* We must cancel all block jobs while the block layer is drained,
diff --git a/stubs/tb-stats.c b/stubs/tb-stats.c
index ceaa1622ce..f9e4ef5d04 100644
--- a/stubs/tb-stats.c
+++ b/stubs/tb-stats.c
@@ -11,6 +11,10 @@
#include "qemu/osdep.h"
#include "tcg/tb-stats.h"
-void tb_stats_init(uint32_t flags)
+void tb_stats_init(uint32_t flags, uint32_t atexit)
+{
+}
+
+void tb_stats_dump_atexit(void)
{
}
diff --git a/util/log.c b/util/log.c
index 0cb987fb74..789b19a226 100644
--- a/util/log.c
+++ b/util/log.c
@@ -526,19 +526,31 @@ int qemu_str_to_log_mask(const char *str, Error **errp)
#ifdef CONFIG_TCG
} else if (g_str_has_prefix(t, "tb_stats:") && t[9] != '\0') {
int flags = TB_STATS_NONE;
+ unsigned atexit = 0;
char *v = t + 9;
+ char *e = strchr(v, ':');
+ size_t len;
- if (g_str_equal(v, "all")) {
+ if (e) {
+ len = e - v;
+ if (qemu_strtoui(e + 1, NULL, 10, &atexit) < 0) {
+ error_setg(errp, "Invalid -d option \"%s\"", t);
+ goto error;
+ }
+ } else {
+ len = strlen(v);
+ }
+ if (strncmp(v, "all", len) == 0) {
flags = TB_STATS_ALL;
- } else if (g_str_equal(v, "jit")) {
+ } else if (strncmp(v, "jit", len) == 0) {
flags = TB_STATS_JIT;
- } else if (g_str_equal(v, "exec")) {
+ } else if (strncmp(v, "exec", len) == 0) {
flags = TB_STATS_EXEC;
} else {
error_setg(errp, "Invalid -d option \"%s\"", t);
goto error;
}
- tb_stats_init(flags);
+ tb_stats_init(flags, atexit);
#endif
} else {
for (item = qemu_log_items; item->mask != 0; item++) {
--
2.34.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 16/16] accel/tcg: Dump hot TBs at the end of the execution
2023-10-03 18:30 ` [PATCH v17 16/16] accel/tcg: Dump hot TBs at the end of the execution Richard Henderson
@ 2023-10-10 12:36 ` Philippe Mathieu-Daudé
2023-10-10 13:23 ` Alex Bennée
0 siblings, 1 reply; 39+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-10-10 12:36 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: fei2.wu
On 3/10/23 20:30, Richard Henderson wrote:
> From: Fei Wu <fei2.wu@intel.com>
>
> Dump the hottest TBs if -d tb_stats:{all,jit,exec}[:dump_num_at_exit]
>
> Signed-off-by: Fei Wu <fei2.wu@intel.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> bsd-user/bsd-proc.h | 2 ++
> include/tcg/tb-stats.h | 10 +++++++++-
> accel/tcg/monitor.c | 8 +++++---
> accel/tcg/tb-stats.c | 27 ++++++++++++++++++++++++++-
> linux-user/exit.c | 10 ++++++----
> softmmu/runstate.c | 2 ++
> stubs/tb-stats.c | 6 +++++-
> util/log.c | 20 ++++++++++++++++----
> 8 files changed, 71 insertions(+), 14 deletions(-)
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index 1652ed0439..2c6fb9bff1 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -59,6 +59,7 @@
> #include "sysemu/runstate-action.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/tpm.h"
> +#include "tcg/tb-stats.h"
> #include "trace.h"
>
> static NotifierList exit_notifiers =
> @@ -846,6 +847,7 @@ void qemu_cleanup(void)
> /* No more vcpu or device emulation activity beyond this point */
> vm_shutdown();
> replay_finish();
> + tb_stats_dump_atexit();
>
> /*
> * We must cancel all block jobs while the block layer is drained,
> diff --git a/stubs/tb-stats.c b/stubs/tb-stats.c
> index ceaa1622ce..f9e4ef5d04 100644
> --- a/stubs/tb-stats.c
> +++ b/stubs/tb-stats.c
> @@ -11,6 +11,10 @@
> #include "qemu/osdep.h"
> #include "tcg/tb-stats.h"
>
> -void tb_stats_init(uint32_t flags)
> +void tb_stats_init(uint32_t flags, uint32_t atexit)
> +{
> +}
> +
> +void tb_stats_dump_atexit(void)
> {
> }
The stub isn't needed using:
-- >8 --
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index 2c6fb9bff1..d05e2b8e1c 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -52,6 +52,7 @@
#include "qom/object.h"
#include "qom/object_interfaces.h"
#include "sysemu/cpus.h"
+#include "sysemu/tcg.h"
#include "sysemu/qtest.h"
#include "sysemu/replay.h"
#include "sysemu/reset.h"
@@ -847,7 +848,9 @@ void qemu_cleanup(void)
/* No more vcpu or device emulation activity beyond this point */
vm_shutdown();
replay_finish();
- tb_stats_dump_atexit();
+ if (tcg_enabled()) {
+ tb_stats_dump_atexit();
+ }
/*
* We must cancel all block jobs while the block layer is drained,
---
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v17 16/16] accel/tcg: Dump hot TBs at the end of the execution
2023-10-10 12:36 ` Philippe Mathieu-Daudé
@ 2023-10-10 13:23 ` Alex Bennée
0 siblings, 0 replies; 39+ messages in thread
From: Alex Bennée @ 2023-10-10 13:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Richard Henderson, fei2.wu, qemu-devel
Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> On 3/10/23 20:30, Richard Henderson wrote:
>> From: Fei Wu <fei2.wu@intel.com>
>> Dump the hottest TBs if -d
>> tb_stats:{all,jit,exec}[:dump_num_at_exit]
>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> bsd-user/bsd-proc.h | 2 ++
>> include/tcg/tb-stats.h | 10 +++++++++-
>> accel/tcg/monitor.c | 8 +++++---
>> accel/tcg/tb-stats.c | 27 ++++++++++++++++++++++++++-
>> linux-user/exit.c | 10 ++++++----
>> softmmu/runstate.c | 2 ++
>> stubs/tb-stats.c | 6 +++++-
>> util/log.c | 20 ++++++++++++++++----
>> 8 files changed, 71 insertions(+), 14 deletions(-)
>
>
>> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
>> index 1652ed0439..2c6fb9bff1 100644
>> --- a/softmmu/runstate.c
>> +++ b/softmmu/runstate.c
>> @@ -59,6 +59,7 @@
>> #include "sysemu/runstate-action.h"
>> #include "sysemu/sysemu.h"
>> #include "sysemu/tpm.h"
>> +#include "tcg/tb-stats.h"
>> #include "trace.h"
>> static NotifierList exit_notifiers =
>> @@ -846,6 +847,7 @@ void qemu_cleanup(void)
>> /* No more vcpu or device emulation activity beyond this point */
>> vm_shutdown();
>> replay_finish();
>> + tb_stats_dump_atexit();
>> /*
>> * We must cancel all block jobs while the block layer is drained,
>> diff --git a/stubs/tb-stats.c b/stubs/tb-stats.c
>> index ceaa1622ce..f9e4ef5d04 100644
>> --- a/stubs/tb-stats.c
>> +++ b/stubs/tb-stats.c
>> @@ -11,6 +11,10 @@
>> #include "qemu/osdep.h"
>> #include "tcg/tb-stats.h"
>> -void tb_stats_init(uint32_t flags)
>> +void tb_stats_init(uint32_t flags, uint32_t atexit)
>> +{
>> +}
>> +
>> +void tb_stats_dump_atexit(void)
>> {
>> }
>
> The stub isn't needed using:
>
> -- >8 --
> diff --git a/softmmu/runstate.c b/softmmu/runstate.c
> index 2c6fb9bff1..d05e2b8e1c 100644
> --- a/softmmu/runstate.c
> +++ b/softmmu/runstate.c
> @@ -52,6 +52,7 @@
> #include "qom/object.h"
> #include "qom/object_interfaces.h"
> #include "sysemu/cpus.h"
> +#include "sysemu/tcg.h"
> #include "sysemu/qtest.h"
> #include "sysemu/replay.h"
> #include "sysemu/reset.h"
> @@ -847,7 +848,9 @@ void qemu_cleanup(void)
> /* No more vcpu or device emulation activity beyond this point */
> vm_shutdown();
> replay_finish();
> - tb_stats_dump_atexit();
> + if (tcg_enabled()) {
> + tb_stats_dump_atexit();
> + }
Why be different from replay_finish() which is a stub?
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v17 00/16] TCG code quality tracking
2023-10-03 18:30 [PATCH v17 00/16] TCG code quality tracking Richard Henderson
` (15 preceding siblings ...)
2023-10-03 18:30 ` [PATCH v17 16/16] accel/tcg: Dump hot TBs at the end of the execution Richard Henderson
@ 2024-11-14 9:28 ` Nikita Shubin
2025-01-21 10:22 ` Chinmay Rath
16 siblings, 1 reply; 39+ messages in thread
From: Nikita Shubin @ 2024-11-14 9:28 UTC (permalink / raw)
To: richard.henderson; +Cc: fei2.wu, qemu-devel, Vanderson M. do Rosario
Hello Richard and Vanderson !
Any news on this series ? It's been 26 spins already (yep i am counting
Vanderson series also).
While it's a really great idea in general, it also has great
educational purpose.
I've rebased v17 on the top of current master branch, but maybe we can
hope for another spin ? Especially with graphs, which are missing in
latest series.
---
Yours,
Nikita Shubin
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v17 00/16] TCG code quality tracking
2024-11-14 9:28 ` [PATCH v17 00/16] TCG code quality tracking Nikita Shubin
@ 2025-01-21 10:22 ` Chinmay Rath
0 siblings, 0 replies; 39+ messages in thread
From: Chinmay Rath @ 2025-01-21 10:22 UTC (permalink / raw)
To: Nikita Shubin, richard.henderson
Cc: fei2.wu, qemu-devel, Vanderson M. do Rosario, harshpb
Hello Richard,
Are you planning to come up with any new iterations of this patch series, or planning to merge it sometime ?
This seems to be very useful, hence wanted to know your plans with this.
Thanks,
Chinmay
On 11/14/24 14:58, Nikita Shubin wrote:
> Hello Richard and Vanderson !
>
> Any news on this series ? It's been 26 spins already (yep i am counting
> Vanderson series also).
>
> While it's a really great idea in general, it also has great
> educational purpose.
>
> I've rebased v17 on the top of current master branch, but maybe we can
> hope for another spin ? Especially with graphs, which are missing in
> latest series.
>
> ---
> Yours,
>
> Nikita Shubin
>
^ permalink raw reply [flat|nested] 39+ messages in thread