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

[Sergey Fedorov]

On 17/05/16 23:04, Emilio G. Cota wrote:
> 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)

So you are going to stick to *legacy* built-ins?

Kind regards,
Sergey

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