Re: [RFC v6 5/6] eal: add atomic bit operations

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-05-04 01:30, Tyler Retzlaff wrote:
> On Fri, May 03, 2024 at 08:41:09AM +0200, Mattias Rönnblom wrote:
>> On 2024-05-02 07:57, Mattias Rönnblom wrote:
>>> Add atomic bit test/set/clear/assign/flip and
>>> test-and-set/clear/assign/flip functions.
>>>
>>> All atomic bit functions allow (and indeed, require) the caller to
>>> specify a memory order.
>>>
>>> RFC v6:
>>>   * Have rte_bit_atomic_test() accept const-marked bitsets.
>>>
>>> RFC v4:
>>>   * Add atomic bit flip.
>>>   * Mark macro-generated private functions experimental.
>>>
>>> RFC v3:
>>>   * Work around lack of C++ support for _Generic (Tyler Retzlaff).
>>>
>>> RFC v2:
>>>   o Add rte_bit_atomic_test_and_assign() (for consistency).
>>>   o Fix bugs in rte_bit_atomic_test_and_[set|clear]().
>>>   o Use <rte_stdatomics.h> to support MSVC.
>>>
>>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>>> ---
>>>   lib/eal/include/rte_bitops.h | 428 +++++++++++++++++++++++++++++++++++
>>>   1 file changed, 428 insertions(+)
>>>
>>> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
>>> index caec4f36bb..9cde982113 100644
>>> --- a/lib/eal/include/rte_bitops.h
>>> +++ b/lib/eal/include/rte_bitops.h
>>> @@ -21,6 +21,7 @@
>>>   #include <rte_compat.h>
>>>   #include <rte_debug.h>
>>> +#include <rte_stdatomic.h>
>>>   #ifdef __cplusplus
>>>   extern "C" {
>>> @@ -399,6 +400,202 @@ extern "C" {
>>>   		 uint32_t *: __rte_bit_once_flip32,		\
>>>   		 uint64_t *: __rte_bit_once_flip64)(addr, nr)
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice.
>>> + *
>>> + * Test if a particular bit in a word is set with a particular memory
>>> + * order.
>>> + *
>>> + * Test a bit with the resulting memory load ordered as per the
>>> + * specified memory order.
>>> + *
>>> + * @param addr
>>> + *   A pointer to the word to query.
>>> + * @param nr
>>> + *   The index of the bit.
>>> + * @param memory_order
>>> + *   The memory order to use. See <rte_stdatomics.h> for details.
>>> + * @return
>>> + *   Returns true if the bit is set, and false otherwise.
>>> + */
>>> +#define rte_bit_atomic_test(addr, nr, memory_order)			\
>>> +	_Generic((addr),						\
>>> +		 uint32_t *: __rte_bit_atomic_test32,			\
>>> +		 const uint32_t *: __rte_bit_atomic_test32,		\
>>> +		 uint64_t *: __rte_bit_atomic_test64,			\
>>> +		 const uint64_t *: __rte_bit_atomic_test64)(addr, nr,	\
>>> +							    memory_order)
>>
>> Should __rte_bit_atomic_test32()'s addr parameter be marked
>> volatile, and two volatile-marked branches added to the above list?
> 
> off-topic comment relating to the generic type selection list above, i was
> reading C17 DR481 recently and i think we may want to avoid providing
> qualified and unauqlified types in the list.
> 
> DR481: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2396.htm#dr_481
> 
> "the controlling expression of a generic selection shall have type
> compatibile with at most one of the types named in its generic
> association list."
> 

Const and unqualified pointers are not compatible. Without the "const 
uint32_t *" element in the association list, passing const-qualified 
pointers to rte_bit_test() will cause a compiler error.

So, if you want to support both passing const-qualified and unqualified 
pointers (which you do, obviously), then you have no other option than 
to treat them separately.

GCC, clang and ICC all seem to agree on this. The standard also is 
pretty clear on this, from what I can tell. "No two generic associations 
in the same generic selection shall specify compatible types." (6.5.1.1, 
note *compatible*). "For two pointer types to be compatible, both shall 
be identically qualified and both shall be pointers to compatible 
types." (6.7.6.1)

> "the type of the controlling expression is the type of the expression as
> if it had undergone an lvalue conversion"
> 
> "lvalue conversion drops type qualifiers"
> 
> so the unqualified type of the controlling expression is only matched
> selection list which i guess that means the qualified entries in the
> list are never selected.
> 
> i suppose the implication here is we couldn't then provide 2 inline
> functions one for volatile qualified and for not volatile qualified.
> 
> as for a single function where the parameter is or isn't volatile
> qualified. if we're always forwarding to an intrinsic i've always
> assumed (perhaps incorrectly) that the intrinsic itself did what was
> semantically correct even without qualification.
> 
> as you note i believe there is a convenience element in providing the
> volatile qualified version since it means the function like macro /
> inline function will accept both volatile qualified and unqualified
> whereas if we did not qualify the parameter it would require the
> caller/user to strip the volatile qualification if present with casts.
> 
> i imagine in most cases we are just forwarding, in which case it seems
> not horrible to provide the qualified version.
> 
>> Both the C11-style GCC built-ins and the C11-proper atomic functions
>> have addresses marked volatile. The Linux kernel and the old __sync
>> GCC built-ins on the other hand, doesn't (although I think you still
>> get volatile semantics). The only point of "volatile", as far as I
>> can see, is to avoid warnings in case the user passed a
>> volatile-marked pointer. The drawback is that *you're asking for
>> volatile semantics*, although with the current compilers, it seems
>> like that is what you get, regardless if you asked for it or not.
>>
>> Just to be clear: even these functions would accept volatile-marked
>> pointers, non-volatile pointers should be accepted as well (and
>> should generally be preferred).
>>
>> Isn't parallel programming in C lovely.
> 
> it's super!
> 
>>
>> <snip>