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