[Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Cast debugging can have a substantial cost (20% or more, measured by
Aurelien on qemu-system-ppc64).  Instead of adding special-cased "fast
casts" in the hot paths, we can just disable it in releases.  At the
same time, add tracing facilities that simplify the analysys of those
problems that cast debugging would reveal.

At least patches 1-7 are for 1.5.

Paolo Bonzini (9):
  qom: improve documentation of cast functions
  qom: allow casting of a NULL class
  qom: add a fast path to object_class_dynamic_cast
  qom: pass file/line/function to asserting casts
  qom: trace asserting casts
  qom: allow turning cast debugging off
  build: disable QOM cast debugging for official releases
  qom: simplify object_class_dynamic_cast, part 1
  qom: simplify object_class_dynamic_cast, part 2

 configure            | 20 ++++++++------
 include/qom/object.h | 40 ++++++++++++++++++++++-----
 qom/object.c         | 77 ++++++++++++++++++++++++++++++++++------------------
 trace-events         |  3 ++
 4 files changed, 99 insertions(+), 41 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 1/9] qom: improve documentation of cast functions

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object.h | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index d0f99c5..41b7068 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -612,7 +612,8 @@ Object *object_dynamic_cast(Object *obj, const char *typename);
  *
  * See object_dynamic_cast() for a description of the parameters of this
  * function.  The only difference in behavior is that this function asserts
- * instead of returning #NULL on failure.
+ * instead of returning #NULL on failure.  This function is not meant to be
+ * called directly, but only through the wrapper macro OBJECT_CHECK.
  */
 Object *object_dynamic_cast_assert(Object *obj, const char *typename);
 
@@ -659,11 +660,29 @@ Type type_register(const TypeInfo *info);
  * @klass: The #ObjectClass to attempt to cast.
  * @typename: The QOM typename of the class to cast to.
  *
- * Returns: This function always returns @klass and asserts on failure.
+ * See object_class_dynamic_cast() for a description of the parameters
+ * of this function.  The only difference in behavior is that this function
+ * asserts instead of returning #NULL on failure.  This function is not
+ * meant to be called directly, but only through the wrapper macros
+ * OBJECT_CLASS_CHECK and INTERFACE_CHECK.
  */
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *klass,
                                               const char *typename);
 
+/**
+ * object_class_dynamic_cast:
+ * @klass: The #ObjectClass to attempt to cast.
+ * @typename: The QOM typename of the class to cast to.
+ *
+ * Returns: If @typename is a class, this function returns @klass if
+ * @typename is a subtype of @klass, else returns #NULL.
+ *
+ * If @typename is an interface, this function returns the interface
+ * definition for @klass if @klass implements it unambiguously; #NULL
+ * is returned if @klass does not implement the interface or if multiple
+ * classes or interfaces on the hierarchy leading to @klass implement
+ * it.  (FIXME: perhaps this can be detected at type definition time?)
+ */
 ObjectClass *object_class_dynamic_cast(ObjectClass *klass,
                                        const char *typename);
 
-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 2/9] qom: allow casting of a NULL class

[Paolo Bonzini]

This mimics what we do in object_dynamic_cast_assert.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 75e6aac..35f4694 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -449,10 +449,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 *type = class->type;
     ObjectClass *ret = NULL;
+    TypeImpl *target_type;
+    TypeImpl *type;
 
+    if (!class) {
+        return NULL;
+    }
+
+    type = class->type;
+    target_type = type_get_by_name(typename);
     if (!target_type) {
         /* target class type unknown, so fail the cast */
         return NULL;
@@ -488,7 +494,7 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
 {
     ObjectClass *ret = object_class_dynamic_cast(class, typename);
 
-    if (!ret) {
+    if (!ret && class) {
         fprintf(stderr, "Object %p is not an instance of type %s\n",
                 class, typename);
         abort();
-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 3/9] qom: add a fast path to object_class_dynamic_cast

[Paolo Bonzini]

For leaf classes, in many cases the callbacks will simply downcast
the object back to the original class.  Add this fast path to
object_class_dynamic_cast, object_dynamic_cast will inherit it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index 35f4694..0aa0c07 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -457,7 +457,12 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
         return NULL;
     }
 
+    /* A simple fast path that can trigger a lot for leaf classes.  */
     type = class->type;
+    if (type->name == typename) {
+        return class;
+    }
+
     target_type = type_get_by_name(typename);
     if (!target_type) {
         /* target class type unknown, so fail the cast */
-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 4/9] qom: pass file/line/function to asserting casts

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qom/object.h | 16 +++++++++++-----
 qom/object.c         | 15 +++++++++------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 41b7068..38f674f 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -476,7 +476,8 @@ struct TypeInfo
  * generated.
  */
 #define OBJECT_CHECK(type, obj, name) \
-    ((type *)object_dynamic_cast_assert(OBJECT(obj), (name)))
+    ((type *)object_dynamic_cast_assert(OBJECT(obj), (name), \
+                                        __FILE__, __LINE__, __func__))
 
 /**
  * OBJECT_CLASS_CHECK:
@@ -489,7 +490,8 @@ struct TypeInfo
  * specific class type.
  */
 #define OBJECT_CLASS_CHECK(class, obj, name) \
-    ((class *)object_class_dynamic_cast_assert(OBJECT_CLASS(obj), (name)))
+    ((class *)object_class_dynamic_cast_assert(OBJECT_CLASS(obj), (name), \
+                                               __FILE__, __LINE__, __func__))
 
 /**
  * OBJECT_GET_CLASS:
@@ -547,7 +549,8 @@ struct InterfaceClass
  * Returns: @obj casted to @interface if cast is valid, otherwise raise error.
  */
 #define INTERFACE_CHECK(interface, obj, name) \
-    ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name)))
+    ((interface *)object_dynamic_cast_assert(OBJECT((obj)), (name), \
+                                             __FILE__, __LINE__, __func__))
 
 /**
  * object_new:
@@ -615,7 +618,8 @@ Object *object_dynamic_cast(Object *obj, const char *typename);
  * instead of returning #NULL on failure.  This function is not meant to be
  * called directly, but only through the wrapper macro OBJECT_CHECK.
  */
-Object *object_dynamic_cast_assert(Object *obj, const char *typename);
+Object *object_dynamic_cast_assert(Object *obj, const char *typename,
+                                   const char *file, int line, const char *func);
 
 /**
  * object_get_class:
@@ -667,7 +671,9 @@ Type type_register(const TypeInfo *info);
  * OBJECT_CLASS_CHECK and INTERFACE_CHECK.
  */
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *klass,
-                                              const char *typename);
+                                              const char *typename,
+                                              const char *file, int line,
+                                              const char *func);
 
 /**
  * object_class_dynamic_cast:
diff --git a/qom/object.c b/qom/object.c
index 0aa0c07..bca6219 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -431,15 +431,16 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
     return NULL;
 }
 
-Object *object_dynamic_cast_assert(Object *obj, const char *typename)
+Object *object_dynamic_cast_assert(Object *obj, const char *typename,
+                                   const char *file, int line, const char *func)
 {
     Object *inst;
 
     inst = object_dynamic_cast(obj, typename);
 
     if (!inst && obj) {
-        fprintf(stderr, "Object %p is not an instance of type %s\n",
-                obj, typename);
+        fprintf(stderr, "%s:%d:%s: Object %p is not an instance of type %s\n",
+                file, line, func, obj, typename);
         abort();
     }
 
@@ -495,13 +496,15 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
 }
 
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
-                                              const char *typename)
+                                              const char *typename,
+                                              const char *file, int line,
+                                              const char *func)
 {
     ObjectClass *ret = object_class_dynamic_cast(class, typename);
 
     if (!ret && class) {
-        fprintf(stderr, "Object %p is not an instance of type %s\n",
-                class, typename);
+        fprintf(stderr, "%s:%d:%s: Object %p is not an instance of type %s\n",
+                file, line, func, class, typename);
         abort();
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 5/9] qom: trace asserting casts

[Paolo Bonzini]

This provides a way to detect the cast that leads to a (reproducible)
crash even when QOM cast debugging is disabled.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 10 +++++++++-
 trace-events |  3 +++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index bca6219..1b9c5ce 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -16,6 +16,7 @@
 #include "qapi/string-input-visitor.h"
 #include "qapi/string-output-visitor.h"
 #include "qapi/qmp/qerror.h"
+#include "trace.h"
 
 /* TODO: replace QObject with a simpler visitor to avoid a dependency
  * of the QOM core on QObject?  */
@@ -436,6 +437,9 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename,
 {
     Object *inst;
 
+    trace_object_dynamic_cast_assert(obj ? obj->class->type->name : "(null)",
+                                     typename, file, line, func);
+
     inst = object_dynamic_cast(obj, typename);
 
     if (!inst && obj) {
@@ -500,8 +504,12 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
                                               const char *file, int line,
                                               const char *func)
 {
-    ObjectClass *ret = object_class_dynamic_cast(class, typename);
+    ObjectClass *ret;
+
+    trace_object_class_dynamic_cast_assert(class ? class->type->name : "(null)",
+                                           typename, file, line, func);
 
+    ret = object_class_dynamic_cast(class, typename);
     if (!ret && class) {
         fprintf(stderr, "%s:%d:%s: Object %p is not an instance of type %s\n",
                 file, line, func, class, typename);
diff --git a/trace-events b/trace-events
index 17d75ab..4413beb 100644
--- a/trace-events
+++ b/trace-events
@@ -1160,3 +1160,6 @@ kvm_vm_ioctl(int type, void *arg) "type %d, arg %p"
 kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type %d, arg %p"
 kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
 
+# qom/object.c
+object_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
+object_class_dynamic_cast_assert(const char *type, const char *target, const char *file, int line, const char *func) "%s->%s (%s:%d:%s)"
-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 6/9] qom: allow turning cast debugging off

[Paolo Bonzini]

Cast debugging can have a substantial cost (20% or more).  Instead of adding
special-cased "fast casts" in the hot paths, we can just disable it in
releases.  The tracing facilities we just added make it easier to analyze
those problems that cast debugging would reveal.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 configure            |  8 ++++++++
 include/qom/object.h | 11 ++++++-----
 qom/object.c         | 15 +++++++++++----
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/configure b/configure
index 9439f1c..dd44562 100755
--- a/configure
+++ b/configure
@@ -220,6 +220,7 @@ blobs="yes"
 pkgversion=""
 pie=""
 zero_malloc=""
+qom_cast_debug="yes"
 trace_backend="nop"
 trace_file="trace"
 spice=""
@@ -688,6 +689,10 @@ for opt do
   ;;
   --enable-sdl) sdl="yes"
   ;;
+  --disable-qom-cast-debug) qom_cast_debug="no"
+  ;;
+  --enable-qom-cast-debug) qom_cast_debug="yes"
+  ;;
   --disable-virtfs) virtfs="no"
   ;;
   --enable-virtfs) virtfs="yes"
@@ -3579,6 +3579,7 @@ echo "gcov enabled      $gcov"
 echo "TPM support       $tpm"
 echo "libssh2 support   $libssh2"
 echo "TPM passthrough   $tpm_passthrough"
+echo "QOM debugging     $qom_cast_debug"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -3909,6 +3915,9 @@ echo "CONFIG_UNAME_RELEASE=\"$uname_release\"" >> $config_host_mak
 if test "$zero_malloc" = "yes" ; then
   echo "CONFIG_ZERO_MALLOC=y" >> $config_host_mak
 fi
+if test "$qom_cast_debug" = "yes" ; then
+  echo "CONFIG_QOM_CAST_DEBUG=y" >> $config_host_mak
+fi
 if test "$rbd" = "yes" ; then
   echo "CONFIG_RBD=y" >> $config_host_mak
 fi
diff --git a/include/qom/object.h b/include/qom/object.h
index 38f674f..63e2a40 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -615,8 +615,9 @@ Object *object_dynamic_cast(Object *obj, const char *typename);
  *
  * See object_dynamic_cast() for a description of the parameters of this
  * function.  The only difference in behavior is that this function asserts
- * instead of returning #NULL on failure.  This function is not meant to be
- * called directly, but only through the wrapper macro OBJECT_CHECK.
+ * instead of returning #NULL on failure if QOM cast debugging is enabled.
+ * This function is not meant to be called directly, but only through
+ * the wrapper macro OBJECT_CHECK.
  */
 Object *object_dynamic_cast_assert(Object *obj, const char *typename,
                                    const char *file, int line, const char *func);
@@ -666,9 +667,9 @@ Type type_register(const TypeInfo *info);
  *
  * See object_class_dynamic_cast() for a description of the parameters
  * of this function.  The only difference in behavior is that this function
- * asserts instead of returning #NULL on failure.  This function is not
- * meant to be called directly, but only through the wrapper macros
- * OBJECT_CLASS_CHECK and INTERFACE_CHECK.
+ * asserts instead of returning #NULL on failure if QOM cast debugging is
+ * enabled.  This function is not meant to be called directly, but only through
+ * the wrapper macros OBJECT_CLASS_CHECK and INTERFACE_CHECK.
  */
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *klass,
                                               const char *typename,
diff --git a/qom/object.c b/qom/object.c
index 1b9c5ce..f5f416b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -435,12 +435,11 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
 Object *object_dynamic_cast_assert(Object *obj, const char *typename,
                                    const char *file, int line, const char *func)
 {
-    Object *inst;
-
     trace_object_dynamic_cast_assert(obj ? obj->class->type->name : "(null)",
                                      typename, file, line, func);
 
-    inst = object_dynamic_cast(obj, typename);
+#ifdef CONFIG_QOM_CAST_DEBUG
+    Object *inst = object_dynamic_cast(obj, typename);
 
     if (!inst && obj) {
         fprintf(stderr, "%s:%d:%s: Object %p is not an instance of type %s\n",
@@ -448,7 +447,9 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename,
         abort();
     }
 
-    return inst;
+    assert(obj == inst);
+#endif
+    return obj;
 }
 
 ObjectClass *object_class_dynamic_cast(ObjectClass *class,
@@ -509,6 +510,12 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
     trace_object_class_dynamic_cast_assert(class ? class->type->name : "(null)",
                                            typename, file, line, func);
 
+#ifndef CONFIG_QOM_CAST_DEBUG
+    if (!class->interfaces) {
+        return class;
+    }
+#endif
+
     ret = object_class_dynamic_cast(class, typename);
     if (!ret && class) {
         fprintf(stderr, "%s:%d:%s: Object %p is not an instance of type %s\n",
-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 7/9] build: disable QOM cast debugging for official releases

[Paolo Bonzini]

QOM cast debugging is useful, but it can have a substantial cost.  We
will add a configure option to toggle it, in the meanwhile let's keep
it disabled in official releases (including -rc).

Replace the old CONFIG_ZERO_MALLOC that had the same rules but has
been unused since we switched to g_malloc/g_free.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 configure | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/configure b/configure
index dd44562..39c7207 100755
--- a/configure
+++ b/configure
@@ -219,8 +219,7 @@ aix="no"
 blobs="yes"
 pkgversion=""
 pie=""
-zero_malloc=""
-qom_cast_debug="yes"
+qom_cast_debug=""
 trace_backend="nop"
 trace_file="trace"
 spice=""
@@ -3390,13 +3389,13 @@ elif test "$debug" = "no" ; then
 fi
 
 
-# Disable zero malloc errors for official releases unless explicitly told to
-# enable/disable
-if test -z "$zero_malloc" ; then
+# Enable QOM casts debugging by default during development unless explicitly
+# told to enable/disable
+if test -z "$qom_cast_debug" ; then
     if test "$z_version" = "50" ; then
-	zero_malloc="no"
+	qom_cast_debug="yes"
     else
-	zero_malloc="yes"
+	qom_cast_debug="no"
     fi
 fi
 
@@ -3911,9 +3910,6 @@ fi
 
 echo "CONFIG_UNAME_RELEASE=\"$uname_release\"" >> $config_host_mak
 
-if test "$zero_malloc" = "yes" ; then
-  echo "CONFIG_ZERO_MALLOC=y" >> $config_host_mak
-fi
 if test "$qom_cast_debug" = "yes" ; then
   echo "CONFIG_QOM_CAST_DEBUG=y" >> $config_host_mak
 fi
-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 8/9] qom: simplify object_class_dynamic_cast, part 1

[Paolo Bonzini]

Access everything from the class.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index f5f416b..f82f12c 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -457,15 +457,13 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
 {
     ObjectClass *ret = NULL;
     TypeImpl *target_type;
-    TypeImpl *type;
 
     if (!class) {
         return NULL;
     }
 
     /* A simple fast path that can trigger a lot for leaf classes.  */
-    type = class->type;
-    if (type->name == typename) {
+    if (class->type->name == typename) {
         return class;
     }
 
@@ -475,7 +473,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
         return NULL;
     }
 
-    if (type->class->interfaces &&
+    if (class->interfaces &&
             type_is_ancestor(target_type, type_interface)) {
         int found = 0;
         GSList *i;
@@ -493,7 +491,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
         if (found > 1) {
             ret = NULL;
         }
-    } else if (type_is_ancestor(type, target_type)) {
+    } else if (type_is_ancestor(class->type, target_type)) {
         ret = class;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH for-1.5 9/9] qom: simplify object_class_dynamic_cast, part 2

[Paolo Bonzini]

Detect ambiguous matches right away, without going through all
interfaces first.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qom/object.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index f82f12c..c69e6a3 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -455,7 +455,6 @@ Object *object_dynamic_cast_assert(Object *obj, const char *typename,
 ObjectClass *object_class_dynamic_cast(ObjectClass *class,
                                        const char *typename)
 {
-    ObjectClass *ret = NULL;
     TypeImpl *target_type;
 
     if (!class) {
@@ -475,27 +474,26 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
 
     if (class->interfaces &&
             type_is_ancestor(target_type, type_interface)) {
-        int found = 0;
+        ObjectClass *ret = NULL;
         GSList *i;
 
         for (i = class->interfaces; i; i = i->next) {
             ObjectClass *target_class = i->data;
 
             if (type_is_ancestor(target_class->type, target_type)) {
+                /* If the match is ambiguous, don't allow a cast */
+                if (ret) {
+                    ret = NULL;
+                }
                 ret = target_class;
-                found++;
             }
          }
-
-        /* The match was ambiguous, don't allow a cast */
-        if (found > 1) {
-            ret = NULL;
-        }
+        return ret;
     } else if (type_is_ancestor(class->type, target_type)) {
-        ret = class;
+        return class;
+    } else {
+        return NULL;
     }
-
-    return ret;
 }
 
 ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Cast debugging can have a substantial cost (20% or more, measured by
> Aurelien on qemu-system-ppc64).

[Needs citation]

> Instead of adding special-cased "fast
> casts" in the hot paths, we can just disable it in releases.  At the
> same time, add tracing facilities that simplify the analysys of those
> problems that cast debugging would reveal.

I'd prefer not to disable but instead focus on improving performance.

I suspect any performance overhead is resolving the type string to
typeimpl.  One work around is to have a dynamic cast that takes a
typeimpl and then use a function that returns a static similar to how
glib works.

If you've got a reproducible case where the overhead is high, it should
be easy to check.

Regards,

Anthony Liguori

>
> At least patches 1-7 are for 1.5.
>
> Paolo Bonzini (9):
>   qom: improve documentation of cast functions
>   qom: allow casting of a NULL class
>   qom: add a fast path to object_class_dynamic_cast
>   qom: pass file/line/function to asserting casts
>   qom: trace asserting casts
>   qom: allow turning cast debugging off
>   build: disable QOM cast debugging for official releases
>   qom: simplify object_class_dynamic_cast, part 1
>   qom: simplify object_class_dynamic_cast, part 2
>
>  configure            | 20 ++++++++------
>  include/qom/object.h | 40 ++++++++++++++++++++++-----
>  qom/object.c         | 77 ++++++++++++++++++++++++++++++++++------------------
>  trace-events         |  3 ++
>  4 files changed, 99 insertions(+), 41 deletions(-)
>
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 15:01, Anthony Liguori ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Cast debugging can have a substantial cost (20% or more, measured by
>> Aurelien on qemu-system-ppc64).
> 
> [Needs citation]

Sure: http://permalink.gmane.org/gmane.comp.emulators.qemu/210830

 49,73%  perf-10672.map           [.] 0x7f7853ab4e0f
 13,23%  qemu-system-ppc64        [.] cpu_ppc_exec
 13,16%  libglib-2.0.so.0.3200.4  [.] g_hash_table_lookup
  8,18%  libglib-2.0.so.0.3200.4  [.] g_str_hash
  2,47%  qemu-system-ppc64        [.] object_class_dynamic_cast
  1,97%  qemu-system-ppc64        [.] type_is_ancestor
  1,05%  libglib-2.0.so.0.3200.4  [.] g_str_equal

That's ~27%.

>> Instead of adding special-cased "fast
>> casts" in the hot paths, we can just disable it in releases.  At the
>> same time, add tracing facilities that simplify the analysys of those
>> problems that cast debugging would reveal.
> 
> I'd prefer not to disable but instead focus on improving performance.

For 1.5?  This is a regression in 1.5 due to more and more usage of
foo_env_on_cpu.

Paolo

> I suspect any performance overhead is resolving the type string to
> typeimpl.  One work around is to have a dynamic cast that takes a
> typeimpl and then use a function that returns a static similar to how
> glib works.
> 
> If you've got a reproducible case where the overhead is high, it should
> be easy to check.
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> At least patches 1-7 are for 1.5.
>>
>> Paolo Bonzini (9):
>>   qom: improve documentation of cast functions
>>   qom: allow casting of a NULL class
>>   qom: add a fast path to object_class_dynamic_cast
>>   qom: pass file/line/function to asserting casts
>>   qom: trace asserting casts
>>   qom: allow turning cast debugging off
>>   build: disable QOM cast debugging for official releases
>>   qom: simplify object_class_dynamic_cast, part 1
>>   qom: simplify object_class_dynamic_cast, part 2
>>
>>  configure            | 20 ++++++++------
>>  include/qom/object.h | 40 ++++++++++++++++++++++-----
>>  qom/object.c         | 77 ++++++++++++++++++++++++++++++++++------------------
>>  trace-events         |  3 ++
>>  4 files changed, 99 insertions(+), 41 deletions(-)
>>
>> -- 
>> 1.8.1.4
> 

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Andreas Färber]

Am 10.05.2013 15:08, schrieb Paolo Bonzini:
> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
>> I'd prefer not to disable but instead focus on improving performance.
> 
> For 1.5?  This is a regression in 1.5 due to more and more usage of
> foo_env_on_cpu.

If CPUs were the only reason, we could simply change those inlines and
ENV_GET_CPU() macro to use a C cast. No complicated interface scenarios
requiring a dynamic cast are used for CPUs so far to my knowledge.

Either way, it would be nice to see the call sites of those
most-impacting dynamic casts! So far I held back my APIC RFC since I'm
not sure how to reproducibly profile things.

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 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 15:23, Andreas Färber ha scritto:
> Am 10.05.2013 15:08, schrieb Paolo Bonzini:
>> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
>>> I'd prefer not to disable but instead focus on improving performance.
>>
>> For 1.5?  This is a regression in 1.5 due to more and more usage of
>> foo_env_on_cpu.
> 
> If CPUs were the only reason, we could simply change those inlines and
> ENV_GET_CPU() macro to use a C cast. No complicated interface scenarios
> requiring a dynamic cast are used for CPUs so far to my knowledge.

Almost nothing really requires a dynamic cast in QEMU.  Only interface
casts do, and there's just a couple of uses of interfaces.

And I wrote in the cover letter that what I want is really avoid the
need for "fast casts" in the hot paths.

Can you guys actually read the commit messages?

> Either way, it would be nice to see the call sites of those
> most-impacting dynamic casts! So far I held back my APIC RFC since I'm
> not sure how to reproducibly profile things.

It's interrupts (both sending and returning from them).

Paolo

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 03:30:17PM +0200, Paolo Bonzini wrote:
> Il 10/05/2013 15:23, Andreas Färber ha scritto:
> > Am 10.05.2013 15:08, schrieb Paolo Bonzini:
> >> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
> >>> I'd prefer not to disable but instead focus on improving performance.
> >>
> >> For 1.5?  This is a regression in 1.5 due to more and more usage of
> >> foo_env_on_cpu.
> > 
> > If CPUs were the only reason, we could simply change those inlines and
> > ENV_GET_CPU() macro to use a C cast. No complicated interface scenarios
> > requiring a dynamic cast are used for CPUs so far to my knowledge.
> 
> Almost nothing really requires a dynamic cast in QEMU.  Only interface
> casts do, and there's just a couple of uses of interfaces.
> 
> And I wrote in the cover letter that what I want is really avoid the
> need for "fast casts" in the hot paths.
> 
> Can you guys actually read the commit messages?
> 
> > Either way, it would be nice to see the call sites of those
> > most-impacting dynamic casts! So far I held back my APIC RFC since I'm
> > not sure how to reproducibly profile things.
> 
> It's interrupts (both sending and returning from them).
> 

More precisely in target-ppc/excp_helper.c:
- in ppc_hw_interrupt
- in helper_store_msr
- in do_rfi (called from helper_rfi)
- in helper_msgsnd

Sot it's at least twice per interruption, which means doing hash table
lookup and string comparison through glib a few hundred to a few
thousand times per second. Before the QOMification, it was a simple
pointer access.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Cast debugging can have a substantial cost (20% or more, measured by
>>> Aurelien on qemu-system-ppc64).
>> 
>> [Needs citation]
>
> Sure: http://permalink.gmane.org/gmane.comp.emulators.qemu/210830
>
>  49,73%  perf-10672.map           [.] 0x7f7853ab4e0f
>  13,23%  qemu-system-ppc64        [.] cpu_ppc_exec
>  13,16%  libglib-2.0.so.0.3200.4  [.] g_hash_table_lookup
>   8,18%  libglib-2.0.so.0.3200.4  [.] g_str_hash
>   2,47%  qemu-system-ppc64        [.] object_class_dynamic_cast
>   1,97%  qemu-system-ppc64        [.] type_is_ancestor
>   1,05%  libglib-2.0.so.0.3200.4  [.] g_str_equal
>
> That's ~27%.
>
>>> Instead of adding special-cased "fast
>>> casts" in the hot paths, we can just disable it in releases.  At the
>>> same time, add tracing facilities that simplify the analysys of those
>>> problems that cast debugging would reveal.
>> 
>> I'd prefer not to disable but instead focus on improving performance.
>
> For 1.5?  This is a regression in 1.5 due to more and more usage of
> foo_env_on_cpu.

If this is a regression, then we should be able to show:

1) workload XYZ gets X in 1.4 and Y in 1.5

2) applying your series demonstrates that we go back to X performance

I'm not convinced this is a true set of statements.  13% of time spent
doing casts could simply suggest that the vcpu is largely idle and has
nothing better to do.

At any rate, given (1), we can at least try some other less invasive
approachs to fixing the problem.  Just removing the unnecessary cast
might end up doing it.

Regards,

Anthony Liguori

>
> Paolo
>
>> I suspect any performance overhead is resolving the type string to
>> typeimpl.  One work around is to have a dynamic cast that takes a
>> typeimpl and then use a function that returns a static similar to how
>> glib works.
>> 
>> If you've got a reproducible case where the overhead is high, it should
>> be easy to check.
>> 
>> Regards,
>> 
>> Anthony Liguori
>> 
>>>
>>> At least patches 1-7 are for 1.5.
>>>
>>> Paolo Bonzini (9):
>>>   qom: improve documentation of cast functions
>>>   qom: allow casting of a NULL class
>>>   qom: add a fast path to object_class_dynamic_cast
>>>   qom: pass file/line/function to asserting casts
>>>   qom: trace asserting casts
>>>   qom: allow turning cast debugging off
>>>   build: disable QOM cast debugging for official releases
>>>   qom: simplify object_class_dynamic_cast, part 1
>>>   qom: simplify object_class_dynamic_cast, part 2
>>>
>>>  configure            | 20 ++++++++------
>>>  include/qom/object.h | 40 ++++++++++++++++++++++-----
>>>  qom/object.c         | 77 ++++++++++++++++++++++++++++++++++------------------
>>>  trace-events         |  3 ++
>>>  4 files changed, 99 insertions(+), 41 deletions(-)
>>>
>>> -- 
>>> 1.8.1.4
>> 

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 10/05/2013 15:23, Andreas Färber ha scritto:
>> Am 10.05.2013 15:08, schrieb Paolo Bonzini:
>>> Il 10/05/2013 15:01, Anthony Liguori ha scritto:
>>>> I'd prefer not to disable but instead focus on improving performance.
>>>
>>> For 1.5?  This is a regression in 1.5 due to more and more usage of
>>> foo_env_on_cpu.
>> 
>> If CPUs were the only reason, we could simply change those inlines and
>> ENV_GET_CPU() macro to use a C cast. No complicated interface scenarios
>> requiring a dynamic cast are used for CPUs so far to my knowledge.
>
> Almost nothing really requires a dynamic cast in QEMU.  Only interface
> casts do, and there's just a couple of uses of interfaces.
>
> And I wrote in the cover letter that what I want is really avoid the
> need for "fast casts" in the hot paths.
>
> Can you guys actually read the commit messages?

Yeah, the vast majority of the series is a nice cleanup.

I just oppose the notion of disabling casts and *especially* only
disabling casts for official builds.

I don't see a reason to do something like this without even attempting
to improve performance first.

Regards,

Anthony Liguori

>> Either way, it would be nice to see the call sites of those
>> most-impacting dynamic casts! So far I held back my APIC RFC since I'm
>> not sure how to reproducibly profile things.
>
> It's interrupts (both sending and returning from them).
>
> Paolo

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 09:27:28AM -0500, Anthony Liguori wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 10/05/2013 15:01, Anthony Liguori ha scritto:
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >>> Cast debugging can have a substantial cost (20% or more, measured by
> >>> Aurelien on qemu-system-ppc64).
> >> 
> >> [Needs citation]
> >
> > Sure: http://permalink.gmane.org/gmane.comp.emulators.qemu/210830
> >
> >  49,73%  perf-10672.map           [.] 0x7f7853ab4e0f
> >  13,23%  qemu-system-ppc64        [.] cpu_ppc_exec
> >  13,16%  libglib-2.0.so.0.3200.4  [.] g_hash_table_lookup
> >   8,18%  libglib-2.0.so.0.3200.4  [.] g_str_hash
> >   2,47%  qemu-system-ppc64        [.] object_class_dynamic_cast
> >   1,97%  qemu-system-ppc64        [.] type_is_ancestor
> >   1,05%  libglib-2.0.so.0.3200.4  [.] g_str_equal
> >
> > That's ~27%.
> >
> >>> Instead of adding special-cased "fast
> >>> casts" in the hot paths, we can just disable it in releases.  At the
> >>> same time, add tracing facilities that simplify the analysys of those
> >>> problems that cast debugging would reveal.
> >> 
> >> I'd prefer not to disable but instead focus on improving performance.
> >
> > For 1.5?  This is a regression in 1.5 due to more and more usage of
> > foo_env_on_cpu.
> 
> If this is a regression, then we should be able to show:
> 
> 1) workload XYZ gets X in 1.4 and Y in 1.5
> 
> 2) applying your series demonstrates that we go back to X performance

I will work on that.

> I'm not convinced this is a true set of statements.  13% of time spent
> doing casts could simply suggest that the vcpu is largely idle and has
> nothing better to do.

It's not in KVM, but in the QEMU case. The emulated CPU was fully loaded
during this perf trace, and the qemu-system-ppc64 process was taking
about 100% of the host CPU. This means that this 13% of the time doing
cast can be used to emulate more instructions.

> At any rate, given (1), we can at least try some other less invasive
> approachs to fixing the problem.  Just removing the unnecessary cast
> might end up doing it.

We have changed a pointer access in a hot path (and more are likely to
come as far as I know) to a complex function. The correct way to fix
that is to remove the complex function.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 16:39, Anthony Liguori ha scritto:
> I just oppose the notion of disabling casts and *especially* only
> disabling casts for official builds.

This actually happens all the time.  Exactly this kind of type-safe cast
is disabled in releases of GCC, but enabled when building from svn trunk.

I have hardly seen any of these failures _during development_, much less
on a release.  I appreciate the advantage of type-safe casts, but in
QEMU they are a solution in search of a problem.  They are cheap to
implement (though not that cheap to execute ;)) so it's perfectly fine
to have them, but they are not _needed_; disabling them is anyway a good
build-time option to have.

Type-safe casts make sense in GTK+/GObject where: 1) type-safe casts
return NULL and log a "critical" error, they do not abort; 2) all
functions fail with another "critical" error if they are passed NULL.
We do neither, so we're just trading a crash now for a crash very soon
after (our call stacks tend to be relatively shallow, with some
exceptions such as the block layer).

Also, in GTK+/GObject the code paths are unpredictable because they
depend on user interaction, and a crash can lead to data loss.  By
contrast, in QEMU most of the code is hardly ever run, and the possible
paths are very few because driver writers tend to use always the same
path.  The day someone is bringing up a new guest OS and encounters such
a crash, we'll tell them to either build from git, or to use
--enable-qom-cast-debug.

I'm sure it will be a long time before that happens...

Paolo

> Regards,
> 
> Anthony Liguori
> 
>>> Either way, it would be nice to see the call sites of those
>>> most-impacting dynamic casts! So far I held back my APIC RFC since I'm
>>> not sure how to reproducibly profile things.
>>
>> It's interrupts (both sending and returning from them).
>>
>> Paolo
> 
> 
> 

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Andreas Färber]

Am 10.05.2013 16:46, schrieb Aurelien Jarno:
> We have changed a pointer access in a hot path (and more are likely to
> come as far as I know) to a complex function. The correct way to fix
> that is to remove the complex function.

FWIW I had started to investigate whether I can tweak TCG to use
CPUFooState for its offset calculations as-is but to call helpers with
FooCPU to get code in a uniform shape, not constantly coding casts as
first thing in helper functions. We'd need a pointer similar to
CPUState::env_ptr in the back part of CPU_COMMON, make FooCPU available
as ArchCPU define and tweak the helper templates to support cpu type
argument in addition to env. So far just an idea.

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 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 02:16:34PM +0200, Paolo Bonzini wrote:
> Cast debugging can have a substantial cost (20% or more, measured by
> Aurelien on qemu-system-ppc64).  Instead of adding special-cased "fast
> casts" in the hot paths, we can just disable it in releases.  At the
> same time, add tracing facilities that simplify the analysys of those
> problems that cast debugging would reveal.
> 
> At least patches 1-7 are for 1.5.
> 
> Paolo Bonzini (9):
>   qom: improve documentation of cast functions
>   qom: allow casting of a NULL class
>   qom: add a fast path to object_class_dynamic_cast
>   qom: pass file/line/function to asserting casts
>   qom: trace asserting casts
>   qom: allow turning cast debugging off
>   build: disable QOM cast debugging for official releases
>   qom: simplify object_class_dynamic_cast, part 1
>   qom: simplify object_class_dynamic_cast, part 2
> 
>  configure            | 20 ++++++++------
>  include/qom/object.h | 40 ++++++++++++++++++++++-----
>  qom/object.c         | 77 ++++++++++++++++++++++++++++++++++------------------
>  trace-events         |  3 ++
>  4 files changed, 99 insertions(+), 41 deletions(-)
> 

I have tested this series with qemu-system-ppc64, on a Core i7 2600 CPU.
The process was set to a single core using taskset. Inside the guest
(Debian ppc64 from debian-ports), I ran the command three times:

  lintian g++-4.8_4.8.0-6_ppc64.deb

I used lintian as it's a perl code, that trigger the discussion about
sparc/ppc comparison.

First of all with this patch series, the object_class_dynamic_cast calls
went down to below 0.1% when using perf top. Before the patch series was
applied, the command took in average on 3 runs 142.4s. With the patch
series, it went down to 129.8s, so almost 9% faster.

To improve the performance a bit more, and come back to the same kind of
code as before, we should move simple accessors from qom/*.c to
include/qom/*.h and mark them as inline, so that they can be removed by
the compiler. Currently, even if the function is simple it's still a
call/ret in the hot path instead of a simple pointer addition.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Andreas Färber]

Am 10.05.2013 17:56, schrieb Aurelien Jarno:
> To improve the performance a bit more, and come back to the same kind of
> code as before, we should move simple accessors from qom/*.c to
> include/qom/*.h and mark them as inline, so that they can be removed by
> the compiler. Currently, even if the function is simple it's still a
> call/ret in the hot path instead of a simple pointer addition.

Could you be more concrete? cpu_reset() and the ELF write functions are
the only such things that comes to my mind from qom/cpu.c, and they will
not be called regularly during your Perl execution. You mean something
from qom/object.c?

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 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 18:18, Andreas Färber ha scritto:
> Am 10.05.2013 17:56, schrieb Aurelien Jarno:
>> To improve the performance a bit more, and come back to the same kind of
>> code as before, we should move simple accessors from qom/*.c to
>> include/qom/*.h and mark them as inline, so that they can be removed by
>> the compiler. Currently, even if the function is simple it's still a
>> call/ret in the hot path instead of a simple pointer addition.
> 
> Could you be more concrete? cpu_reset() and the ELF write functions are
> the only such things that comes to my mind from qom/cpu.c, and they will
> not be called regularly during your Perl execution. You mean something
> from qom/object.c?

Yes, the _assert versions of the casts could be made inline.

Paolo

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 10/05/2013 16:39, Anthony Liguori ha scritto:
>> I just oppose the notion of disabling casts and *especially* only
>> disabling casts for official builds.
>
> This actually happens all the time.  Exactly this kind of type-safe cast
> is disabled in releases of GCC, but enabled when building from svn trunk.

Let's assume for a moment that you are right and this behavior is what
we should have.  Let's also assume there is a real regression here
which has yet to have been established.

As soon as we open up 1.6 for development, we face an immediate
regression in performance.  So we need to fix the real problem here
anyway.

I strongly suspect that if there is a problem, that optimizing
leaf/concrete casts will take care of it.  Otherwise, a small lookup
cache ought to do the trick too.  We can even use pointer comparisons
for the lookup cache which should make it extremely cheap.

Disabling casts doesn't give us a long term fix.  One of the solutions
above does and patches also exist.

So first, let's establish if there really is a performance issue here
and if so, let's find a long term solution.

Let's independently evaluate your proposal for 1.6.  You may in fact be
right that it's the right thing to do long term, but I'm quite confident
that it's not the right solution to the potential issue here.

> I have hardly seen any of these failures _during development_, much less
> on a release.

It's the most common failure that I catch in my own testing.  Most often
around hotplug which tends to break the most.

Of course, these changes never make it into the tree which is an
indication that the mechanism works very well :-)

>  I appreciate the advantage of type-safe casts, but in
> QEMU they are a solution in search of a problem.  They are cheap to
> implement (though not that cheap to execute ;)) so it's perfectly fine
> to have them, but they are not _needed_; disabling them is anyway a good
> build-time option to have.

Note that the cast macros are a big improvement in code readability.
The only real replacement would be static casts which would downgrade
safety.

If you want to debate the merits of the safety, that's fine.

> Type-safe casts make sense in GTK+/GObject where: 1) type-safe casts
> return NULL and log a "critical" error, they do not abort;  2) all
> functions fail with another "critical" error if they are passed NULL.
> We do neither, so we're just trading a crash now for a crash very soon
> after (our call stacks tend to be relatively shallow, with some
> exceptions such as the block layer).

The big assumption here is that dereference a NULL results in
predictable failure.  This is not always the case and can lead to
security vulnerabilities.

> Also, in GTK+/GObject the code paths are unpredictable because they
> depend on user interaction, and a crash can lead to data loss.  By
> contrast, in QEMU most of the code is hardly ever run, and the possible
> paths are very few because driver writers tend to use always the same
> path.  The day someone is bringing up a new guest OS and encounters such
> a crash, we'll tell them to either build from git, or to use
> --enable-qom-cast-debug.
>
> I'm sure it will be a long time before that happens...

If you are concerned about the performance, provide a concrete example
of poor performance and I will fix it.

If we find one that is unfixable, then we should consider your
proposal.

Regards,

Anthony Liguori

>
> Paolo
>
>> Regards,
>> 
>> Anthony Liguori
>> 
>>>> Either way, it would be nice to see the call sites of those
>>>> most-impacting dynamic casts! So far I held back my APIC RFC since I'm
>>>> not sure how to reproducibly profile things.
>>>
>>> It's interrupts (both sending and returning from them).
>>>
>>> Paolo
>> 
>> 
>> 

Re: [Qemu-devel] [PATCH for-1.5 8/9] qom: simplify object_class_dynamic_cast, part 1

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Access everything from the class.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qom/object.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index f5f416b..f82f12c 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -457,15 +457,13 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>  {
>      ObjectClass *ret = NULL;
>      TypeImpl *target_type;
> -    TypeImpl *type;
>  
>      if (!class) {
>          return NULL;
>      }
>  
>      /* A simple fast path that can trigger a lot for leaf classes.  */
> -    type = class->type;
> -    if (type->name == typename) {
> +    if (class->type->name == typename) {

This won't trigger because we unconditionally strdup().  Can you add
this the bit from my patch that sets ->name appropriately?

Aurelien, can you try the resulting series with
--{enable,disable}-qom-casts?  My suspicion is that this is the primary
source of speed up.

If you can make an image available too, I can try this myself.

Regards,

Anthony Liguori

>          return class;
>      }
>  
> @@ -475,7 +473,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>          return NULL;
>      }
>  
> -    if (type->class->interfaces &&
> +    if (class->interfaces &&
>              type_is_ancestor(target_type, type_interface)) {
>          int found = 0;
>          GSList *i;
> @@ -493,7 +491,7 @@ ObjectClass *object_class_dynamic_cast(ObjectClass *class,
>          if (found > 1) {
>              ret = NULL;
>          }
> -    } else if (type_is_ancestor(type, target_type)) {
> +    } else if (type_is_ancestor(class->type, target_type)) {
>          ret = class;
>      }
>  
> -- 
> 1.8.1.4

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Aurelien Jarno <aurelien@aurel32.net> writes:

> On Fri, May 10, 2013 at 02:16:34PM +0200, Paolo Bonzini wrote:
>> Cast debugging can have a substantial cost (20% or more, measured by
>> Aurelien on qemu-system-ppc64).  Instead of adding special-cased "fast
>> casts" in the hot paths, we can just disable it in releases.  At the
>> same time, add tracing facilities that simplify the analysys of those
>> problems that cast debugging would reveal.
>> 
>> At least patches 1-7 are for 1.5.
>> 
>> Paolo Bonzini (9):
>>   qom: improve documentation of cast functions
>>   qom: allow casting of a NULL class
>>   qom: add a fast path to object_class_dynamic_cast
>>   qom: pass file/line/function to asserting casts
>>   qom: trace asserting casts
>>   qom: allow turning cast debugging off
>>   build: disable QOM cast debugging for official releases
>>   qom: simplify object_class_dynamic_cast, part 1
>>   qom: simplify object_class_dynamic_cast, part 2
>> 
>>  configure            | 20 ++++++++------
>>  include/qom/object.h | 40 ++++++++++++++++++++++-----
>>  qom/object.c         | 77 ++++++++++++++++++++++++++++++++++------------------
>>  trace-events         |  3 ++
>>  4 files changed, 99 insertions(+), 41 deletions(-)
>> 
>
> I have tested this series with qemu-system-ppc64, on a Core i7 2600 CPU.
> The process was set to a single core using taskset. Inside the guest
> (Debian ppc64 from debian-ports), I ran the command three times:
>
>   lintian g++-4.8_4.8.0-6_ppc64.deb
>
> I used lintian as it's a perl code, that trigger the discussion about
> sparc/ppc comparison.
>
> First of all with this patch series, the object_class_dynamic_cast calls
> went down to below 0.1% when using perf top. Before the patch series was
> applied, the command took in average on 3 runs 142.4s. With the patch
> series, it went down to 129.8s, so almost 9% faster.

I just posted another patch which I believe will also reduce this
overhead without eliminating the checks.

We do a staggering number of casts...  The patch I posted makes the
overwhelming majority of them nothing more than a single pointer
comparison and a couple derefs.

Regards,

Anthony Liguori

>
> To improve the performance a bit more, and come back to the same kind of
> code as before, we should move simple accessors from qom/*.c to
> include/qom/*.h and mark them as inline, so that they can be removed by
> the compiler. Currently, even if the function is simple it's still a
> call/ret in the hot path instead of a simple pointer addition.

>
> -- 
> Aurelien Jarno	                        GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 12:41:07PM -0500, Anthony Liguori wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Il 10/05/2013 16:39, Anthony Liguori ha scritto:
> >> I just oppose the notion of disabling casts and *especially* only
> >> disabling casts for official builds.
> >
> > This actually happens all the time.  Exactly this kind of type-safe cast
> > is disabled in releases of GCC, but enabled when building from svn trunk.
> 
> Let's assume for a moment that you are right and this behavior is what
> we should have.  Let's also assume there is a real regression here
> which has yet to have been established.

I have pointed out in another mail of this thread, that this type
checking account for about 9% of slowdown. Isn't that enough to say it's
a regression?

I still have to compare to 1.4, but it's not very fair, as a
significant work has been but on TCG to improve the performances,
especially on the target-ppc code.

I am currently preparing an image so that people can test that
themselves.

> As soon as we open up 1.6 for development, we face an immediate
> regression in performance.  So we need to fix the real problem here
> anyway.
> 
> I strongly suspect that if there is a problem, that optimizing
> leaf/concrete casts will take care of it.  Otherwise, a small lookup
> cache ought to do the trick too.  We can even use pointer comparisons
> for the lookup cache which should make it extremely cheap.
>
>
> Disabling casts doesn't give us a long term fix.  One of the solutions
> above does and patches also exist.

Why do you insist on using dynamic cast, on a hot path? It was using a
simple pointer before QOM in 1.4, it won't be less safe with if we use
a static cast instead of a dynamic one in 1.5.

With your arguments, we should drop all dataplane code, as it doesn't
use the QEMU core code. People should just not complain about the
performance.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Aurelien Jarno <aurelien@aurel32.net> writes:

> On Fri, May 10, 2013 at 12:41:07PM -0500, Anthony Liguori wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > Il 10/05/2013 16:39, Anthony Liguori ha scritto:
>> >> I just oppose the notion of disabling casts and *especially* only
>> >> disabling casts for official builds.
>> >
>> > This actually happens all the time.  Exactly this kind of type-safe cast
>> > is disabled in releases of GCC, but enabled when building from svn trunk.
>> 
>> Let's assume for a moment that you are right and this behavior is what
>> we should have.  Let's also assume there is a real regression here
>> which has yet to have been established.
>
> I have pointed out in another mail of this thread, that this type
> checking account for about 9% of slowdown. Isn't that enough to say it's
> a regression?

Yes, it's a regression.  I hadn't seen your note when I wrote the above.

I believe http://article.gmane.org/gmane.comp.emulators.qemu/210976
fixes the problem without eliminating any checking.

> I still have to compare to 1.4, but it's not very fair, as a
> significant work has been but on TCG to improve the performances,
> especially on the target-ppc code.
>
> I am currently preparing an image so that people can test that
> themselves.

I can reproduce large numbers of casts FWIW so no need to on my end.
I've confirmed the above patch fixes it.

>> Disabling casts doesn't give us a long term fix.  One of the solutions
>> above does and patches also exist.
>
> Why do you insist on using dynamic cast, on a hot path?

It's a question of safety.

> It was using a
> simple pointer before QOM in 1.4, it won't be less safe with if we use
> a static cast instead of a dynamic one in 1.5.

The patch I posted should make it equally fast.

> With your arguments, we should drop all dataplane code, as it doesn't
> use the QEMU core code. People should just not complain about the
> performance.

No one is complaining about performance.

I merely stated that if performance is a problem, give us a chance to
fix it before completely disabling a feature.

And indeed, now I have a fix that doesn't disable anything.

Regards,

Anthony Liguori

>
> -- 
> Aurelien Jarno                          GPG: 1024D/F1BCDB73
> aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 10/05/2013 19:41, Anthony Liguori ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 10/05/2013 16:39, Anthony Liguori ha scritto:
>>> I just oppose the notion of disabling casts and *especially* only
>>> disabling casts for official builds.
>>
>> This actually happens all the time.  Exactly this kind of type-safe cast
>> is disabled in releases of GCC, but enabled when building from svn trunk.
> 
> Let's assume for a moment that you are right and this behavior is what
> we should have.  Let's also assume there is a real regression here
> which has yet to have been established.

Aurelien timed the effect of my patch two hours before you sent this
message.  If it's not a regression in 1.5 (which is quite obvious from
the profile), it is a regression from the introduction of CPU classes
(1.3 or 1.4), when this code didn't exist at all.

And in 1.5 we introduced virtio-net casts as well (or did mst sneak in
his change anyway? ;)). If 10% is the effect of a few hundred
interrupts/sec, perhaps the same effect is visible on a few thousand
packets/sec.  I wouldn't bet against that one week from release.

(In fact, I'm not going to bet against that after release, either.  I'll
propose to disable QOM cast checking in Fedora and RHEL).

> As soon as we open up 1.6 for development, we face an immediate
> regression in performance.  So we need to fix the real problem here
> anyway.

No, we don't.  We will simply have to live with a debugging vs.
production tradeoff.  You will need to remember using the appropriate
option when doing performance tests on development releases.  We can
print a message if necessary, but it's really common practice.  What
about lockdep or might_sleep debugging or similar checks that Fedora
only enables for the kernel during development phases?

> I strongly suspect that if there is a problem, that optimizing
> leaf/concrete casts will take care of it.  Otherwise, a small lookup
> cache ought to do the trick too.  We can even use pointer comparisons
> for the lookup cache which should make it extremely cheap.

Still more expensive than no code at all.

> Let's independently evaluate your proposal for 1.6.

No, my proposal has to be evaluated for 1.5, and perhaps reverted later.
 We have a potentially large *set* of regressions (large amounts of QOM
casts were introduced in 1.5) that we found after -rc1, and the big
hammer is the only way to deal with it.

> Of course, these changes never make it into the tree which is an
> indication that the mechanism works very well :-)

Yes, it works *great*.  I don't want to give the impression that I think
the opposite.  But how often have you seen them abort in the distro
QEMU, as opposed to a development version?  And how often have you seen
"CRITICAL: foo != NULL" or "CRITICAL: GTK_IS_WIDGET (foo)" from a GTK+
app?  For me, it is never vs. quite often.

It works great simply because it's very cheap to add and makes debugging
much nicer.

> Note that the cast macros are a big improvement in code readability.
> The only real replacement would be static casts which would downgrade
> safety.

Yes, we should keep the cast macros, no doubt about that.  And the
checks, for development.  But adding tracing, while removing the actual
checks, is a pretty good speed compromise IMHO.  And leaving the checks
in before the freeze matches what most developers probably desire, and
avoids bitrotting of the check code.

> If you want to debate the merits of the safety, that's fine.
> 
>> Type-safe casts make sense in GTK+/GObject where: 1) type-safe casts
>> return NULL and log a "critical" error, they do not abort;  2) all
>> functions fail with another "critical" error if they are passed NULL.
>> We do neither, so we're just trading a crash now for a crash very soon
>> after (our call stacks tend to be relatively shallow, with some
>> exceptions such as the block layer).
> 
> The big assumption here is that dereference a NULL results in
> predictable failure.  This is not always the case and can lead to
> security vulnerabilities.

That's not what I was talking about.  I was talking about code like this:

void
gtk_window_set_wmclass (GtkWindow *window,
                        const gchar *wmclass_name,
                        const gchar *wmclass_class)
{
  GtkWindowPrivate *priv;

  g_return_if_fail (GTK_IS_WINDOW (window));
  priv = window->priv;

  ...
}

that avoids NULL pointer dereferences before they happen, while still
doing the checks.

Anyway, yes: the checks can catch some security vulnerabilities.  But
they won't catch many uses-after-free, they won't catch wrong
refcounting, they won't catch the _actual_ vulnerabilities that we had,
nor probably the ones that we could have.  Every segfault we fix could
potentially become a guest->host exploit with enough skill, the guest
has control of an enormous part of the QEMU address space.  But we fix
segfaults, not QOM cast failures.

Before QOM casts, I don't remember seeing many such failures _in the
tree_.  They of course happen during development, but QOM casts really
help before patches are posted.

> If you are concerned about the performance, provide a concrete example
> of poor performance and I will fix it.
> 
> If we find one that is unfixable, then we should consider your
> proposal.

Nothing is unfixable.  Worst of all, you can just stick static C casts
in the hot paths.  But that's what I want to avoid, I want my brain to
concentrate on the code on the hot paths, not on pointless differences
between those parts and the rest of a device.

Paolo

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 10/05/2013 19:41, Anthony Liguori ha scritto:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Il 10/05/2013 16:39, Anthony Liguori ha scritto:
>>>> I just oppose the notion of disabling casts and *especially* only
>>>> disabling casts for official builds.
>>>
>>> This actually happens all the time.  Exactly this kind of type-safe cast
>>> is disabled in releases of GCC, but enabled when building from svn trunk.
>> 
>> Let's assume for a moment that you are right and this behavior is what
>> we should have.  Let's also assume there is a real regression here
>> which has yet to have been established.
>
> Aurelien timed the effect of my patch two hours before you sent this
> message.  If it's not a regression in 1.5 (which is quite obvious from
> the profile), it is a regression from the introduction of CPU classes
> (1.3 or 1.4), when this code didn't exist at all.
>
> And in 1.5 we introduced virtio-net casts as well (or did mst sneak in
> his change anyway? ;)). If 10% is the effect of a few hundred
> interrupts/sec, perhaps the same effect is visible on a few thousand
> packets/sec.  I wouldn't bet against that one week from release.

The thing is, none of these casts should be for more than 1 level and
the patch I provided makes those casts almost free.

I believe the reason it didn't fix the problem for Aurelien is because
the string addresses were different because of different compilation
units.  I guess binutils doesn't collapse strings when linking.

>> As soon as we open up 1.6 for development, we face an immediate
>> regression in performance.  So we need to fix the real problem here
>> anyway.
>
> No, we don't.  We will simply have to live with a debugging vs.
> production tradeoff.

I appreciate all of the arguments below.  I'm very concerned about
reducing safety checks but am sympathetic to performance concerns.

Here's what I would like to do.  I'll apply 1-6 of your series which
gives us the infrastructure to disable qom casts.  That at least let's
the code get tested in case we need it.

I will hold off applying patch 7 hoping that the patch I provided to
Aurelien solves the problem.  If it doesn't and we're unable to find a
better solution, I'll apply patch 7 for the release.

Either way, the infrastructure is there for a distro to make a decision
although I think disabling qom casts is an extremely bad decision to
make.

Given the v2 version of my patch, I'm quite confident that casting in
the vast majority of circumstances would avoid a g_hash_table lookup so
that should eliminate any performance concerns.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Aurelien Jarno]

On Fri, May 10, 2013 at 10:59:22PM +0200, Paolo Bonzini wrote:
> Il 10/05/2013 19:41, Anthony Liguori ha scritto:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> >> Il 10/05/2013 16:39, Anthony Liguori ha scritto:
> >>> I just oppose the notion of disabling casts and *especially* only
> >>> disabling casts for official builds.
> >>
> >> This actually happens all the time.  Exactly this kind of type-safe cast
> >> is disabled in releases of GCC, but enabled when building from svn trunk.
> > 
> > Let's assume for a moment that you are right and this behavior is what
> > we should have.  Let's also assume there is a real regression here
> > which has yet to have been established.
> 
> Aurelien timed the effect of my patch two hours before you sent this
> message.  If it's not a regression in 1.5 (which is quite obvious from
> the profile), it is a regression from the introduction of CPU classes
> (1.3 or 1.4), when this code didn't exist at all.
> 
> And in 1.5 we introduced virtio-net casts as well (or did mst sneak in
> his change anyway? ;)). If 10% is the effect of a few hundred
> interrupts/sec, perhaps the same effect is visible on a few thousand
> packets/sec.  I wouldn't bet against that one week from release.

I have run perf top on a KVM x86-64 instance running iperf. I measured
around 3% of the CPU taken by g_hash_table_lookup + g_str_hash +
__strcmp_sse42. I don't know how it was before though.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Paolo Bonzini]

Il 11/05/2013 01:06, Anthony Liguori ha scritto:
> The thing is, none of these casts should be for more than 1 level and
> the patch I provided makes those casts almost free.

Assuming strings are collapsed (and that's why my original attempt to
introduce the fast path only looked at the concrete classes: I was
trying to optimize it within a device model).

> I believe the reason it didn't fix the problem for Aurelien is because
> the string addresses were different because of different compilation
> units.  I guess binutils doesn't collapse strings when linking.

No, it doesn't.  GCC can do that if you use link-time optimization, but
binutils doesn't look into the data section.  It is a black box.

>>> As soon as we open up 1.6 for development, we face an immediate
>>> regression in performance.  So we need to fix the real problem here
>>> anyway.
>>
>> No, we don't.  We will simply have to live with a debugging vs.
>> production tradeoff.
> 
> I appreciate all of the arguments below.  I'm very concerned about
> reducing safety checks but am sympathetic to performance concerns.
> 
> Here's what I would like to do.  I'll apply 1-6 of your series which
> gives us the infrastructure to disable qom casts.  That at least let's
> the code get tested in case we need it.

Fair enough, thanks!  Are you going to replace patch 3 with yours?
(Though I don't think we need to check the parent, see above).

> I will hold off applying patch 7 hoping that the patch I provided to
> Aurelien solves the problem.  If it doesn't and we're unable to find a
> better solution, I'll apply patch 7 for the release.

Let's please test virtio-net as well, it looks like Aurelien has the
benchmark.  /me goes swimming now. :)

Paolo

Re: [Qemu-devel] [PATCH for-1.5 0/9] Disable expensive QOM cast debugging for official releases

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori