Re: [Qemu-devel] [PATCH v5 07/18] qemu-thread: add simple test-and-set spinlock

[Emilio G. Cota]

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 <address@hidden>
Date:   Thu Apr 28 10:56:26 2016 -0400

    atomics: add atomic_test_and_set_acquire
    
    Signed-off-by: Emilio G. Cota <address@hidden>

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