Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Mon, Sep 19, 2016 at 7:54 PM Alex Bennée <alex.ben...@linaro.org> 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 <alex.ben...@linaro.org>
>>
>
> 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 <marcandre.lur...@redhat.com>
>
>
>> ---
>> 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
