diff mbox

qom and debug

Message ID 20160830133741.776e8ff0.cornelia.huck@de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Cornelia Huck Aug. 30, 2016, 11:37 a.m. UTC 
On Tue, 30 Aug 2016 14:15:14 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Aug 30, 2016 at 01:11:05PM +0200, Cornelia Huck wrote:
> > On Tue, 30 Aug 2016 13:21:23 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > BTW downstreams are building with --disable-qom-cast-debug which drops
> > > all QOM casts on data path - one way is to say we just make this the
> > > default upstream as well. Another to say that we want to distinguish
> > > fast path calls from slow path, this way we will be able to bring back
> > > some of the checks.
> > 
> > I find CONFIG_QOM_CAST_DEBUG a bit inconsistent, btw:
> > 
> > - for object casts, we optimize away all checks and just return the
> > object for !debug
> > - for class casts, we optimize away only the caching and still keep the
> > checking (why would we drop the caching if this can speed up things?)
> > 
> > We certainly want to have debug turned on during development to avoid
> > nasty surprises later (otherwise, why even bother?), but it makes sense
> > to turn it off for a release. (Is there an easy way to turn it off for
> > the release, normal or stable, and keep it during the development
> > cycle?)
> 
> I think the assumption was class casts are not on data path.
> Ideally we'd keep it on for release too for non-datapath things,
> to help improve security.

This would probably need some more fine-grained configuration.

For now, what about this completely untested patch that at least adds
caching for the class->interfaces case if debug is off?

Comments

Michael S. Tsirkin Aug. 30, 2016, 11:57 a.m. UTC | #1 
On Tue, Aug 30, 2016 at 01:37:41PM +0200, Cornelia Huck wrote:
> On Tue, 30 Aug 2016 14:15:14 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Aug 30, 2016 at 01:11:05PM +0200, Cornelia Huck wrote:
> > > On Tue, 30 Aug 2016 13:21:23 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > BTW downstreams are building with --disable-qom-cast-debug which drops
> > > > all QOM casts on data path - one way is to say we just make this the
> > > > default upstream as well. Another to say that we want to distinguish
> > > > fast path calls from slow path, this way we will be able to bring back
> > > > some of the checks.
> > > 
> > > I find CONFIG_QOM_CAST_DEBUG a bit inconsistent, btw:
> > > 
> > > - for object casts, we optimize away all checks and just return the
> > > object for !debug
> > > - for class casts, we optimize away only the caching and still keep the
> > > checking (why would we drop the caching if this can speed up things?)
> > > 
> > > We certainly want to have debug turned on during development to avoid
> > > nasty surprises later (otherwise, why even bother?), but it makes sense
> > > to turn it off for a release. (Is there an easy way to turn it off for
> > > the release, normal or stable, and keep it during the development
> > > cycle?)
> > 
> > I think the assumption was class casts are not on data path.
> > Ideally we'd keep it on for release too for non-datapath things,
> > to help improve security.
> 
> This would probably need some more fine-grained configuration.

We can start with Jason's patch :)

> For now, what about this completely untested patch that at least adds
> caching for the class->interfaces case if debug is off?

Fine with me.

> diff --git a/qom/object.c b/qom/object.c
> index 8166b7d..05f1fe4 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -696,12 +696,16 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
>                                                const char *func)
>  {
>      ObjectClass *ret;
> +    int i;
>  
>      trace_object_class_dynamic_cast_assert(class ? class->type->name : "(null)",
>                                             typename, file, line, func);
>  
> -#ifdef CONFIG_QOM_CAST_DEBUG
> -    int i;
> +#ifndef CONFIG_QOM_CAST_DEBUG
> +    if (!class || !class->interfaces) {
> +        return class;
> +    }
> +#endif
>  
>      for (i = 0; class && i < OBJECT_CLASS_CAST_CACHE; i++) {
>          if (class->class_cast_cache[i] == typename) {
> @@ -709,11 +713,6 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
>              goto out;
>          }
>      }
> -#else
> -    if (!class || !class->interfaces) {
> -        return class;
> -    }
> -#endif
>  
>      ret = object_class_dynamic_cast(class, typename);
>      if (!ret && class) {
> @@ -722,7 +721,6 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
>          abort();
>      }
>  
> -#ifdef CONFIG_QOM_CAST_DEBUG
>      if (class && ret == class) {
>          for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
>              class->class_cast_cache[i - 1] = class->class_cast_cache[i];
> @@ -730,7 +728,6 @@ ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
>          class->class_cast_cache[i - 1] = typename;
>      }
>  out:
> -#endif
>      return ret;
>  }
>
diff mbox

Patch

diff --git a/qom/object.c b/qom/object.c
index 8166b7d..05f1fe4 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -696,12 +696,16 @@  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
                                               const char *func)
 {
     ObjectClass *ret;
+    int i;
 
     trace_object_class_dynamic_cast_assert(class ? class->type->name : "(null)",
                                            typename, file, line, func);
 
-#ifdef CONFIG_QOM_CAST_DEBUG
-    int i;
+#ifndef CONFIG_QOM_CAST_DEBUG
+    if (!class || !class->interfaces) {
+        return class;
+    }
+#endif
 
     for (i = 0; class && i < OBJECT_CLASS_CAST_CACHE; i++) {
         if (class->class_cast_cache[i] == typename) {
@@ -709,11 +713,6 @@  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
             goto out;
         }
     }
-#else
-    if (!class || !class->interfaces) {
-        return class;
-    }
-#endif
 
     ret = object_class_dynamic_cast(class, typename);
     if (!ret && class) {
@@ -722,7 +721,6 @@  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
         abort();
     }
 
-#ifdef CONFIG_QOM_CAST_DEBUG
     if (class && ret == class) {
         for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
             class->class_cast_cache[i - 1] = class->class_cast_cache[i];
@@ -730,7 +728,6 @@  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
         class->class_cast_cache[i - 1] = typename;
     }
 out:
-#endif
     return ret;
 }