[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v14 03/10] accel: collecting TB execution count
From: |
Wu, Fei |
Subject: |
Re: [PATCH v14 03/10] accel: collecting TB execution count |
Date: |
Fri, 2 Jun 2023 09:54:31 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 |
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" <vandersonmr2@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 <vandersonmr2@gmail.com>
>>>> Message-Id: <20190829173437.5926-3-vandersonmr2@gmail.com>
>>>> [AJB: Fix author]
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Signed-off-by: Fei Wu <fei2.wu@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~