On 6/1/2023 10:03 PM, Richard Henderson wrote: > On 5/31/23 22:44, Wu, Fei wrote: >> On 6/1/2023 8:05 AM, Richard Henderson wrote: >>> On 5/30/23 01:35, Fei Wu wrote: >>>> From: "Vanderson M. do Rosario" <vanderson...@gmail.com> >>>> >>>> If a TB has a TBS (TBStatistics) with the TB_EXEC_STATS >>>> enabled, then we instrument the start code of this TB >>>> to atomically count the number of times it is executed. >>>> We count both the number of "normal" executions and atomic >>>> executions of a TB. >>>> >>>> The execution count of the TB is stored in its respective >>>> TBS. >>>> >>>> All TBStatistics are created by default with the flags from >>>> default_tbstats_flag. >>>> >>>> [Richard Henderson created the inline gen_tb_exec_count] >>>> >>>> Signed-off-by: Vanderson M. do Rosario <vanderson...@gmail.com> >>>> Message-Id: <20190829173437.5926-3-vanderson...@gmail.com> >>>> [AJB: Fix author] >>>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org> >>>> Signed-off-by: Fei Wu <fei2...@intel.com> >>>> --- >>>> accel/tcg/cpu-exec.c | 6 ++++++ >>>> accel/tcg/tb-stats.c | 6 ++++++ >>>> accel/tcg/tcg-runtime.c | 1 + >>>> accel/tcg/translate-all.c | 7 +++++-- >>>> accel/tcg/translator.c | 25 +++++++++++++++++++++++++ >>>> include/exec/gen-icount.h | 1 + >>>> include/exec/tb-stats-flags.h | 5 +++++ >>>> include/exec/tb-stats.h | 13 +++++++++++++ >>>> 8 files changed, 62 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c >>>> index 0e741960da..c0d8f26237 100644 >>>> --- a/accel/tcg/cpu-exec.c >>>> +++ b/accel/tcg/cpu-exec.c >>>> @@ -25,6 +25,7 @@ >>>> #include "trace.h" >>>> #include "disas/disas.h" >>>> #include "exec/exec-all.h" >>>> +#include "exec/tb-stats.h" >>>> #include "tcg/tcg.h" >>>> #include "qemu/atomic.h" >>>> #include "qemu/rcu.h" >>>> @@ -562,7 +563,12 @@ void cpu_exec_step_atomic(CPUState *cpu) >>>> mmap_unlock(); >>>> } >>>> + if (tb_stats_enabled(tb, TB_EXEC_STATS)) { >>>> + tb->tb_stats->executions.atomic++; >>>> + } >>>> + >>>> cpu_exec_enter(cpu); >>>> + >>>> /* execute the generated code */ >>>> trace_exec_tb(tb, pc); >>>> cpu_tb_exec(cpu, tb, &tb_exit); >>>> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c >>>> index f988bd8a31..143a52ef5c 100644 >>>> --- a/accel/tcg/tb-stats.c >>>> +++ b/accel/tcg/tb-stats.c >>>> @@ -22,6 +22,7 @@ enum TBStatsStatus { >>>> }; >>>> static enum TBStatsStatus tcg_collect_tb_stats; >>>> +static uint32_t default_tbstats_flag; >>>> void init_tb_stats_htable(void) >>>> { >>>> @@ -56,3 +57,8 @@ bool tb_stats_collection_paused(void) >>>> { >>>> return tcg_collect_tb_stats == TB_STATS_PAUSED; >>>> } >>>> + >>>> +uint32_t get_default_tbstats_flag(void) >>>> +{ >>>> + return default_tbstats_flag; >>>> +} >>> >>> What is the purpose of this function, instead of a global variable? >>> What is the meaning of 'default' in its name? >>> >> tbs have their specific settings, e.g. after 'filter' cmd: >> * the last_search tbs has their stats_enabled kept >> * tbs not in the list sets their flag to TB_PAUSED > > How does this affect anything at all? > > We are not *checking* the tb->tb_stats->stats_enabled bit except at code > generation time, not code execution time. Therefore nothing ever reads > the TB_PAUSED bit (or, correspondingly, the clearing of the other > bits). The setting of the bit is permanent. > At dump time, it does check stats_enabled e.g. in dump_tb_header(). So the question is whether FILTER is necessary at all? If not, we can remove FILTER together with PAUSE, and only keep START & STOP in hmp cmd.
Thanks, Fei. >> yes, it might looks better. But there is no correctness issue either as >> it checks if the specific bit is enabled during collecting stats. > > No, it does not. See above. > > > r~