Re: [PATCH v9 04/13] accel: replacing part of CONFIG_PROFILER with TBStats

[Richard Henderson]

On 10/7/19 11:28 AM, Alex Bennée wrote:
> +struct jit_profile_info {
> +    uint64_t translations;
> +    uint64_t aborted;
> +    uint64_t ops;
> +    unsigned ops_max;
> +    uint64_t del_ops;
> +    uint64_t temps;
> +    unsigned temps_max;
> +    uint64_t host;
> +    uint64_t guest;
> +    uint64_t search_data;
> +};

Hmm.  Maybe better to define a single structure that you can share between the
collection here and the storage in TCGProfile?

Also, "host" and "guest" are not helpful names...

> +    jpi->host += tbs->code.out_len;
> +    jpi->guest += tbs->code.in_len;

... code_int_len and code_out_len are helpful names.

> @@ -1807,6 +1805,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>          tb->tb_stats = NULL;
>      }
>  
> +
>      tcg_func_start(tcg_ctx);
>  
>      tcg_ctx->cpu = env_cpu(env);

Watch the random whitespace.

> +#define stat_per_translation(stat, name) \
> +    (stat->translations.total ? stat->name / stat->translations.total : 0)

Does this really need to go in the header?


> +static void collect_jit_profile_info(void *p, uint32_t hash, void *userp)
> +{
> +    struct jit_profile_info *jpi = userp;
> +    TBStatistics *tbs = p;
> +
> +    jpi->translations += tbs->translations.total;
> +    jpi->ops += tbs->code.num_tcg_ops;
> +    if (stat_per_translation(tbs, code.num_tcg_ops) > jpi->ops_max) {
> +        jpi->ops_max = stat_per_translation(tbs, code.num_tcg_ops);
> +    }
> +    jpi->del_ops += tbs->code.deleted_ops;
> +    jpi->temps += tbs->code.temps;
> +    if (stat_per_translation(tbs, code.temps) > jpi->temps_max) {
> +        jpi->temps_max = stat_per_translation(tbs, code.temps);
> +    }
> +    jpi->host += tbs->code.out_len;
> +    jpi->guest += tbs->code.in_len;
> +    jpi->search_data += tbs->code.search_out_len;
> +}

E.g.

    unsigned long total = tbs->translations.total;

    if (total) {
        unsigned tmp;

        tmp = tbs->code.num_tcg_ops / total;
        jpi->ops_max = MAX(jpi->ops_max, tmp);

        tmp = tbs->code.temps / total;
        jpi->temps_max = MAX(jpi->temps_max, tmp);
    }

It does occur to me to wonder why code.num_guest_inst etc are "unsigned" while
translations.total are "unsigned long", given that they are both accumulations
over many TBs.


r~