| Message ID | 87obcizxgk.fsf@codemonkey.ws |
|---|---|
| State | New |
| Headers | show |
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
[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
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
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
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
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;