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

[Alex Bennée]

Marc-André Lureau <address@hidden> writes:

> 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.

I was surprised that CONFIG_QOM_CAST_DEBUG is the default because it
does a bunch of stuff on every cast. The other option of course would be
to use --disable-qom-cast-debug when building for sanitizers although
maybe we should just be defaulting to off?

> 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.

TBH the whole object model thing is a bit of a mystery to me that I
haven't delved that far into it. I guess I should learn about it some
more at some point.

>
> 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


--
Alex Bennée