"Wu, Fei" <fei2...@intel.com> writes:
> On 4/6/2023 3:46 PM, Alex Bennée wrote:
>>
>> Fei Wu <fei2...@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.
See my reply to Richard but:
Subject: [PATCH v9 00/13] TCG code quality tracking and perf integration
Date: Mon, 7 Oct 2019 16:28:26 +0100
Message-Id: <20191007152839.30804-1-alex.ben...@linaro.org>
may be of interest?
>
> Thanks,
> Fei.
>
>> Richard, what do you think?
>>
>>>
>>> Signed-off-by: Fei Wu <fei2...@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;
>>
>>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
