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
I guess 'default' means the flag for creating new tbstats.
>> @@ -295,6 +295,7 @@ static TBStatistics *tb_get_stats(tb_page_addr_t
>> phys_pc, target_ulong pc,
>> new_stats->pc = pc;
>> new_stats->cs_base = cs_base;
>> new_stats->flags = flags;
>> + new_stats->stats_enabled = get_default_tbstats_flag();
>
> Is this merely to record how we have generated a given TB?
> What is the purpose of this flag over the global variable?
>
see above.
>> diff --git a/include/exec/tb-stats-flags.h
>> b/include/exec/tb-stats-flags.h
>> index 87ee3d902e..fa71eb6f0c 100644
>> --- a/include/exec/tb-stats-flags.h
>> +++ b/include/exec/tb-stats-flags.h
>> @@ -11,6 +11,9 @@
>> #ifndef TB_STATS_FLAGS
>> #define TB_STATS_FLAGS
>> +#define TB_NOTHING (1 << 0)
>> +#define TB_EXEC_STATS (1 << 1)
>
> Why is NOTHING a non-zero flag?
>
yes, it might looks better. But there is no correctness issue either as
it checks if the specific bit is enabled during collecting stats.
>> --- a/include/exec/tb-stats.h
>> +++ b/include/exec/tb-stats.h
>> @@ -31,6 +31,9 @@
>> #include "exec/tb-stats-flags.h"
>> #include "tcg/tcg.h"
>> +#define tb_stats_enabled(tb, JIT_STATS) \
>> + (tb && tb->tb_stats && (tb->tb_stats->stats_enabled & JIT_STATS))
>
> Inline function, though again, why stats_enabled vs global variable?
>
will inline it.
Thanks,
Fei.
>
> r~
