diff mbox series

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

Message ID 20231008215147.3362612-2-rjones@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2] target/riscv: Use a direct cast for better performance | expand

Commit Message

Richard W.M. Jones Oct. 8, 2023, 9:50 p.m. UTC 
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(-)

Comments

Philippe Mathieu-Daudé Oct. 9, 2023, 10:23 a.m. UTC | #1 
Hi Richard,

On 8/10/23 23: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);

You might want to use:

        RISCVCPU *cpu =  env_archcpu(env);

Other occurences in target/riscv/internals.h.

> +    /*
> +     * 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;
>       RISCVExtStatus fs, vs;
>       uint32_t flags = 0;
>
LIU Zhiwei Oct. 9, 2023, 12:36 p.m. UTC | #2 
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,

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>       RISCVExtStatus fs, vs;
>       uint32_t flags = 0;
>
Richard W.M. Jones Oct. 9, 2023, 12:53 p.m. UTC | #3 
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) or here.  If you have
any suggestions I can try things.

> Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

I posted a v3 based on Philippe's feedback.

Rich.

> Zhiwei
> 
> >      RISCVExtStatus fs, vs;
> >      uint32_t flags = 0;
LIU Zhiwei Oct. 9, 2023, 12:58 p.m. UTC | #4 
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;
diff mbox series

Patch

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;
     RISCVExtStatus fs, vs;
     uint32_t flags = 0;