From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59843) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6wgm-00088A-RL for qemu-devel@nongnu.org; Wed, 07 Aug 2013 01:53:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V6wgh-0003dW-Na for qemu-devel@nongnu.org; Wed, 07 Aug 2013 01:53:40 -0400 Received: from mail-pb0-f51.google.com ([209.85.160.51]:48197) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6wgh-0003dG-ER for qemu-devel@nongnu.org; Wed, 07 Aug 2013 01:53:35 -0400 Received: by mail-pb0-f51.google.com with SMTP id jt11so1457457pbb.24 for ; Tue, 06 Aug 2013 22:53:34 -0700 (PDT) Message-ID: <5201E0D8.3040604@ozlabs.ru> Date: Wed, 07 Aug 2013 15:53:28 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1375323463-32747-1-git-send-email-afaerber@suse.de> <1375323463-32747-2-git-send-email-afaerber@suse.de> <52008FAE.8060709@ozlabs.ru> <5200B4C1.9010008@suse.de> <5201C0CA.2070808@ozlabs.ru> <5201DD73.1080001@ozlabs.ru> <5201DEDE.1020901@suse.de> In-Reply-To: <5201DEDE.1020901@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH for-next v2 01/22] object: Add OBJECT_GET_PARENT_CLASS() macro List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: Peter Crosthwaite , qemu-devel@nongnu.org, Anthony Liguori On 08/07/2013 03:45 PM, Andreas Färber wrote: > Am 07.08.2013 07:38, schrieb Alexey Kardashevskiy: >> On 08/07/2013 02:20 PM, Peter Crosthwaite wrote: >>> Hi, >>> >>> On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy wrote: >>>> On 08/06/2013 06:33 PM, Andreas Färber wrote: >>>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy: >>>>>> On 08/01/2013 12:17 PM, Andreas Färber wrote: >>>>>>> The object argument is currently unused and may be used to optimize the >>>>>>> class lookup when needed. >>>>>>> >>>>>>> Inspired-by: Peter Crosthwaite >>>>>>> Signed-off-by: Andreas Färber >>>>>>> --- >>>>>>> include/qom/object.h | 10 ++++++++++ >>>>>>> 1 file changed, 10 insertions(+) >>>>>>> >>>>>>> diff --git a/include/qom/object.h b/include/qom/object.h >>>>>>> index 23fc048..a8e71dc 100644 >>>>>>> --- a/include/qom/object.h >>>>>>> +++ b/include/qom/object.h >>>>>>> @@ -511,6 +511,16 @@ struct TypeInfo >>>>>>> OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name) >>>>>>> >>>>>>> /** >>>>>>> + * OBJECT_GET_PARENT_CLASS: >>>>>>> + * @obj: The object to obtain the parent class for. >>>>>>> + * @name: The QOM typename of @obj. >>>>>>> + * >>>>>>> + * Returns the parent class for a given object of a specific class. >>>>>>> + */ >>>>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>>>>> + object_class_get_parent(object_class_by_name(name)) >>>>>>> + >>>>>>> +/** >>>>>>> * InterfaceInfo: >>>>>>> * @type: The name of the interface. >>>>>>> * >>>>>>> >>>>>> >>>>>> Has anyone ever tried to use this macro? >>>>> >>>>> Since you're asking me, obviously later in this virtio series it's used >>>>> and in the IndustryPack series as well. >>>>> >>>>> I'm not aware of anyone else having used it yet - I'm still waiting for >>>>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it >>>>> on qom-next. >>>> >>>> >>>> Still do not understand what "obj" is doing here. >>> >>> The object is currently where cast cache optimizations are >>> implemented. So having a handle to it is useful if ever these >>> get-parent operations end up in fast paths and we need to do one of >>> Anthony's caching tricks. >>> >>>> This what I would suggest: >>>> >>>> #define OBJECT_GET_PARENT_CLASS(obj, name) \ >>>> object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name))) >>>> >>> >>> You have changed the semantic from what Andreas has implemented. This >>> will always return the parent of the concrete class, whereas to solve >>> the save-override-call problem you need to get the parent of abstract >>> level that is overriding the function (not always the concrete class). >>> >>>> @name here is just to make sure we are at the right level of the class >>>> hierarchy. >>>> >>> >>> Its @name that is actually important. Its more than just assertive, >>> variation of @name for the same object will and should return >>> different results (aside from just checking). The demonstrative case >>> for this requirement is TYPE_ARM_CPU, which has a realize fn that >>> needs to call the parent version as implemented by TYPE_CPU. ARM CPUs >>> however have a whole bunch of concrete classes inheriting from >>> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only >>> be able to get a handle to the parent of the concrete class (i.e. >>> TYPE_ARM_CPU) >> >> This is what I would really (really) expect in this situation. >> >>> and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as >>> intended. >> >> Oh. Then it is not get_parent() at all, this is get_grand_parent. > > No. It is parent of TYPE_ARM_CPU, as specified by the name argument. Here I lost you. You really want finalize() of TYPE_CPU, no matter how many classes are between TYPE_CPU and your final ARM CPU class. So call it directly, why do you need this workaround with get_parent if it is not really a parent of the current obj and you do not trust to what you get in finalize callback field of all intermediate classes (TYPE_CPU_ARM in this example)? Again, I am not arguing, I really that dump and do not understand :) > obj serves for possible optimization, as demonstrated by an earlier > patch by Peter that avoids the hashtable lookup by iterating over obj's > parent classes. If we pass obj down through our macros then such changes > can be done in one central place rather than reviewing and changing the > whole code base. >> realize() of TYPE_ARM_CPU_CORTEXTA9 (example) calls its parent >> (TYPE_ARM_CPU) realize() and that realize() calls its parent (TYPE_CPU) >> realize. >> >> It some level (for example, TYPE_ARM_CPU) does not want to implement >> realize(), then pointer to realize() from the upper class should be copied >> into TYPE_ARM_CPU's type info struct. class_init() callbacks are called for >> every class in the chain, so if some class_init() won't write to realize() >> pointer, it should be what the parent initialized it to, no? >> >> What do I miss here? > > You are missing that we do implement realize for TYPE_ARM_CPU, not for > its derived types. So we want to call TYPE_CPU's realize from there, not > some random realize determined by what type dev happens to have. How is it random? Inheritance is strict. It is CPU -> ARM_CPU -> A9_ARM_CPU so their finalize() should point to something reasonable or inherit from a parent. -- Alexey