Re: [RFC 2/7] eal: add generic bit manipulation macros

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

On 2024-03-05 19:22, Tyler Retzlaff wrote:
> On Tue, Mar 05, 2024 at 07:08:36PM +0100, Mattias Rönnblom wrote:
>> On 2024-03-04 17:42, Tyler Retzlaff wrote:
>>> On Sat, Mar 02, 2024 at 02:53:23PM +0100, Mattias Rönnblom wrote:
>>>> Add bit-level test/set/clear/assign macros operating on both 32-bit
>>>> and 64-bit words by means of C11 generic selection.
>>>>
>>>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>>>> ---
>>>
>>> _Generic is nice here. should we discourage direct use of the inline
>>> functions in preference of using the macro always? either way lgtm.
>>>
>>
>> That was something I considered, but decided against it for RFC v1.
>> I wasn't even sure people would like _Generic.
>>
>> The big upside of having only the _Generic macros would be a much
>> smaller API, but maybe a tiny bit less (type-)safe to use.
> 
> i'm curious what misuse pattern you anticipate or have seen that may be
> less type-safe? just so i can look out for them.
> 

That was just a gut feeling, not to be taken too seriously.

uint32_t *p = some_void_pointer;
/../
rte_bit_set32(p, 17);

A code section like this is redundant in the way the type (or at least 
type size) is coded both into the function name, and the pointer type. 
The use of rte_set_bit() will eliminate this, which is good (DRY), and 
bad, because now the type isn't "double-checked".

As you can see, it's a pretty weak argument.

> i (perhaps naively) have liked generic functions for their selection of
> the "correct" type and for _Generic if no leg/case exists compiler
> error (as opposed to e.g. silent truncation).
> 
>>
>> Also, _Generic is new for DPDK, so who knows what issues it might
>> cause with old compilers.
> 
> i was thinking about this overnight, it's supposed to be standard C11
> and my use on various compilers showed no problem but I can't recall if
> i did any evaluation when consuming as a part of a C++ translation unit
> so there could be problems.
> 

It would be unfortunate if DPDK was prohibited from using _Generic.

>>
>> Thanks.
>>
>>> Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
>>>
>>>>   lib/eal/include/rte_bitops.h | 81 ++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 81 insertions(+)
>>>>
>>>> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
>>>> index 9a368724d5..afd0f11033 100644
>>>> --- a/lib/eal/include/rte_bitops.h
>>>> +++ b/lib/eal/include/rte_bitops.h
>>>> @@ -107,6 +107,87 @@ extern "C" {
>>>>   #define RTE_FIELD_GET64(mask, reg) \
>>>>   		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
>>>> +/**
>>>> + * Test bit in word.
>>>> + *
>>>> + * Generic selection macro to test the value of a bit in a 32-bit or
>>>> + * 64-bit word. The type of operation depends on the type of the @c
>>>> + * addr parameter.
>>>> + *
>>>> + * This macro does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the word to modify.
>>>> + * @param nr
>>>> + *   The index of the bit.
>>>> + */
>>>> +#define rte_bit_test(addr, nr)				\
>>>> +	_Generic((addr),				\
>>>> +		 uint32_t *: rte_bit_test32,		\
>>>> +		 uint64_t *: rte_bit_test64)(addr, nr)
>>>> +
>>>> +/**
>>>> + * Set bit in word.
>>>> + *
>>>> + * Generic selection macro to set a bit in a 32-bit or 64-bit
>>>> + * word. The type of operation depends on the type of the @c addr
>>>> + * parameter.
>>>> + *
>>>> + * This macro does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the word to modify.
>>>> + * @param nr
>>>> + *   The index of the bit.
>>>> + */
>>>> +#define rte_bit_set(addr, nr)				\
>>>> +	_Generic((addr),				\
>>>> +		 uint32_t *: rte_bit_set32,		\
>>>> +		 uint64_t *: rte_bit_set64)(addr, nr)
>>>> +
>>>> +/**
>>>> + * Clear bit in word.
>>>> + *
>>>> + * Generic selection macro to clear a bit in a 32-bit or 64-bit
>>>> + * word. The type of operation depends on the type of the @c addr
>>>> + * parameter.
>>>> + *
>>>> + * This macro does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the word to modify.
>>>> + * @param nr
>>>> + *   The index of the bit.
>>>> + */
>>>> +#define rte_bit_clear(addr, nr)			\
>>>> +	_Generic((addr),				\
>>>> +		 uint32_t *: rte_bit_clear32,		\
>>>> +		 uint64_t *: rte_bit_clear64)(addr, nr)
>>>> +
>>>> +/**
>>>> + * Assign a value to a bit in word.
>>>> + *
>>>> + * Generic selection macro to assign a value to a bit in a 32-bit or 64-bit
>>>> + * word. The type of operation depends on the type of the @c addr parameter.
>>>> + *
>>>> + * This macro does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the word to modify.
>>>> + * @param nr
>>>> + *   The index of the bit.
>>>> + * @param value
>>>> + *   The new value of the bit - true for '1', or false for '0'.
>>>> + */
>>>> +#define rte_bit_assign(addr, nr, value)			\
>>>> +	_Generic((addr),				\
>>>> +		 uint32_t *: rte_bit_assign32,			\
>>>> +		 uint64_t *: rte_bit_assign64)(addr, nr, value)
>>>> +
>>>>   /**
>>>>    * Test if a particular bit in a 32-bit word is set.
>>>>    *
>>>> -- 
>>>> 2.34.1