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