| Message ID | 20191007152839.30804-7-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | TCG code quality tracking and perf integration | expand |
On 10/7/19 11:28 AM, Alex Bennée wrote: > From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com> > > -d tb_stats[[,level=(+all+jit+exec+time)][,dump_limit=<number>]] > > "dump_limit" is used to limit the number of dumped TBStats in > linux-user mode. > > [all+jit+exec+time] control the profilling level used > by the TBStats. Can be used as follow: > > -d tb_stats > -d tb_stats,level=jit+time > -d tb_stats,dump_limit=15 > ... > > Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com> > Message-Id: <20190829173437.5926-7-vandersonmr2@gmail.com> > [AJB: fix authorship, reword title] > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > AJB: > - reword title > - add stubs for enabling > - move things across to tb-stats-flags.h > --- > accel/tcg/tb-stats.c | 5 +++++ > include/exec/gen-icount.h | 1 + > include/exec/tb-stats-flags.h | 29 +++++++++++++++++++++++++++++ > include/exec/tb-stats.h | 16 +++------------- > include/qemu/log.h | 1 + > stubs/Makefile.objs | 1 + > stubs/tb-stats.c | 27 +++++++++++++++++++++++++++ > util/log.c | 35 +++++++++++++++++++++++++++++++++++ > 8 files changed, 102 insertions(+), 13 deletions(-) > create mode 100644 include/exec/tb-stats-flags.h > create mode 100644 stubs/tb-stats.c > > diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c > index f431159fd2..1c66e03979 100644 > --- a/accel/tcg/tb-stats.c > +++ b/accel/tcg/tb-stats.c > @@ -193,3 +193,8 @@ uint32_t get_default_tbstats_flag(void) > { > return default_tbstats_flag; > } > + > +void set_default_tbstats_flag(uint32_t flags) > +{ > + default_tbstats_flag = flags; > +} > diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h > index be006383b9..3987adfb0e 100644 > --- a/include/exec/gen-icount.h > +++ b/include/exec/gen-icount.h > @@ -2,6 +2,7 @@ > #define GEN_ICOUNT_H > > #include "qemu/timer.h" > +#include "tb-stats-flags.h" > > /* Helpers for instruction counting code generation. */ > > diff --git a/include/exec/tb-stats-flags.h b/include/exec/tb-stats-flags.h > new file mode 100644 > index 0000000000..8455073048 > --- /dev/null > +++ b/include/exec/tb-stats-flags.h > @@ -0,0 +1,29 @@ > +/* > + * QEMU System Emulator, Code Quality Monitor System > + * > + * We define the flags and control bits here to avoid complications of > + * including TCG/CPU information in common code. > + * > + * Copyright (c) 2019 Vanderson M. do Rosario <vandersonmr2@gmail.com> > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > +#ifndef TB_STATS_FLAGS > +#define TB_STATS_FLAGS > + > +#define TB_NOTHING (1 << 0) Repeating my question about TB_NOTHING -- what is it? > +#define TB_EXEC_STATS (1 << 1) > +#define TB_JIT_STATS (1 << 2) > +#define TB_JIT_TIME (1 << 3) > + > +/* TBStatistic collection controls */ > +void enable_collect_tb_stats(void); > +void disable_collect_tb_stats(void); > +void pause_collect_tb_stats(void); > +bool tb_stats_collection_enabled(void); > +bool tb_stats_collection_paused(void); > + > +uint32_t get_default_tbstats_flag(void); > +void set_default_tbstats_flag(uint32_t); Is a get/set really better than an exported variable? Should we have created this header in the first place, rather than moving stuff here in patch 6? > + } else if (g_str_has_prefix(*tmp, "tb_stats")) { > + mask |= CPU_LOG_TB_STATS; > + set_default_tbstats_flag(TB_JIT_STATS | TB_EXEC_STATS | TB_JIT_TIME); Surely TB_ALL_STATS? > + } else if (g_str_equal(*level_tmp, "all")) { > + flags |= TB_JIT_STATS | TB_EXEC_STATS | TB_JIT_TIME; Likewise. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 10/7/19 11:28 AM, Alex Bennée wrote: >> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com> >> >> -d tb_stats[[,level=(+all+jit+exec+time)][,dump_limit=<number>]] >> >> "dump_limit" is used to limit the number of dumped TBStats in >> linux-user mode. >> >> [all+jit+exec+time] control the profilling level used >> by the TBStats. Can be used as follow: >> >> -d tb_stats >> -d tb_stats,level=jit+time >> -d tb_stats,dump_limit=15 >> ... >> >> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com> >> Message-Id: <20190829173437.5926-7-vandersonmr2@gmail.com> >> [AJB: fix authorship, reword title] >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> AJB: >> - reword title >> - add stubs for enabling >> - move things across to tb-stats-flags.h >> --- >> accel/tcg/tb-stats.c | 5 +++++ >> include/exec/gen-icount.h | 1 + >> include/exec/tb-stats-flags.h | 29 +++++++++++++++++++++++++++++ >> include/exec/tb-stats.h | 16 +++------------- >> include/qemu/log.h | 1 + >> stubs/Makefile.objs | 1 + >> stubs/tb-stats.c | 27 +++++++++++++++++++++++++++ >> util/log.c | 35 +++++++++++++++++++++++++++++++++++ >> 8 files changed, 102 insertions(+), 13 deletions(-) >> create mode 100644 include/exec/tb-stats-flags.h >> create mode 100644 stubs/tb-stats.c >> >> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c >> index f431159fd2..1c66e03979 100644 >> --- a/accel/tcg/tb-stats.c >> +++ b/accel/tcg/tb-stats.c >> @@ -193,3 +193,8 @@ uint32_t get_default_tbstats_flag(void) >> { >> return default_tbstats_flag; >> } >> + >> +void set_default_tbstats_flag(uint32_t flags) >> +{ >> + default_tbstats_flag = flags; >> +} >> diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h >> index be006383b9..3987adfb0e 100644 >> --- a/include/exec/gen-icount.h >> +++ b/include/exec/gen-icount.h >> @@ -2,6 +2,7 @@ >> #define GEN_ICOUNT_H >> >> #include "qemu/timer.h" >> +#include "tb-stats-flags.h" >> >> /* Helpers for instruction counting code generation. */ >> >> diff --git a/include/exec/tb-stats-flags.h b/include/exec/tb-stats-flags.h >> new file mode 100644 >> index 0000000000..8455073048 >> --- /dev/null >> +++ b/include/exec/tb-stats-flags.h >> @@ -0,0 +1,29 @@ >> +/* >> + * QEMU System Emulator, Code Quality Monitor System >> + * >> + * We define the flags and control bits here to avoid complications of >> + * including TCG/CPU information in common code. >> + * >> + * Copyright (c) 2019 Vanderson M. do Rosario <vandersonmr2@gmail.com> >> + * >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + */ >> +#ifndef TB_STATS_FLAGS >> +#define TB_STATS_FLAGS >> + >> +#define TB_NOTHING (1 << 0) > > Repeating my question about TB_NOTHING -- what is it? > >> +#define TB_EXEC_STATS (1 << 1) >> +#define TB_JIT_STATS (1 << 2) >> +#define TB_JIT_TIME (1 << 3) >> + >> +/* TBStatistic collection controls */ >> +void enable_collect_tb_stats(void); >> +void disable_collect_tb_stats(void); >> +void pause_collect_tb_stats(void); >> +bool tb_stats_collection_enabled(void); >> +bool tb_stats_collection_paused(void); >> + >> +uint32_t get_default_tbstats_flag(void); >> +void set_default_tbstats_flag(uint32_t); > > Is a get/set really better than an exported variable? It makes things easier for log.c which is used for multiple binaries although I never actually used empty inlines instead having stubs. I'll have to check if the tools define CONFIG_TCG anyway. > > Should we have created this header in the first place, > rather than moving stuff here in patch 6? Yes. I'll move it. > > Surely TB_ALL_STATS? > >> + } else if (g_str_equal(*level_tmp, "all")) { >> + flags |= TB_JIT_STATS | TB_EXEC_STATS | TB_JIT_TIME; > > Likewise. > > > r~ Thanks, -- Alex Bennée
diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c index f431159fd2..1c66e03979 100644 --- a/accel/tcg/tb-stats.c +++ b/accel/tcg/tb-stats.c @@ -193,3 +193,8 @@ uint32_t get_default_tbstats_flag(void) { return default_tbstats_flag; } + +void set_default_tbstats_flag(uint32_t flags) +{ + default_tbstats_flag = flags; +} diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h index be006383b9..3987adfb0e 100644 --- a/include/exec/gen-icount.h +++ b/include/exec/gen-icount.h @@ -2,6 +2,7 @@ #define GEN_ICOUNT_H #include "qemu/timer.h" +#include "tb-stats-flags.h" /* Helpers for instruction counting code generation. */ diff --git a/include/exec/tb-stats-flags.h b/include/exec/tb-stats-flags.h new file mode 100644 index 0000000000..8455073048 --- /dev/null +++ b/include/exec/tb-stats-flags.h @@ -0,0 +1,29 @@ +/* + * QEMU System Emulator, Code Quality Monitor System + * + * We define the flags and control bits here to avoid complications of + * including TCG/CPU information in common code. + * + * Copyright (c) 2019 Vanderson M. do Rosario <vandersonmr2@gmail.com> + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ +#ifndef TB_STATS_FLAGS +#define TB_STATS_FLAGS + +#define TB_NOTHING (1 << 0) +#define TB_EXEC_STATS (1 << 1) +#define TB_JIT_STATS (1 << 2) +#define TB_JIT_TIME (1 << 3) + +/* TBStatistic collection controls */ +void enable_collect_tb_stats(void); +void disable_collect_tb_stats(void); +void pause_collect_tb_stats(void); +bool tb_stats_collection_enabled(void); +bool tb_stats_collection_paused(void); + +uint32_t get_default_tbstats_flag(void); +void set_default_tbstats_flag(uint32_t); + +#endif diff --git a/include/exec/tb-stats.h b/include/exec/tb-stats.h index a142972960..019d316f5c 100644 --- a/include/exec/tb-stats.h +++ b/include/exec/tb-stats.h @@ -30,6 +30,8 @@ #include "exec/tb-context.h" #include "tcg.h" +#include "exec/tb-stats-flags.h" + #define tb_stats_enabled(tb, JIT_STATS) \ (tb && tb->tb_stats && (tb->tb_stats->stats_enabled & JIT_STATS)) @@ -108,21 +110,9 @@ bool tb_stats_cmp(const void *ap, const void *bp); void dump_jit_exec_time_info(uint64_t dev_time); +void set_tbstats_flags(uint32_t flags); void init_tb_stats_htable_if_not(void); void dump_jit_profile_info(TCGProfile *s); -#define TB_NOTHING (1 << 0) -#define TB_EXEC_STATS (1 << 1) -#define TB_JIT_STATS (1 << 2) -#define TB_JIT_TIME (1 << 3) - -void enable_collect_tb_stats(void); -void disable_collect_tb_stats(void); -void pause_collect_tb_stats(void); -bool tb_stats_collection_enabled(void); -bool tb_stats_collection_paused(void); - -uint32_t get_default_tbstats_flag(void); - #endif diff --git a/include/qemu/log.h b/include/qemu/log.h index b097a6cae1..a8d1997cde 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -45,6 +45,7 @@ static inline bool qemu_log_separate(void) /* LOG_TRACE (1 << 15) is defined in log-for-trace.h */ #define CPU_LOG_TB_OP_IND (1 << 16) #define CPU_LOG_TB_FPU (1 << 17) +#define CPU_LOG_TB_STATS (1 << 18) /* Lock output for a series of related logs. Since this is not needed * for a single qemu_log / qemu_log_mask / qemu_log_mask_and_addr, we diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 9c7393b08c..1c5cd05147 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -41,3 +41,4 @@ stub-obj-y += ram-block.o stub-obj-y += ramfb.o stub-obj-y += fw_cfg.o stub-obj-$(CONFIG_SOFTMMU) += semihost.o +stub-obj-$(CONFIG_TCG) += tb-stats.o diff --git a/stubs/tb-stats.c b/stubs/tb-stats.c new file mode 100644 index 0000000000..d212c2a1fa --- /dev/null +++ b/stubs/tb-stats.c @@ -0,0 +1,27 @@ +/* + * 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 "exec/tb-stats-flags.h" + +void enable_collect_tb_stats(void) +{ + return; +} + +bool tb_stats_collection_enabled(void) +{ + return false; +} + +void set_default_tbstats_flag(uint32_t flags) +{ + return; +} diff --git a/util/log.c b/util/log.c index 1d1b33f7d9..86bd691967 100644 --- a/util/log.c +++ b/util/log.c @@ -19,17 +19,20 @@ #include "qemu/osdep.h" #include "qemu/log.h" +#include "qemu/qemu-print.h" #include "qemu/range.h" #include "qemu/error-report.h" #include "qapi/error.h" #include "qemu/cutils.h" #include "trace/control.h" +#include "exec/tb-stats-flags.h" static char *logfilename; FILE *qemu_logfile; int qemu_loglevel; static int log_append = 0; static GArray *debug_regions; +int32_t max_num_hot_tbs_to_dump; /* Return the number of characters emitted. */ int qemu_log(const char *fmt, ...) @@ -273,6 +276,9 @@ const QEMULogItem qemu_log_items[] = { { CPU_LOG_TB_NOCHAIN, "nochain", "do not chain compiled TBs so that \"exec\" and \"cpu\" show\n" "complete traces" }, + { CPU_LOG_TB_STATS, "tb_stats[[,level=(+all+jit+exec+time)][,dump_limit=<number>]]", + "enable collection of TBs statistics" + "(and dump until given a limit if in user mode).\n" }, { 0, NULL, NULL }, }; @@ -294,6 +300,35 @@ int qemu_str_to_log_mask(const char *str) trace_enable_events((*tmp) + 6); mask |= LOG_TRACE; #endif + } else if (g_str_has_prefix(*tmp, "tb_stats")) { + mask |= CPU_LOG_TB_STATS; + set_default_tbstats_flag(TB_JIT_STATS | TB_EXEC_STATS | TB_JIT_TIME); + enable_collect_tb_stats(); + } else if (tb_stats_collection_enabled() && + g_str_has_prefix(*tmp, "dump_limit=")) { + max_num_hot_tbs_to_dump = atoi((*tmp) + 11); + } else if (tb_stats_collection_enabled() && + g_str_has_prefix(*tmp, "level=")) { + uint32_t flags = 0; + char **level_parts = g_strsplit(*tmp + 6, "+", 0); + char **level_tmp; + for (level_tmp = level_parts; level_tmp && *level_tmp; level_tmp++) { + if (g_str_equal(*level_tmp, "jit")) { + flags |= TB_JIT_STATS; + } else if (g_str_equal(*level_tmp, "exec")) { + flags |= TB_EXEC_STATS; + } else if (g_str_equal(*level_tmp, "time")) { + flags |= TB_JIT_TIME; + } else if (g_str_equal(*level_tmp, "all")) { + flags |= TB_JIT_STATS | TB_EXEC_STATS | TB_JIT_TIME; + } else { + /* FIXME: set errp */ + fprintf(stderr, "no option level=%s, valid options are:" + "all, jit, exec or/and time\n", *level_tmp); + exit(1); + } + set_default_tbstats_flag(flags); + } } else { for (item = qemu_log_items; item->mask != 0; item++) { if (g_str_equal(*tmp, item->name)) {