Re: [PATCH 1/2] accel/tcg/plugin: export host insn size

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

On 4/6/2023 3:46 PM, Alex Bennée wrote:
> 
> Fei Wu <fei2.wu@intel.com> writes:
> 
>> The translation ratio of host to guest instruction count is one of the
>> key performance factor of binary translation. TCG doesn't collect host
>> instruction count at present, it does collect host instruction size
>> instead, although they are not the same thing as instruction size might
>> not be fixed, instruction size is still a valid estimation.
> 
> I'm not so sure about exposing this information to plugins because we
> try to avoid leaking internal implementation details to plugins. Aside
> from that the very act of instrumenting will increase the size of the
> target buffer.
> 
> If your aim is to examine JIT efficiency what is wrong with the current
> "info jit" that you can access via the HMP? Also I'm wondering if its
> time to remove the #ifdefs from CONFIG_PROFILER because I doubt the
> extra data it collects is that expensive.
> 
"info jit" collects the translation time expansion ratio, it doesn't
distinguish between hot and cold blocks:
    TB avg target size  14 max=1918 bytes
    TB avg host size    287 bytes (expansion ratio: 19.7)

My primary aim is to collect the runtime expansion ratio, so hot blocks
weigh more than cold blocks. My concern is this series might not be the
proper way to implement it, just as you mentioned in another reply.

Thanks,
Fei.

> Richard, what do you think?
> 
>>
>> Signed-off-by: Fei Wu <fei2.wu@intel.com>
>> ---
>>  accel/tcg/plugin-gen.c       | 1 +
>>  include/qemu/plugin.h        | 2 ++
>>  include/qemu/qemu-plugin.h   | 8 ++++++++
>>  plugins/api.c                | 5 +++++
>>  plugins/qemu-plugins.symbols | 1 +
>>  5 files changed, 17 insertions(+)
>>
>> diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
>> index 5efb8db258..4a3ca8fa2f 100644
>> --- a/accel/tcg/plugin-gen.c
>> +++ b/accel/tcg/plugin-gen.c
>> @@ -881,6 +881,7 @@ bool plugin_gen_tb_start(CPUState *cpu, const DisasContextBase *db,
>>          ptb->haddr2 = NULL;
>>          ptb->mem_only = mem_only;
>>          ptb->mem_helper = false;
>> +        ptb->host_insn_size = &db->tb->tc.size;
>>  
>>          plugin_gen_empty_callback(PLUGIN_GEN_FROM_TB);
>>      }
>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
>> index bc0781cab8..b38fd139e1 100644
>> --- a/include/qemu/plugin.h
>> +++ b/include/qemu/plugin.h
>> @@ -151,6 +151,8 @@ struct qemu_plugin_tb {
>>      /* if set, the TB calls helpers that might access guest memory */
>>      bool mem_helper;
>>  
>> +    uint64_t *host_insn_size;
>> +
>>      GArray *cbs[PLUGIN_N_CB_SUBTYPES];
>>  };
>>  
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index 50a9957279..2397574a21 100644
>> --- a/include/qemu/qemu-plugin.h
>> +++ b/include/qemu/qemu-plugin.h
>> @@ -336,6 +336,14 @@ void qemu_plugin_register_vcpu_insn_exec_inline(struct qemu_plugin_insn *insn,
>>   */
>>  size_t qemu_plugin_tb_n_insns(const struct qemu_plugin_tb *tb);
>>  
>> +/**
>> + * qemu_plugin_tb_n_insns() - query helper for host insns size in TB
>> + * @tb: opaque handle to TB passed to callback
>> + *
>> + * Returns: address of host insns size of this block
> 
> If we went ahead with this we need to be very clear when you can call
> this helper because the data will only be valid at certain points (which
> is another argument against this).
> 
>> + */
>> +void *qemu_plugin_tb_host_insn_size(const struct qemu_plugin_tb *tb);
>> +
>>  /**
>>   * qemu_plugin_tb_vaddr() - query helper for vaddr of TB start
>>   * @tb: opaque handle to TB passed to callback
>> diff --git a/plugins/api.c b/plugins/api.c
>> index 2078b16edb..0d70cb1f0f 100644
>> --- a/plugins/api.c
>> +++ b/plugins/api.c
>> @@ -188,6 +188,11 @@ size_t qemu_plugin_tb_n_insns(const struct qemu_plugin_tb *tb)
>>      return tb->n;
>>  }
>>  
>> +void *qemu_plugin_tb_host_insn_size(const struct qemu_plugin_tb *tb)
>> +{
>> +    return tb->host_insn_size;
>> +}
>> +
>>  uint64_t qemu_plugin_tb_vaddr(const struct qemu_plugin_tb *tb)
>>  {
>>      return tb->vaddr;
>> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
>> index 71f6c90549..3e92c3b8ba 100644
>> --- a/plugins/qemu-plugins.symbols
>> +++ b/plugins/qemu-plugins.symbols
>> @@ -39,6 +39,7 @@
>>    qemu_plugin_start_code;
>>    qemu_plugin_tb_get_insn;
>>    qemu_plugin_tb_n_insns;
>> +  qemu_plugin_tb_host_insn_size;
>>    qemu_plugin_tb_vaddr;
>>    qemu_plugin_uninstall;
>>    qemu_plugin_vcpu_for_each;
> 
>