Re: [PATCH v14 03/10] accel: collecting TB execution count

[Richard Henderson]

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.

> 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~