| Message ID | 20160919155139.28371-6-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi On Mon, Sep 19, 2016 at 7:54 PM Alex Bennée <alex.bennee@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.bennee@linaro.org> > 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 <marcandre.lureau@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
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Sep 19, 2016 at 7:54 PM Alex Bennée <alex.bennee@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.bennee@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.lureau@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
On 20/09/2016 16:59, Alex Bennée wrote: > > 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? The casts are not too expensive and usually are not in the fast paths (we use explicit C casts there) so we were leaving them enabled. They do find bugs sometimes. Paolo
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
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.bennee@linaro.org> --- qom/object.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)