[Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

A transition from CPUPPCState to PowerPCCPU can be considered safe,
just like PowerPCCPU::env access in the opposite direction.

This should slightly improve interrupt performance.

Reported-by: Anthony Liguori <aliguori@us.ibm.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-ppc/cpu-qom.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index eb03a00..f62181f 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -91,7 +91,7 @@ typedef struct PowerPCCPU {
 
 static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
 {
-    return POWERPC_CPU(container_of(env, PowerPCCPU, env));
+    return container_of(env, PowerPCCPU, env);
 }
 
 #define ENV_GET_CPU(e) CPU(ppc_env_get_cpu(e))
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Peter Maydell]

On 10 May 2013 15:39, Andreas Färber <afaerber@suse.de> wrote:
> A transition from CPUPPCState to PowerPCCPU can be considered safe,
> just like PowerPCCPU::env access in the opposite direction.
>
> This should slightly improve interrupt performance.

>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>  {
> -    return POWERPC_CPU(container_of(env, PowerPCCPU, env));
> +    return container_of(env, PowerPCCPU, env);
>  }

So if this is worthwhile shouldn't we be doing it for
all our CPUs?

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

Am 10.05.2013 16:42, schrieb Peter Maydell:
> On 10 May 2013 15:39, Andreas Färber <afaerber@suse.de> wrote:
>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>> just like PowerPCCPU::env access in the opposite direction.
>>
>> This should slightly improve interrupt performance.
> 
>>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>>  {
>> -    return POWERPC_CPU(container_of(env, PowerPCCPU, env));
>> +    return container_of(env, PowerPCCPU, env);
>>  }
> 
> So if this is worthwhile shouldn't we be doing it for
> all our CPUs?

I thought ppc were the exception, but you're right there's 15
occurrences remaining, i.e. all targets do it that way currently.

Don't have time right now for large cross-tree cleanups, so feel free to
profile with and without this patch.

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 for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Paolo Bonzini]

Il 10/05/2013 16:47, Andreas Färber ha scritto:
> Am 10.05.2013 16:42, schrieb Peter Maydell:
>> On 10 May 2013 15:39, Andreas Färber <afaerber@suse.de> wrote:
>>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>>> just like PowerPCCPU::env access in the opposite direction.
>>>
>>> This should slightly improve interrupt performance.
>>
>>>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>>>  {
>>> -    return POWERPC_CPU(container_of(env, PowerPCCPU, env));
>>> +    return container_of(env, PowerPCCPU, env);
>>>  }

This doesn't fix the same in problem in ENV_GET_CPU.  It would fix half
of the problem, but not the other half.

I don't want to think in advance of what casts are in hot paths.  The
less I think about such useless details, the less bug I will put in my
code.  I _want_ to code defensively (and have gdb pinpoint problems fast
while developing), I just don't want that to come at a performance cost
for users.

Algorithmic invariants are what you need to assert against.  As far as
type-safety is concerned, people are used to SIGSEGVs and it doesn't
change anything to them if they get SIGABRTs instead.  *If* the
type-safety is a major concern, at least patch 4 of my series should be
a no brainer.  And with the tracing support introduced by patch 5, the
price of having a little less type-safety should be more than acceptable.

>> So if this is worthwhile shouldn't we be doing it for
>> all our CPUs?
> 
> I thought ppc were the exception, but you're right there's 15
> occurrences remaining, i.e. all targets do it that way currently.
> 
> Don't have time right now for large cross-tree cleanups, so feel free to
> profile with and without this patch.

Well, we have a week to get this in.

Paolo

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 May 2013 15:39, Andreas Färber <afaerber@suse.de> wrote:
>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>> just like PowerPCCPU::env access in the opposite direction.
>>
>> This should slightly improve interrupt performance.
>
>>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>>  {
>> -    return POWERPC_CPU(container_of(env, PowerPCCPU, env));
>> +    return container_of(env, PowerPCCPU, env);
>>  }
>
> So if this is worthwhile shouldn't we be doing it for
> all our CPUs?

Ack.

Regards,

Anthony Liguori

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> A transition from CPUPPCState to PowerPCCPU can be considered safe,
> just like PowerPCCPU::env access in the opposite direction.
>
> This should slightly improve interrupt performance.
>
> Reported-by: Anthony Liguori <aliguori@us.ibm.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Another option would be to leave it and do something like:

diff --git a/qom/object.c b/qom/object.c
index 75e6aac..cba1d88 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -126,8 +126,13 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
 
 TypeImpl *type_register(const TypeInfo *info)
 {
+    TypeImpl *impl;
+
     assert(info->parent);
-    return type_register_internal(info);
+    impl = type_register_internal(info);
+    g_free(impl->name);
+    impl->name = info->name;
+    return impl;
 }
 
 TypeImpl *type_register_static(const TypeInfo *info)
@@ -449,10 +490,16 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename)
 ObjectClass *object_class_dynamic_cast(ObjectClass *class,
                                        const char *typename)
 {
-    TypeImpl *target_type = type_get_by_name(typename);
+    TypeImpl *target_type;
     TypeImpl *type = class->type;
     ObjectClass *ret = NULL;
 
+    if (type->name == typename) {
+        return class;
+    }
+
+    target_type = type_get_by_name(typename);
+
     if (!target_type) {
         /* target class type unknown, so fail the cast */
         return NULL;

Which makes casting an object to it's concrete class free.

Regards,

Anthony Liguori

> ---
>  target-ppc/cpu-qom.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index eb03a00..f62181f 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -91,7 +91,7 @@ typedef struct PowerPCCPU {
>  
>  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
>  {
> -    return POWERPC_CPU(container_of(env, PowerPCCPU, env));
> +    return container_of(env, PowerPCCPU, env);
>  }
>  
>  #define ENV_GET_CPU(e) CPU(ppc_env_get_cpu(e))
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Paolo Bonzini]

Il 10/05/2013 17:06, Anthony Liguori ha scritto:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>> just like PowerPCCPU::env access in the opposite direction.
>>
>> This should slightly improve interrupt performance.
>>
>> Reported-by: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Another option would be to leave it and do something like:

... patch 3 in my series. ;)

Paolo

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

[resending after bounce]

Am 10.05.2013 17:06, schrieb Anthony Liguori:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>> just like PowerPCCPU::env access in the opposite direction.
>>
>> This should slightly improve interrupt performance.
>>
>> Reported-by: Anthony Liguori <aliguori@us.ibm.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> 
> Another option would be to leave it and do something like:
> 
> diff --git a/qom/object.c b/qom/object.c
> index 75e6aac..cba1d88 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -126,8 +126,13 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
>  
>  TypeImpl *type_register(const TypeInfo *info)
>  {
> +    TypeImpl *impl;
> +
>      assert(info->parent);
> -    return type_register_internal(info);
> +    impl = type_register_internal(info);
> +    g_free(impl->name);
> +    impl->name = info->name;
> +    return impl;
>  }
>  
>  TypeImpl *type_register_static(const TypeInfo *info)
> @@ -449,10 +490,16 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename)
>  ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>                                         const char *typename)
>  {
> -    TypeImpl *target_type = type_get_by_name(typename);
> +    TypeImpl *target_type;
>      TypeImpl *type = class->type;
>      ObjectClass *ret = NULL;
>  
> +    if (type->name == typename) {
> +        return class;
> +    }
> +
> +    target_type = type_get_by_name(typename);
> +
>      if (!target_type) {
>          /* target class type unknown, so fail the cast */
>          return NULL;
> 
> Which makes casting an object to it's concrete class free.

Doesn't help here since concrete class is POWER7_v2.1-ppc64-cpu whereas
we're casting to ppc64-cpu, with two-level hierarchy by now:
POWER7_v2.1 -> POWER7 -> ppc64 -> device -> object.

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

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

Am 10.05.2013 17:32, schrieb Alexander Graf:
> 
> On 10.05.2013, at 17:23, Andreas Färber wrote:
> 
>> Am 10.05.2013 17:06, schrieb Anthony Liguori:
>>> Andreas Färber <afaerber@suse.de> writes:
>>>
>>>> A transition from CPUPPCState to PowerPCCPU can be considered safe,
>>>> just like PowerPCCPU::env access in the opposite direction.
>>>>
>>>> This should slightly improve interrupt performance.
>>>>
>>>> Reported-by: Anthony Liguori <aliguori@us.ibm.com>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>
>>> Another option would be to leave it and do something like:
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index 75e6aac..cba1d88 100644
>>> --- a/qom/object.c
>>> +++ b/qom/object.c
>>> @@ -126,8 +126,13 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
>>>
>>> TypeImpl *type_register(const TypeInfo *info)
>>> {
>>> +    TypeImpl *impl;
>>> +
>>>     assert(info->parent);
>>> -    return type_register_internal(info);
>>> +    impl = type_register_internal(info);
>>> +    g_free(impl->name);
>>> +    impl->name = info->name;
>>> +    return impl;
>>> }
>>>
>>> TypeImpl *type_register_static(const TypeInfo *info)
>>> @@ -449,10 +490,16 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename)
>>> ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>>>                                        const char *typename)
>>> {
>>> -    TypeImpl *target_type = type_get_by_name(typename);
>>> +    TypeImpl *target_type;
>>>     TypeImpl *type = class->type;
>>>     ObjectClass *ret = NULL;
>>>
>>> +    if (type->name == typename) {
>>> +        return class;
>>> +    }
>>> +
>>> +    target_type = type_get_by_name(typename);
>>> +
>>>     if (!target_type) {
>>>         /* target class type unknown, so fail the cast */
>>>         return NULL;
>>>
>>> Which makes casting an object to it's concrete class free.
>>
>> Doesn't help here since concrete class is POWER7_v2.1-ppc64-cpu whereas
>> we're casting to ppc64-cpu, with two-level hierarchy by now:
>> POWER7_v2.1 -> POWER7 -> ppc64 -> device -> object.
> 
> How much performance penalty do we get from this?

Not sure which "this" you are referring to, but in general dynamic_cast
does a check for interfaces (which we don't have) and then iterates
through the hierarchy with string comparisons, i.e. negative, negative,
positive for POWERPC_CPU(). My original patch here dropped this penalty
for ppc_env_get_cpu(); CPU() would still result in negative, negative,
negative, positive.

Personally I wouldn't oppose dropping these checks for release builds as
proposed by Paolo in his series; for me, the value of POWERPC_CPU() is
being closer to an OO cast than any container_of()-style expressions.

But I can also see Anthony's point that we should try to optimize
dynamic_cast rather than circumventing it.

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 for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Peter Maydell]

On 10 May 2013 17:14, Andreas Färber <afaerber@suse.de> wrote:
> Personally I wouldn't oppose dropping these checks for release builds as
> proposed by Paolo in his series; for me, the value of POWERPC_CPU() is
> being closer to an OO cast than any container_of()-style expressions.
>
> But I can also see Anthony's point that we should try to optimize
> dynamic_cast rather than circumventing it.

I don't think we should be doing anything dynamically at all.
We know at compile time that we've been passed a CPUPPCState*,
and we know that we always get from that to a CPUState*
by subtracting a compile-time-constant offset. Nothing about
this is dynamic at all and anything we do at runtime beyond
that subtraction is pure overhead.

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-1.5?] target-ppc: Drop unnecessary dynamic cast in ppc_env_get_cpu()

[Andreas Färber]

Am 10.05.2013 18:20, schrieb Peter Maydell:
> On 10 May 2013 17:14, Andreas Färber <afaerber@suse.de> wrote:
>> Personally I wouldn't oppose dropping these checks for release builds as
>> proposed by Paolo in his series; for me, the value of POWERPC_CPU() is
>> being closer to an OO cast than any container_of()-style expressions.
>>
>> But I can also see Anthony's point that we should try to optimize
>> dynamic_cast rather than circumventing it.
> 
> I don't think we should be doing anything dynamically at all.
> We know at compile time that we've been passed a CPUPPCState*,
> and we know that we always get from that to a CPUState*
> by subtracting a compile-time-constant offset. Nothing about
> this is dynamic at all and anything we do at runtime beyond
> that subtraction is pure overhead.

No one doubts that for CPU.

But if you think of AXI, I2C and the general connecter inheritance vs.
aggregation discussion, it becomes much more hairy! In particular
Anthony's solution to the I/O port VGA vs. ISA problem was dropping
ISADevice as base type and turning ISA into an interface on a chipset
object. Then you may know which interfaces are available on your device,
but you still need some special non-C cast, aka dynamic_cast.

So in the end we want C++ (replace with favorite native OO language) but
we can't switch because not all devices are QOM'ified yet, and when
QOM'ifying them we get complaints about bad QOM performance.
Either we ignore performance hits and hurry up with the conversion or we
address performance hits to at least some degree, otherwise we're not
going to reach a satisfactory solution...

Andreas

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