diff mbox

[RFC,v0,6/8] xilinx dont cast to interface types with links

Message ID 4FD86E40.1000100@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini June 13, 2012, 10:41 a.m. UTC 
Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
> Something is broken with casting to an interface type then setting a link. Cast
> these two to object for linking instead and it all works.

I don't have a board image but I don't see anything visibly bad without this patch.

However, something that _is_ bad indeed happens; we try to ref/unref an interface
object.  This patch fixes it while keeping things efficient (and in fact fixes one
TODO).  Can add a link to an image on http://wiki.qemu.org/Testing and/or test it?

Anthony, is this above your disgust level?

Paolo


------------------------ 8< -----------------------


Paolo

Comments

Avi Kivity June 13, 2012, 10:55 a.m. UTC | #1 
On 06/13/2012 01:41 PM, Paolo Bonzini wrote:
> 
> However, something that _is_ bad indeed happens; we try to ref/unref an interface
> object.  This patch fixes it while keeping things efficient (and in fact fixes one
> TODO).  Can add a link to an image on http://wiki.qemu.org/Testing and/or test it?
> 
> Anthony, is this above your disgust level?
> 
> Paolo
> 
> 
> ------------------------ 8< -----------------------
> 
> diff --git a/qom/object.c b/qom/object.c
> index c3a7a47..bd60838 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -63,7 +63,7 @@ struct TypeImpl
>      InterfaceImpl interfaces[MAX_INTERFACES];
>  };
>  
> -#define INTERFACE(obj) INTERFACE_CHECK(obj, TYPE_INTERFACE)
> +#define INTERFACE(obj) ((Interface *)(obj))
>  
>  static Type type_interface;
>  
> @@ -239,13 +239,21 @@ static void type_initialize(TypeImpl *ti)
>      }
>  }
>  
> +#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
> +
> +static inline bool object_is_interface(Object *obj) {
> +    return obj->interfaces == INTERFACE_MAGIC;
> +}


Why play games?

  static GSList interface_magic;

  static inline bool object_is_interface(Object *obj)
  {
     return obj->interfaces == &interface_magic;
  }

though I think we can spare a bool (and developer's sanity) in Object.
Paolo Bonzini June 13, 2012, 10:59 a.m. UTC | #2 
Il 13/06/2012 12:55, Avi Kivity ha scritto:
>> > +#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
>> > +
>> > +static inline bool object_is_interface(Object *obj) {
>> > +    return obj->interfaces == INTERFACE_MAGIC;
>> > +}
> 
> Why play games?
> 
>   static GSList interface_magic;
> 
>   static inline bool object_is_interface(Object *obj)
>   {
>      return obj->interfaces == &interface_magic;
>   }

Indeed that's beautiful. :)

> though I think we can spare a bool (and developer's sanity) in Object.

Yes, we could shrink the refcount to 16-bit and get place for some
bitfields.

Paolo
Avi Kivity June 13, 2012, 11:02 a.m. UTC | #3 
On 06/13/2012 01:59 PM, Paolo Bonzini wrote:
> 
>> though I think we can spare a bool (and developer's sanity) in Object.
> 
> Yes, we could shrink the refcount to 16-bit and get place for some
> bitfields.


I fact there's a four-byte hole after ->ref as it is.
Paolo Bonzini June 13, 2012, 11:05 a.m. UTC | #4 
Il 13/06/2012 13:02, Avi Kivity ha scritto:
>>> >> though I think we can spare a bool (and developer's sanity) in Object.
>> > 
>> > Yes, we could shrink the refcount to 16-bit and get place for some
>> > bitfields.
> 
> I fact there's a four-byte hole after ->ref as it is.

Not on 32-bits...

Paolo
Avi Kivity June 13, 2012, 11:08 a.m. UTC | #5 
On 06/13/2012 02:05 PM, Paolo Bonzini wrote:
> Il 13/06/2012 13:02, Avi Kivity ha scritto:
>>>> >> though I think we can spare a bool (and developer's sanity) in Object.
>>> > 
>>> > Yes, we could shrink the refcount to 16-bit and get place for some
>>> > bitfields.
>> 
>> I fact there's a four-byte hole after ->ref as it is.
> 
> Not on 32-bits...

Those hardly exists.  And anyway it's silly to save a few bytes of
memory when you're going to drop tons of it on the guest itself.  Better
to spend the effort where we have O(guest size) allocations and keep the
code clear and efficient.
Anthony Liguori June 13, 2012, 1:55 p.m. UTC | #6 
On 06/13/2012 05:41 AM, Paolo Bonzini wrote:
> Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
>> Something is broken with casting to an interface type then setting a link. Cast
>> these two to object for linking instead and it all works.
>
> I don't have a board image but I don't see anything visibly bad without this patch.
>
> However, something that _is_ bad indeed happens; we try to ref/unref an interface
> object.  This patch fixes it while keeping things efficient (and in fact fixes one
> TODO).  Can add a link to an image on http://wiki.qemu.org/Testing and/or test it?
>
> Anthony, is this above your disgust level?

Isn't there a patch floating around to make Object a proper TypeInfo?  In which 
case, couldn't we make Interface not inherit from Object and change the OBJECT() 
cast macro to tell the difference?

Regards,

Anthony Liguori

>
> Paolo
>
>
> ------------------------ 8<  -----------------------
>
> diff --git a/qom/object.c b/qom/object.c
> index c3a7a47..bd60838 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -63,7 +63,7 @@ struct TypeImpl
>       InterfaceImpl interfaces[MAX_INTERFACES];
>   };
>
> -#define INTERFACE(obj) INTERFACE_CHECK(obj, TYPE_INTERFACE)
> +#define INTERFACE(obj) ((Interface *)(obj))
>
>   static Type type_interface;
>
> @@ -239,13 +239,21 @@ static void type_initialize(TypeImpl *ti)
>       }
>   }
>
> +#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
> +
> +static inline bool object_is_interface(Object *obj) {
> +    return obj->interfaces == INTERFACE_MAGIC;
> +}
> +
>   static void object_interface_init(Object *obj, InterfaceImpl *iface)
>   {
>       TypeImpl *ti = iface->type;
>       Interface *iface_obj;
>
> +    assert(!object_is_interface(obj));
>       iface_obj = INTERFACE(object_new(ti->name));
>       iface_obj->iface_obj = obj;
> +    iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;
>
>       obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
>   }
> @@ -332,10 +340,12 @@ static void object_deinit(Object *obj, TypeImpl *type)
>           type->instance_finalize(obj);
>       }
>
> -    while (obj->interfaces) {
> -        Interface *iface_obj = obj->interfaces->data;
> -        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
> -        object_delete(OBJECT(iface_obj));
> +    if (!object_is_interface(obj)) {
> +        while (obj->interfaces) {
> +            Interface *iface_obj = obj->interfaces->data;
> +            obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
> +            object_delete(OBJECT(iface_obj));
> +        }
>       }
>
>       if (type_has_parent(type)) {
> @@ -410,6 +420,13 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
>       TypeImpl *target_type = type_get_by_name(typename);
>       GSList *i;
>
> +    /* Check if obj is an interface and its containing object is a direct
> +     * ancestor of typename.  object_is_interface is a very fast test.
> +     */
> +    if (object_is_interface(obj)) {
> +        obj = INTERFACE(obj)->iface_obj;
> +    }
> +
>       /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
>        * we want to go back from interfaces to the parent.
>       */
> @@ -417,24 +434,6 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
>           return obj;
>       }
>
> -    /* Check if obj is an interface and its containing object is a direct
> -     * ancestor of typename.  In principle we could do this test at the very
> -     * beginning of object_dynamic_cast, avoiding a second call to
> -     * object_is_type.  However, casting between interfaces is relatively
> -     * rare, and object_is_type(obj, type_interface) would fail almost always.
> -     *
> -     * Perhaps we could add a magic value to the object header for increased
> -     * (run-time) type safety and to speed up tests like this one.  If we ever
> -     * do that we can revisit the order here.
> -     */
> -    if (object_is_type(obj, type_interface)) {
> -        assert(!obj->interfaces);
> -        obj = INTERFACE(obj)->iface_obj;
> -        if (object_is_type(obj, target_type)) {
> -            return obj;
> -        }
> -    }
> -
>       if (!target_type) {
>           return obj;
>       }
> @@ -597,11 +596,19 @@ GSList *object_class_get_list(const char *implements_type,
>
>   void object_ref(Object *obj)
>   {
> +    if (object_is_interface(obj)) {
> +        obj = INTERFACE(obj)->iface_obj;
> +    }
> +
>       obj->ref++;
>   }
>
>   void object_unref(Object *obj)
>   {
> +    if (object_is_interface(obj)) {
> +        obj = INTERFACE(obj)->iface_obj;
> +    }
> +
>       g_assert(obj->ref>  0);
>       obj->ref--;
>
> @@ -979,7 +986,7 @@ gchar *object_get_canonical_path(Object *obj)
>       Object *root = object_get_root();
>       char *newpath = NULL, *path = NULL;
>
> -    if (object_is_type(obj, type_interface)) {
> +    if (object_is_interface(obj)) {
>           obj = INTERFACE(obj)->iface_obj;
>       }
>
>
> Paolo
>
>
diff mbox

Patch

diff --git a/qom/object.c b/qom/object.c
index c3a7a47..bd60838 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -63,7 +63,7 @@  struct TypeImpl
     InterfaceImpl interfaces[MAX_INTERFACES];
 };
 
-#define INTERFACE(obj) INTERFACE_CHECK(obj, TYPE_INTERFACE)
+#define INTERFACE(obj) ((Interface *)(obj))
 
 static Type type_interface;
 
@@ -239,13 +239,21 @@  static void type_initialize(TypeImpl *ti)
     }
 }
 
+#define INTERFACE_MAGIC    ((GSList *) (intptr_t)0xBAD0BAD)
+
+static inline bool object_is_interface(Object *obj) {
+    return obj->interfaces == INTERFACE_MAGIC;
+}
+
 static void object_interface_init(Object *obj, InterfaceImpl *iface)
 {
     TypeImpl *ti = iface->type;
     Interface *iface_obj;
 
+    assert(!object_is_interface(obj));
     iface_obj = INTERFACE(object_new(ti->name));
     iface_obj->iface_obj = obj;
+    iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;
 
     obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
 }
@@ -332,10 +340,12 @@  static void object_deinit(Object *obj, TypeImpl *type)
         type->instance_finalize(obj);
     }
 
-    while (obj->interfaces) {
-        Interface *iface_obj = obj->interfaces->data;
-        obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
-        object_delete(OBJECT(iface_obj));
+    if (!object_is_interface(obj)) {
+        while (obj->interfaces) {
+            Interface *iface_obj = obj->interfaces->data;
+            obj->interfaces = g_slist_delete_link(obj->interfaces, obj->interfaces);
+            object_delete(OBJECT(iface_obj));
+        }
     }
 
     if (type_has_parent(type)) {
@@ -410,6 +420,13 @@  Object *object_dynamic_cast(Object *obj, const char *typename)
     TypeImpl *target_type = type_get_by_name(typename);
     GSList *i;
 
+    /* Check if obj is an interface and its containing object is a direct
+     * ancestor of typename.  object_is_interface is a very fast test.
+     */
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     /* Check if typename is a direct ancestor.  Special-case TYPE_OBJECT,
      * we want to go back from interfaces to the parent.
     */
@@ -417,24 +434,6 @@  Object *object_dynamic_cast(Object *obj, const char *typename)
         return obj;
     }
 
-    /* Check if obj is an interface and its containing object is a direct
-     * ancestor of typename.  In principle we could do this test at the very
-     * beginning of object_dynamic_cast, avoiding a second call to
-     * object_is_type.  However, casting between interfaces is relatively
-     * rare, and object_is_type(obj, type_interface) would fail almost always.
-     *
-     * Perhaps we could add a magic value to the object header for increased
-     * (run-time) type safety and to speed up tests like this one.  If we ever
-     * do that we can revisit the order here.
-     */
-    if (object_is_type(obj, type_interface)) {
-        assert(!obj->interfaces);
-        obj = INTERFACE(obj)->iface_obj;
-        if (object_is_type(obj, target_type)) {
-            return obj;
-        }
-    }
-
     if (!target_type) {
         return obj;
     }
@@ -597,11 +596,19 @@  GSList *object_class_get_list(const char *implements_type,
 
 void object_ref(Object *obj)
 {
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     obj->ref++;
 }
 
 void object_unref(Object *obj)
 {
+    if (object_is_interface(obj)) {
+        obj = INTERFACE(obj)->iface_obj;
+    }
+
     g_assert(obj->ref > 0);
     obj->ref--;
 
@@ -979,7 +986,7 @@  gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath = NULL, *path = NULL;
 
-    if (object_is_type(obj, type_interface)) {
+    if (object_is_interface(obj)) {
         obj = INTERFACE(obj)->iface_obj;
     }