On 5/3/2023 4:12 PM, Richard Henderson wrote:
> On 4/21/23 14:24, Fei Wu wrote:
>> From: "Vanderson M. do Rosario" <vanderson...@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.ben...@linaro.org>
>> Signed-off-by: Vanderson M. do Rosario <vanderson...@gmail.com>
>> Message-Id: <20190829173437.5926-2-vanderson...@gmail.com>
>> [AJB: fix git author, review comments]
>> Signed-off-by: Alex Bennée <alex.ben...@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~
