On Tue, May 17, 2016 at 12:19:27 -0700, Richard Henderson wrote:
> On 05/17/2016 10:13 AM, Sergey Fedorov wrote:
> >> > +static inline void qemu_spin_lock(QemuSpin *spin)
> >> > +{
> >> > + while (atomic_test_and_set_acquire(&spin->value)) {
> >>From gcc-4.8 info page, node "__atomic Builtins", description of
> > __atomic_test_and_set():
> >
> > It should be only used for operands of type 'bool' or 'char'.
> >
>
> Hum. I thought I remembered all operand sizes there, but I've just re-checked
> and you're right about bool (and really only bool).
>
> Perhaps we should just stick with __sync_test_and_set then. I'm thinking here
> of e.g. armv6, a reasonable host, which can't operate on 1 byte atomic values.
I like this idea, it gets rid of any guesswork (as in my previous email).
I've changed the patch to:
commit 8f89d36b6203b78df2bf1e3f82871b8aa2ca83b7
Author: Emilio G. Cota <c...@braap.org>
Date: Thu Apr 28 10:56:26 2016 -0400
atomics: add atomic_test_and_set_acquire
Signed-off-by: Emilio G. Cota <c...@braap.org>
diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 5bc4d6c..95de7a7 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -113,6 +113,13 @@
} while(0)
#endif
+/*
+ * We might we tempted to use __atomic_test_and_set with __ATOMIC_ACQUIRE;
+ * however, the documentation explicitly says that we should only pass
+ * a boolean to it, so we use __sync_lock_test_and_set, which doesn't
+ * have this limitation, and is documented to have acquire semantics.
+ */
+#define atomic_test_and_set_acquire(ptr) __sync_lock_test_and_set(ptr, true)
/* All the remaining operations are fully sequentially consistent */
@@ -327,6 +334,8 @@
#endif
#endif
+#define atomic_test_and_set_acquire(ptr) __sync_lock_test_and_set(ptr, true)
+
/* Provide shorter names for GCC atomic builtins. */
#define atomic_fetch_inc(ptr) __sync_fetch_and_add(ptr, 1)
#define atomic_fetch_dec(ptr) __sync_fetch_and_add(ptr, -1)
---
An alternative would be to add just a single line, right below the
barrier() definition or at the end of the file. Adding both lines
is IMO a bit clearer, since the newly-added comment only applies
under the C11 definitions.
Thanks,
Emilio
