[PATCH 1/2] Optimize generic spinlock code and use C11 like atomic macros.

[Stefan Liebler]

On 02/28/2017 08:15 AM, Torvald Riegel wrote:
> On Mon, 2017-02-27 at 17:57 +0000, Szabolcs Nagy wrote:
>> On 26/02/17 20:35, Torvald Riegel wrote:
>>> On Sun, 2017-02-26 at 21:29 +0100, Florian Weimer wrote:
>>>> * Torvald Riegel:
>>>>
>>>>>> … like this, it would change the C++ name mangling of anything related
>>>>>> to pthread_spinlock_t.  It is defined as a typedef, so the mangling
>>>>>> uses the definition to refer to the type, not the name, according to
>>>>>> the language rules, where typedef does not create a new type, and
>>>>>> typedefs with the same definition can be used interchangeably.
>>>>>
>>>>> I'm not saying that we should change the definition to a union.  What
>>>>> 2.14.2 in the document cited above states is that the pointers to the
>>>>> union and the individual parts are interchangeable.  So we can use a
>>>>> pointer to a part (ie, non-volatile) interchangeably with the pointer to
>>>>> the union that we use internally.
>>>>
>>>> The relevant quote from that document (C memory object and value
>>>> semantics: the space of de facto and ISO standards) is:
>>>>
>>>> | The standard says: 6.7.2.1p16 “The size of a union is
>>>> | sufficient to contain the largest of its members. The value of
>>>> | at most one of the members can be stored in a union object
>>>> | at any time. *A pointer to a union object, suitably converted,*
>>>> | *points to each of its members (or if a member is a bit-field,*
>>>> | *then to the unit in which it resides), and vice versa.*” (bold
>>>> | emphasis added).
>>>>
>>>> So I think this is only valid if you actually start with a union
>>>> object.  A plain top-level definition of a volatile int is not a union
>>>> member, so this rule does not apply (in the “vice versa” part).
>>>
>>> Then we'll have to agree to disagree.  What do others think about this
>>> question?
>>
>> i think the standard says that
>>
>> volatile int x;
>> *(int*)&x;
>>
>> is undefined and i think
>>
>> int r = *p;
>
> At least on the architectures that already use the __atomic builtins, we
> never perform such an access -- everything goes through __atomic*.  On
> other architectures, we already use plain, non-volatile accesses to
> emulate real atomics (eg, see atomic_store_relaxed).  ISTM that we
> wouldn't be making things worse in the sense that we already make
> assumptions about what the compiler would do or would not (e.g., the
> compiler does not store to *p again as in your third example).
>

Yes, architectures which use the C11 atomics passes the volatile int * 
to the __atomic* builtins.

For architectures not using C11 atomics:
 From the spinlock perspective, the latest patch does not access a 
casted pointer which discards the volatile qualifier.

Instead in atomic_exchange_acq there is a
read from a volatile int * into a local int variable.
The volatile int * is passed to
atomic_compare_and_exchange_bool_acq macro.
The passed oldval / newval variables have type int instead
of volatile int.

atomic_load_relaxed is now using a local variable of type int instead of 
volatile int. Reading from the volatile int * is done by gcc via inline 
assembly. Afterwards the local int variable is returned to the user of 
this macro.

__atomic_val_bysize is now using a local variable of type int instead
of volatile int. The return value of pre##_<sizeof(*mem)>_##post is 
stored in the local variable and then returned to the user of this macro.

If this is okay, I prefer to use the changes from the latest patch.
Then this will fix - if compiled with GCC >= 5 - the usage of
the atomic macros in include/atomic.h with volatile int *
for architectures which do not define USE_ATOMIC_COMPILER_BUILTINS
to one.

If this is not okay, I would skip those changes in include/atomic.h
for this patch.
After we have a solution, we should work on a separate patch.
Even if gcc provides an extension as mentioned by Florian, I think it 
will only be available with new GCCs. What about the current GCCs?

Bye
Stefan