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

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

On 2024-04-25 12:25, Morten Brørup wrote:
>> +#define rte_bit_atomic_test(addr, nr, memory_order)			\
>> +	_Generic((addr),						\
>> +		 uint32_t *: __rte_bit_atomic_test32,			\
>> +		 uint64_t *: __rte_bit_atomic_test64)(addr, nr, memory_order)
> 
> I wonder if these should have RTE_ATOMIC qualifier:
> 
> +		 RTE_ATOMIC(uint32_t) *: __rte_bit_atomic_test32,			\
> +		 RTE_ATOMIC(uint64_t) *: __rte_bit_atomic_test64)(addr, nr, memory_order)
> 
> 
>> +#define __RTE_GEN_BIT_ATOMIC_TEST(size)					\
>> +	static inline bool						\
>> +	__rte_bit_atomic_test ## size(const uint ## size ## _t *addr,	\
> 
> I wonder if the "addr" parameter should have RTE_ATOMIC qualifier:
> 
> +	__rte_bit_atomic_test ## size(const RTE_ATOMIC(uint ## size ## _t) *addr,	\
> 
> instead of casting into a_addr.
> 

Check the cover letter for the rationale for the cast.

Where I'm at now is that I think C11 _Atomic is rather poor design. The 
assumption that an object which allows for atomic access always should 
require all operations upon it to be atomic, regardless of where it is 
in its lifetime, and which thread is accessing it, does not hold, in the 
general case.

The only reason for _Atomic being as it is, as far as I can see, is to 
accommodate for ISAs which does not have the appropriate atomic machine 
instructions, and thus require a lock or some other data associated with 
the actual user-data-carrying bits. Neither GCC nor DPDK supports any 
such ISAs, to my knowledge. I suspect neither never will. So the cast 
will continue to work.

>> +				      unsigned int nr, int memory_order) \
>> +	{								\
>> +		RTE_ASSERT(nr < size);					\
>> +									\
>> +		const RTE_ATOMIC(uint ## size ## _t) *a_addr =		\
>> +			(const RTE_ATOMIC(uint ## size ## _t) *)addr;	\
>> +		uint ## size ## _t mask = (uint ## size ## _t)1 << nr;	\
>> +		return rte_atomic_load_explicit(a_addr, memory_order) & mask; \
>> +	}
> 
> 
> Similar considerations regarding volatile qualifier for the "once" operations.
>