[Qemu-devel] [PATCH qom-cpu for-2.0] cpu: Avoid QOM casts for CPU()

[Qemu-devel] [PATCH qom-cpu for-2.0] cpu: Avoid QOM casts for CPU()

[Andreas Färber]

CPU address spaces touching load and store helpers as well as the
movement of (almost) all fields from CPU_COMMON to CPUState have led to
a noticeable increase of CPU() usage in "hot" paths for both TCG and KVM.

While CPU()'s OBJECT_CHECK() might help detect development errors, i.e. in
form of crashes due to QOM vs. non-QOM mismatches rather than QOM type
mismatches, it is not really needed at runtime since mostly used in
CPU-specific paths, coming from a target-specific CPU subtype. If that
pointer is damaged, other errors are highly likely occur elsewhere anyway.

Keep the CPU() macro for a consistent developer experience and
flexibility to exchange its implementation, but turn it into a pure
C cast for now.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qom/cpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index f99885a..0aa1bdc 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -53,7 +53,7 @@ typedef uint64_t vaddr;
 
 #define TYPE_CPU "cpu"
 
-#define CPU(obj) OBJECT_CHECK(CPUState, (obj), TYPE_CPU)
+#define CPU(obj) ((CPUState *)(obj))
 #define CPU_CLASS(class) OBJECT_CLASS_CHECK(CPUClass, (class), TYPE_CPU)
 #define CPU_GET_CLASS(obj) OBJECT_GET_CLASS(CPUClass, (obj), TYPE_CPU)
 
-- 
1.8.4.5

Re: [Qemu-devel] [PATCH qom-cpu for-2.0] cpu: Avoid QOM casts for CPU()

[Peter Maydell]

On 28 March 2014 15:49, Andreas Färber <afaerber@suse.de> wrote:
> CPU address spaces touching load and store helpers as well as the
> movement of (almost) all fields from CPU_COMMON to CPUState have led to
> a noticeable increase of CPU() usage in "hot" paths for both TCG and KVM.
>
> While CPU()'s OBJECT_CHECK() might help detect development errors, i.e. in
> form of crashes due to QOM vs. non-QOM mismatches rather than QOM type
> mismatches, it is not really needed at runtime since mostly used in
> CPU-specific paths, coming from a target-specific CPU subtype. If that
> pointer is damaged, other errors are highly likely occur elsewhere anyway.
>
> Keep the CPU() macro for a consistent developer experience and
> flexibility to exchange its implementation, but turn it into a pure
> C cast for now.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  include/qom/cpu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index f99885a..0aa1bdc 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -53,7 +53,7 @@ typedef uint64_t vaddr;
>
>  #define TYPE_CPU "cpu"
>
> -#define CPU(obj) OBJECT_CHECK(CPUState, (obj), TYPE_CPU)
> +#define CPU(obj) ((CPUState *)(obj))
>  #define CPU_CLASS(class) OBJECT_CLASS_CHECK(CPUClass, (class), TYPE_CPU)
>  #define CPU_GET_CLASS(obj) OBJECT_GET_CLASS(CPUClass, (obj), TYPE_CPU)
>
> --
> 1.8.4.5

Not important if we're going to revert this as soon as 2.0
is out of the door, but if we expect this to be around for
longer than that it would be worth a brief comment explaining
why this cast macro is a special case.

thanks
-- PMM

Re: [Qemu-devel] [PATCH qom-cpu for-2.0] cpu: Avoid QOM casts for CPU()

[Andreas Färber]

Am 28.03.2014 16:52, schrieb Peter Maydell:
> On 28 March 2014 15:49, Andreas Färber <afaerber@suse.de> wrote:
>> CPU address spaces touching load and store helpers as well as the
>> movement of (almost) all fields from CPU_COMMON to CPUState have led to
>> a noticeable increase of CPU() usage in "hot" paths for both TCG and KVM.
>>
>> While CPU()'s OBJECT_CHECK() might help detect development errors, i.e. in
>> form of crashes due to QOM vs. non-QOM mismatches rather than QOM type
>> mismatches, it is not really needed at runtime since mostly used in
>> CPU-specific paths, coming from a target-specific CPU subtype. If that
>> pointer is damaged, other errors are highly likely occur elsewhere anyway.

"to occur"

>>
>> Keep the CPU() macro for a consistent developer experience and

"and for"

>> flexibility to exchange its implementation, but turn it into a pure
>> C cast for now.
>>
>> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  include/qom/cpu.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index f99885a..0aa1bdc 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -53,7 +53,7 @@ typedef uint64_t vaddr;
>>
>>  #define TYPE_CPU "cpu"
>>
>> -#define CPU(obj) OBJECT_CHECK(CPUState, (obj), TYPE_CPU)

/* Avoid unnecessary penalties in hot paths by using an unchecked cast. */ ?

>> +#define CPU(obj) ((CPUState *)(obj))
>>  #define CPU_CLASS(class) OBJECT_CLASS_CHECK(CPUClass, (class), TYPE_CPU)
>>  #define CPU_GET_CLASS(obj) OBJECT_GET_CLASS(CPUClass, (obj), TYPE_CPU)
>>
>> --
>> 1.8.4.5
> 
> Not important if we're going to revert this as soon as 2.0
> is out of the door, but if we expect this to be around for
> longer than that it would be worth a brief comment explaining
> why this cast macro is a special case.

Right now I don't see a post-2.0 solution emerging, so yes,
documentation is good.

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom-cpu for-2.0] cpu: Avoid QOM casts for CPU()

[Andreas Färber]

Am 28.03.2014 18:10, schrieb Andreas Färber:
> Am 28.03.2014 16:52, schrieb Peter Maydell:
>> On 28 March 2014 15:49, Andreas Färber <afaerber@suse.de> wrote:
>>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>>> index f99885a..0aa1bdc 100644
>>> --- a/include/qom/cpu.h
>>> +++ b/include/qom/cpu.h
>>> @@ -53,7 +53,7 @@ typedef uint64_t vaddr;
>>>
>>>  #define TYPE_CPU "cpu"
>>>
>>> -#define CPU(obj) OBJECT_CHECK(CPUState, (obj), TYPE_CPU)
> 
> /* Avoid unnecessary penalties in hot paths by using an unchecked cast. */ ?

Based on a suggestion from Peter on IRC extending this to:

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 0aa1bdc..df977c8 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -53,7 +53,12 @@ typedef uint64_t vaddr;

 #define TYPE_CPU "cpu"

+/* Since this macro is used a lot in hot code paths and in conjunction with
+ * FooCPU *foo_env_get_cpu(), we deviate from usual QOM practice by using
+ * an unchecked cast.
+ */
 #define CPU(obj) ((CPUState *)(obj))
+
 #define CPU_CLASS(class) OBJECT_CLASS_CHECK(CPUClass, (class), TYPE_CPU)
 #define CPU_GET_CLASS(obj) OBJECT_GET_CLASS(CPUClass, (obj), TYPE_CPU)


Thanks, applied to qom-cpu:
https://github.com/afaerber/qemu-cpu/commits/qom-cpu

Pull coming up shortly.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg