Re: [PATCH v2] target/riscv: Use a direct cast for better performance

[LIU Zhiwei <baxiantai@gmail.com>]

On 2023/10/9 20:53, Richard W.M. Jones wrote:
> On Mon, Oct 09, 2023 at 08:36:28PM +0800, LIU Zhiwei wrote:
>> On 2023/10/9 5:50, Richard W.M. Jones wrote:
>>> RISCV_CPU(cs) uses a checked cast.  When QOM cast debugging is enabled
>>> this adds about 5% total overhead when emulating RV64 on x86-64 host.
>>>
>>> Using a RISC-V guest with 16 vCPUs, 16 GB of guest RAM, virtio-blk
>>> disk.  The guest has a copy of the qemu source tree.  The test
>>> involves compiling the qemu source tree with 'make clean; time make -j16'.
>>>
>>> Before making this change the compile step took 449 & 447 seconds over
>>> two consecutive runs.
>>>
>>> After making this change, 428 & 422 seconds.
>>>
>>> The saving is about 5%.
>>>
>>> Thanks: Paolo Bonzini
>>> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
>>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>> ---
>>>   target/riscv/cpu_helper.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index 3a02079290..479d9863ae 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -66,7 +66,11 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, vaddr *pc,
>>>                             uint64_t *cs_base, uint32_t *pflags)
>>>   {
>>>       CPUState *cs = env_cpu(env);
>>> -    RISCVCPU *cpu = RISCV_CPU(cs);
>>> +    /*
>>> +     * Using the checked cast RISCV_CPU(cs) imposes ~ 5% overhead when
>>> +     * QOM cast debugging is enabled, so use a direct cast instead.
>>> +     */
>>> +    RISCVCPU *cpu = (RISCVCPU *)cs;
>> This function is very hot. Maybe we should cache the tbflags instead
>> of calculate it here. Otherwise,
> This function is indeed very hot, taking over 20% of total host time
> in my guest stress test.
>
> How would we cache the flags?  AIUI they simply depend on machine
> state and we must recalculate them either when the machine state
> changes (sprinkle "update_tbflags" everywhere)
Yes, we should do in this way.
>   or here.  If you have
> any suggestions I can try things.
I think it exceeds this patch.
>
>> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
> I posted a v3 based on Philippe's feedback.

OK.

Thanks
Zhiwei

>
> Rich.
>
>> Zhiwei
>>
>>>       RISCVExtStatus fs, vs;
>>>       uint32_t flags = 0;