Re: [PATCH v9 03/13] accel: collecting JIT statistics

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 10/7/19 11:28 AM, Alex Bennée wrote:
>> @@ -1795,6 +1799,10 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>>          if (flag & TB_EXEC_STATS) {
>>              tb->tb_stats->stats_enabled |= TB_EXEC_STATS;
>>          }
>> +
>> +        if (flag & TB_JIT_STATS) {
>> +            tb->tb_stats->stats_enabled |= TB_JIT_STATS;
>> +        }
>
> So, assuming that you really meant this, and not replacing as I was guessing vs
> patch 2, then this is
>
>     tb->tb_stats->stats_enabled |=
>         flag & (TB_EXEC_STATS | TB_JIT_STATS);
>
> But I still think it's weird to be wanting to modify the shared structure.
> What does that mean for concurrent code generation?

The idea was to have per translation area granularity on collecting the
stats so we didn't have to burden all areas with the overhead. Currently
this only takes effect when qemu_log_in_addr_range is in effect. However
as the run goes on we could make decisions to disable some or all stats
for stuff that doesn't come up that frequently.

However the current positioning doesn't work as we keep setting the flag
so I think we need to apply get_default_tbstats_flag() inside
tb_get_stats only when we first create the data block.

>
>> +    /*
>> +     * Collect JIT stats when enabled. We batch them all up here to
>> +     * avoid spamming the cache with atomic accesses
>> +     */
>> +    if (tb_stats_enabled(tb, TB_JIT_STATS)) {
>> +        TBStatistics *ts = tb->tb_stats;
>> +        qemu_mutex_lock(&ts->jit_stats_lock);
>> +
>> +        ts->code.num_guest_inst += prof->translation.nb_guest_insns;
>> +        ts->code.num_tcg_ops += prof->translation.nb_ops_pre_opt;
>> +        ts->code.num_tcg_ops_opt += tcg_ctx->nb_ops;
>> +        ts->code.spills += prof->translation.nb_spills;
>> +        ts->code.out_len += tb->tc.size;
>> +
>> +        ts->translations.total++;
>> +        if (phys_page2 != -1) {
>> +            ts->translations.spanning++;
>> +        }
>> +
>> +        g_ptr_array_add(ts->tbs, tb);
>> +
>> +        qemu_mutex_unlock(&ts->jit_stats_lock);
>> +    }
>
> Hmm.  So we're to interpret all of code.field as sums, the average of which can
> be obtained by dividing by translations.total.  Ok, but that should probably be
> mentioned in a comment in/near the structure definition.

OK

> What are we planning to do with the set of all tb's collected here?

Originally we kept track for the coverset calculation as we need to know
where each individual TB goes next. The code was racy so I dropped it
from the series so tracking this now is possibly redundant although it
might be useful in the future.

>
>> @@ -3125,6 +3126,7 @@ static void temp_sync
>>          case TEMP_VAL_REG:
>>              tcg_out_st(s, ts->type, ts->reg,
>>                         ts->mem_base->reg, ts->mem_offset);
>> +            s->prof.translation.nb_spills++;
>>              break;
>>  
>>          case TEMP_VAL_MEM:
>
> This is not a spill in the common compiler definition.
>
> This is "write the temp to its backing storage".  While this does happen in the
> course of spilling, the vast majority of these are because we've finished
> modifying a global temp and must now update memory.  Which is not nearly the
> same thing as "spill".
>
> A spill in the compiler definition happens in tcg_reg_alloc, right after the
> comment "We must spill something".  ;-)

OK I'll fix that.

-- 
Alex Bennée