Re: [PATCH v11 01/14] accel/tcg: introduce TBStatistics structure

["Wu, Fei" <fei2.wu@intel.com>]

On 5/3/2023 4:12 PM, Richard Henderson wrote:
> On 4/21/23 14:24, Fei Wu wrote:
>> From: "Vanderson M. do Rosario" <vandersonmr2@gmail.com>
>>
>> To store statistics for each TB, we created a TBStatistics structure
>> which is linked with the TBs. TBStatistics can stay alive after
>> tb_flush and be relinked to a regenerated TB. So the statistics can
>> be accumulated even through flushes.
>>
>> The goal is to have all present and future qemu/tcg statistics and
>> meta-data stored in this new structure.
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Vanderson M. do Rosario <vandersonmr2@gmail.com>
>> Message-Id: <20190829173437.5926-2-vandersonmr2@gmail.com>
>> [AJB: fix git author, review comments]
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   accel/tcg/meson.build     |  1 +
>>   accel/tcg/tb-context.h    |  1 +
>>   accel/tcg/tb-hash.h       |  7 +++++
>>   accel/tcg/tb-maint.c      | 19 ++++++++++++
>>   accel/tcg/tb-stats.c      | 58 +++++++++++++++++++++++++++++++++++++
>>   accel/tcg/translate-all.c | 43 +++++++++++++++++++++++++++
>>   include/exec/exec-all.h   |  3 ++
>>   include/exec/tb-stats.h   | 61 +++++++++++++++++++++++++++++++++++++++
>>   8 files changed, 193 insertions(+)
>>   create mode 100644 accel/tcg/tb-stats.c
>>   create mode 100644 include/exec/tb-stats.h
>>
>> diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
>> index aeb20a6ef0..9263bdde11 100644
>> --- a/accel/tcg/meson.build
>> +++ b/accel/tcg/meson.build
>> @@ -4,6 +4,7 @@ tcg_ss.add(files(
>>     'cpu-exec-common.c',
>>     'cpu-exec.c',
>>     'tb-maint.c',
>> +  'tb-stats.c',
>>     'tcg-runtime-gvec.c',
>>     'tcg-runtime.c',
>>     'translate-all.c',
>> diff --git a/accel/tcg/tb-context.h b/accel/tcg/tb-context.h
>> index cac62d9749..d7910d586b 100644
>> --- a/accel/tcg/tb-context.h
>> +++ b/accel/tcg/tb-context.h
>> @@ -35,6 +35,7 @@ struct TBContext {
>>       /* statistics */
>>       unsigned tb_flush_count;
>>       unsigned tb_phys_invalidate_count;
>> +    struct qht tb_stats;
>>   };
>>     extern TBContext tb_ctx;
>> diff --git a/accel/tcg/tb-hash.h b/accel/tcg/tb-hash.h
>> index 83dc610e4c..87d657a1c6 100644
>> --- a/accel/tcg/tb-hash.h
>> +++ b/accel/tcg/tb-hash.h
>> @@ -67,4 +67,11 @@ uint32_t tb_hash_func(tb_page_addr_t phys_pc,
>> target_ulong pc, uint32_t flags,
>>       return qemu_xxhash7(phys_pc, pc, flags, cf_mask,
>> trace_vcpu_dstate);
>>   }
>>   +static inline
>> +uint32_t tb_stats_hash_func(tb_page_addr_t phys_pc, target_ulong pc,
>> +                            uint32_t flags)
>> +{
>> +    return qemu_xxhash5(phys_pc, pc, flags);
>> +}
>> +
> 
> Why are you avoiding the hash of cs_base?
> It certainly changes the comparison for a number of guests.
> 
Just as you mentioned below, it's checked during compare. There is a
comment in TBStatistics definition.

>> +/*
>> + * This is the more or less the same compare as tb_cmp(), but the
>> + * data persists over tb_flush. We also aggregate the various
>> + * variations of cflags under one record and ignore the details of
>> + * page overlap (although we can count it).
>> + */
>> +bool tb_stats_cmp(const void *ap, const void *bp)
>> +{
>> +    const TBStatistics *a = ap;
>> +    const TBStatistics *b = bp;
>> +
>> +    return a->phys_pc == b->phys_pc &&
>> +        a->pc == b->pc &&
>> +        a->cs_base == b->cs_base &&
>> +        a->flags == b->flags;
>> +}
> 
> So, comparing cs_base, but not hashing it?
> 
Yes.

>> +void disable_collect_tb_stats(void)
>> +{
>> +    tcg_collect_tb_stats = TB_STATS_PAUSED;
>> +}
>> +
>> +void pause_collect_tb_stats(void)
>> +{
>> +    tcg_collect_tb_stats = TB_STATS_STOPPED;
>> +}
> 
> These two seem swapped.
> 
Yes, it seems so, I will update it.

Thanks,
Fei.

> 
> r~