Re: [Qemu-devel] [RFC 5/8] qom/object: update class cache atomically

[Marc-André Lureau]

Hi

On Mon, Sep 19, 2016 at 7:54 PM Alex Bennée <address@hidden> wrote:

> The idiom CPU_GET_CLASS(cpu) is fairly extensively used in various
> threads and trips of ThreadSanitizer due to the fact it updates
> obj->class->object_cast_cache behind the scenes. As this is just a
> fast-path cache there is no need to lock updates just ensure that we
> don't get torn-updates from two racing lookups. While this is unlikely
> on x86 we use the plain atomic_read/set primitives to make this
> explicit and keep the sanitizer happy.
>
> Signed-off-by: Alex Bennée <address@hidden>
>

Looks fine to me, would be nicer to have an idea of the performance hit,
but I suppose it is marginal.

btw, object_dynamic_cast_assert code is a bit weird: it always inserts at
the end of the array, and shifts the other cached values down (why?). If
there are class hierarchies with a depth and interfaces over 4
(OBJECT_CLASS_CAST_CACHE) this looks like it may be inefficient, no? I
can't find performance tests for object, perhaps it doesn't matter after
all.

Reviewed-by: Marc-André Lureau <address@hidden>


> ---
>  qom/object.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index 8166b7d..7a05e35 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -614,7 +614,7 @@ Object *object_dynamic_cast_assert(Object *obj, const
> char *typename,
>      Object *inst;
>
>      for (i = 0; obj && i < OBJECT_CLASS_CAST_CACHE; i++) {
> -        if (obj->class->object_cast_cache[i] == typename) {
> +        if (atomic_read(&obj->class->object_cast_cache[i]) == typename) {
>              goto out;
>          }
>      }
> @@ -631,10 +631,10 @@ Object *object_dynamic_cast_assert(Object *obj,
> const char *typename,
>
>      if (obj && obj == inst) {
>          for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
> -            obj->class->object_cast_cache[i - 1] =
> -                    obj->class->object_cast_cache[i];
> +            atomic_set(&obj->class->object_cast_cache[i - 1],
> +                       atomic_read(&obj->class->object_cast_cache[i]));
>          }
> -        obj->class->object_cast_cache[i - 1] = typename;
> +        atomic_set(&obj->class->object_cast_cache[i - 1], typename);
>      }
>
>  out:
> @@ -704,7 +704,7 @@ ObjectClass
> *object_class_dynamic_cast_assert(ObjectClass *class,
>      int i;
>
>      for (i = 0; class && i < OBJECT_CLASS_CAST_CACHE; i++) {
> -        if (class->class_cast_cache[i] == typename) {
> +        if (atomic_read(&class->class_cast_cache[i]) == typename) {
>              ret = class;
>              goto out;
>          }
> @@ -725,9 +725,10 @@ ObjectClass
> *object_class_dynamic_cast_assert(ObjectClass *class,
>  #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];
> +            atomic_set(&class->class_cast_cache[i - 1],
> +                       atomic_read(&class->class_cast_cache[i]));
>          }
> -        class->class_cast_cache[i - 1] = typename;
> +        atomic_set(&class->class_cast_cache[i - 1], typename);
>      }
>  out:
>  #endif
> --
> 2.9.3
>
>
> --
Marc-André Lureau