Re: [Qemu-devel] [PATCH v2 3/3] trace: instrument and trace tcg tb flush activity

[Alex Bennée]

Andreas Färber writes:

> Hi,
>
> Am 15.07.2014 13:42, schrieb Alex Bennée:
<snip>
>> index df977c8..8376678 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -243,6 +243,10 @@ struct CPUState {
>>      void *env_ptr; /* CPUArchState */
>>      struct TranslationBlock *current_tb;
>>      struct TranslationBlock *tb_jmp_cache[TB_JMP_CACHE_SIZE];
>> +    struct {
>> +        int     hits;
>> +        int     misses;
>
> Is anything else going to be added here? If not, the indentation can be
> dropped.

At the moment probably not.

>
>> +    } tb_jmp_cache_stats;
>
> This is lacking documentation. Should be trivial to add for this field
> (not here, above the struct). To document the subfields we may need to
> name the struct.

Would not a simple comment be enough?

>
>>      struct GDBRegisterState *gdb_regs;
>>      int gdb_num_regs;
>>      int gdb_num_g_regs;
>> @@ -584,6 +588,15 @@ void cpu_exit(CPUState *cpu);
>>   */
>>  void cpu_resume(CPUState *cpu);
>>  
>> +
>> +/**
>> + * tb_flush_all_jmp_cache:
>> + * @cpu: The CPU jmp cache to flush
>> + *
>> + * Flush all the entries from the cpu fast jump cache
>
> "CPU" for consistency

OK

>
<snip>
>> +#ifndef CONFIG_TRACE_NOP
>> +static inline void trace_inc_counter(int *counter) {
>> +    int cnt = *counter;
>> +    cnt++;
>> +    *counter = cnt;
>> +}
>> +#else
>> +static inline void trace_inc_counter(int *counter) { /* do nothing */ }
>> +#endif
>> +
>>  #endif  /* TRACE_H */
>
> Coding Style issues with the first function. For simplicity just keep
> the first implementation but with the proper brace placement, and then
> just put the #ifdef into the function body. That avoids the signatures
> getting out of sync.

mea-culpa, I forgot to run checkpatch.pl....
>>  
>>      memset(tcg_ctx.tb_ctx.tb_phys_hash, 0, 
>> sizeof(tcg_ctx.tb_ctx.tb_phys_hash));
>> @@ -1520,6 +1530,8 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong 
>> addr)
>>      i = tb_jmp_cache_hash_page(addr);
>>      memset(&cpu->tb_jmp_cache[i], 0,
>>             TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
>
> Can this one be dropped, too?

No, this is only a partial invalidation. I did toy with instrumenting
how many entries are flushed but it didn't seem the be worth it given
with the current architecture there is not much we can do.

>
>> +
>> +    trace_tb_flush_jump_cache(addr);
>>  }
>>  
>>  void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
>
> Cheers,
> Andreas

-- 
Alex Bennée