[RFC,1/7] eal: extend bit manipulation functions

Message ID 20240302135328.531940-2-mattias.ronnblom@ericsson.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers
Series Improve EAL bit operations API |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Mattias Rönnblom March 2, 2024, 1:53 p.m. UTC
  Add functionality to test, set, clear, and assign the value to
individual bits in 32-bit or 64-bit words.

These functions have no implications on memory ordering, atomicity and
does not use volatile and thus does not prevent any compiler
optimizations.

Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
---
 lib/eal/include/rte_bitops.h | 194 ++++++++++++++++++++++++++++++++++-
 1 file changed, 192 insertions(+), 2 deletions(-)
  

Comments

Stephen Hemminger March 2, 2024, 5:05 p.m. UTC | #1
On Sat, 2 Mar 2024 14:53:22 +0100
Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:

> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> index 449565eeae..9a368724d5 100644
> --- a/lib/eal/include/rte_bitops.h
> +++ b/lib/eal/include/rte_bitops.h
> @@ -2,6 +2,7 @@
>   * Copyright(c) 2020 Arm Limited
>   * Copyright(c) 2010-2019 Intel Corporation
>   * Copyright(c) 2023 Microsoft Corporation
> + * Copyright(c) 2024 Ericsson AB
>   */
>  

Unless this is coming from another project code base, the common
practice is not to add copyright for each contributor in later versions.

> +/**
> + * Test if a particular bit in a 32-bit word is set.
> + *
> + * This function does not give any guarantees in regards to memory
> + * ordering or atomicity.
> + *
> + * @param addr
> + *   A pointer to the 32-bit word to query.
> + * @param nr
> + *   The index of the bit (0-31).
> + * @return
> + *   Returns true if the bit is set, and false otherwise.
> + */
> +static inline bool
> +rte_bit_test32(const uint32_t *addr, unsigned int nr);

Is it possible to reorder these inlines to avoid having
forward declarations?

Also, new functions should be marked __rte_experimental
for a release or two.
  
Mattias Rönnblom March 3, 2024, 6:26 a.m. UTC | #2
On 2024-03-02 18:05, Stephen Hemminger wrote:
> On Sat, 2 Mar 2024 14:53:22 +0100
> Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> 
>> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
>> index 449565eeae..9a368724d5 100644
>> --- a/lib/eal/include/rte_bitops.h
>> +++ b/lib/eal/include/rte_bitops.h
>> @@ -2,6 +2,7 @@
>>    * Copyright(c) 2020 Arm Limited
>>    * Copyright(c) 2010-2019 Intel Corporation
>>    * Copyright(c) 2023 Microsoft Corporation
>> + * Copyright(c) 2024 Ericsson AB
>>    */
>>   
> 
> Unless this is coming from another project code base, the common
> practice is not to add copyright for each contributor in later versions.
> 

Unless it's a large contribution (compared to the rest of the file)?

I guess that's why the 916c50d commit adds the Microsoft copyright notice.

>> +/**
>> + * Test if a particular bit in a 32-bit word is set.
>> + *
>> + * This function does not give any guarantees in regards to memory
>> + * ordering or atomicity.
>> + *
>> + * @param addr
>> + *   A pointer to the 32-bit word to query.
>> + * @param nr
>> + *   The index of the bit (0-31).
>> + * @return
>> + *   Returns true if the bit is set, and false otherwise.
>> + */
>> +static inline bool
>> +rte_bit_test32(const uint32_t *addr, unsigned int nr);
> 
> Is it possible to reorder these inlines to avoid having
> forward declarations?
> 

Yes, but I'm not sure it's a net gain.

A statement expression macro seems like a perfect tool for the job, but 
then MSVC doesn't support statement expressions. You could also have a 
macro that just generate the function body, as oppose to the whole function.

I'll consider if I should just bite the bullet and expand all the 
macros. 4x duplication.

> Also, new functions should be marked __rte_experimental
> for a release or two.

Yes, thanks.
  
Tyler Retzlaff March 4, 2024, 4:34 p.m. UTC | #3
On Sun, Mar 03, 2024 at 07:26:36AM +0100, Mattias Rönnblom wrote:
> On 2024-03-02 18:05, Stephen Hemminger wrote:
> >On Sat, 2 Mar 2024 14:53:22 +0100
> >Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> >
> >>diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> >>index 449565eeae..9a368724d5 100644
> >>--- a/lib/eal/include/rte_bitops.h
> >>+++ b/lib/eal/include/rte_bitops.h
> >>@@ -2,6 +2,7 @@
> >>   * Copyright(c) 2020 Arm Limited
> >>   * Copyright(c) 2010-2019 Intel Corporation
> >>   * Copyright(c) 2023 Microsoft Corporation
> >>+ * Copyright(c) 2024 Ericsson AB
> >>   */
> >
> >Unless this is coming from another project code base, the common
> >practice is not to add copyright for each contributor in later versions.
> >
> 
> Unless it's a large contribution (compared to the rest of the file)?
> 
> I guess that's why the 916c50d commit adds the Microsoft copyright notice.
> 
> >>+/**
> >>+ * Test if a particular bit in a 32-bit word is set.
> >>+ *
> >>+ * This function does not give any guarantees in regards to memory
> >>+ * ordering or atomicity.
> >>+ *
> >>+ * @param addr
> >>+ *   A pointer to the 32-bit word to query.
> >>+ * @param nr
> >>+ *   The index of the bit (0-31).
> >>+ * @return
> >>+ *   Returns true if the bit is set, and false otherwise.
> >>+ */
> >>+static inline bool
> >>+rte_bit_test32(const uint32_t *addr, unsigned int nr);
> >
> >Is it possible to reorder these inlines to avoid having
> >forward declarations?
> >
> 
> Yes, but I'm not sure it's a net gain.
> 
> A statement expression macro seems like a perfect tool for the job,
> but then MSVC doesn't support statement expressions. You could also
> have a macro that just generate the function body, as oppose to the
> whole function.

statement expressions can be used even with MSVC when using C. but GCC
documentation discourages their use for C++. since the header is
consumed by C++ in addition to C it's preferrable to avoid them.

> 
> I'll consider if I should just bite the bullet and expand all the
> macros. 4x duplication.
> 
> >Also, new functions should be marked __rte_experimental
> >for a release or two.
> 
> Yes, thanks.
  
Mattias Rönnblom March 5, 2024, 6:01 p.m. UTC | #4
On 2024-03-04 17:34, Tyler Retzlaff wrote:
> On Sun, Mar 03, 2024 at 07:26:36AM +0100, Mattias Rönnblom wrote:
>> On 2024-03-02 18:05, Stephen Hemminger wrote:
>>> On Sat, 2 Mar 2024 14:53:22 +0100
>>> Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
>>>
>>>> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
>>>> index 449565eeae..9a368724d5 100644
>>>> --- a/lib/eal/include/rte_bitops.h
>>>> +++ b/lib/eal/include/rte_bitops.h
>>>> @@ -2,6 +2,7 @@
>>>>    * Copyright(c) 2020 Arm Limited
>>>>    * Copyright(c) 2010-2019 Intel Corporation
>>>>    * Copyright(c) 2023 Microsoft Corporation
>>>> + * Copyright(c) 2024 Ericsson AB
>>>>    */
>>>
>>> Unless this is coming from another project code base, the common
>>> practice is not to add copyright for each contributor in later versions.
>>>
>>
>> Unless it's a large contribution (compared to the rest of the file)?
>>
>> I guess that's why the 916c50d commit adds the Microsoft copyright notice.
>>
>>>> +/**
>>>> + * Test if a particular bit in a 32-bit word is set.
>>>> + *
>>>> + * This function does not give any guarantees in regards to memory
>>>> + * ordering or atomicity.
>>>> + *
>>>> + * @param addr
>>>> + *   A pointer to the 32-bit word to query.
>>>> + * @param nr
>>>> + *   The index of the bit (0-31).
>>>> + * @return
>>>> + *   Returns true if the bit is set, and false otherwise.
>>>> + */
>>>> +static inline bool
>>>> +rte_bit_test32(const uint32_t *addr, unsigned int nr);
>>>
>>> Is it possible to reorder these inlines to avoid having
>>> forward declarations?
>>>
>>
>> Yes, but I'm not sure it's a net gain.
>>
>> A statement expression macro seems like a perfect tool for the job,
>> but then MSVC doesn't support statement expressions. You could also
>> have a macro that just generate the function body, as oppose to the
>> whole function.
> 
> statement expressions can be used even with MSVC when using C. but GCC
> documentation discourages their use for C++. since the header is

GCC documentation discourages statement expressions *of a particular 
form* from being included in headers to be consumed by C++.

They would be fine to use here, especially considering they wouldn't be 
a part of the public API (i.e., only invoked from the static inline 
functions in the API).

> consumed by C++ in addition to C it's preferrable to avoid them.
> 
>>
>> I'll consider if I should just bite the bullet and expand all the
>> macros. 4x duplication.
>>
>>> Also, new functions should be marked __rte_experimental
>>> for a release or two.
>>
>> Yes, thanks.
  
Tyler Retzlaff March 5, 2024, 6:06 p.m. UTC | #5
On Tue, Mar 05, 2024 at 07:01:50PM +0100, Mattias Rönnblom wrote:
> On 2024-03-04 17:34, Tyler Retzlaff wrote:
> >On Sun, Mar 03, 2024 at 07:26:36AM +0100, Mattias Rönnblom wrote:
> >>On 2024-03-02 18:05, Stephen Hemminger wrote:
> >>>On Sat, 2 Mar 2024 14:53:22 +0100
> >>>Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> >>>
> >>>>diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> >>>>index 449565eeae..9a368724d5 100644
> >>>>--- a/lib/eal/include/rte_bitops.h
> >>>>+++ b/lib/eal/include/rte_bitops.h
> >>>>@@ -2,6 +2,7 @@
> >>>>   * Copyright(c) 2020 Arm Limited
> >>>>   * Copyright(c) 2010-2019 Intel Corporation
> >>>>   * Copyright(c) 2023 Microsoft Corporation
> >>>>+ * Copyright(c) 2024 Ericsson AB
> >>>>   */
> >>>
> >>>Unless this is coming from another project code base, the common
> >>>practice is not to add copyright for each contributor in later versions.
> >>>
> >>
> >>Unless it's a large contribution (compared to the rest of the file)?
> >>
> >>I guess that's why the 916c50d commit adds the Microsoft copyright notice.
> >>
> >>>>+/**
> >>>>+ * Test if a particular bit in a 32-bit word is set.
> >>>>+ *
> >>>>+ * This function does not give any guarantees in regards to memory
> >>>>+ * ordering or atomicity.
> >>>>+ *
> >>>>+ * @param addr
> >>>>+ *   A pointer to the 32-bit word to query.
> >>>>+ * @param nr
> >>>>+ *   The index of the bit (0-31).
> >>>>+ * @return
> >>>>+ *   Returns true if the bit is set, and false otherwise.
> >>>>+ */
> >>>>+static inline bool
> >>>>+rte_bit_test32(const uint32_t *addr, unsigned int nr);
> >>>
> >>>Is it possible to reorder these inlines to avoid having
> >>>forward declarations?
> >>>
> >>
> >>Yes, but I'm not sure it's a net gain.
> >>
> >>A statement expression macro seems like a perfect tool for the job,
> >>but then MSVC doesn't support statement expressions. You could also
> >>have a macro that just generate the function body, as oppose to the
> >>whole function.
> >
> >statement expressions can be used even with MSVC when using C. but GCC
> >documentation discourages their use for C++. since the header is
> 
> GCC documentation discourages statement expressions *of a particular
> form* from being included in headers to be consumed by C++.
> 
> They would be fine to use here, especially considering they wouldn't
> be a part of the public API (i.e., only invoked from the static
> inline functions in the API).

agreed, there should be no problem.

> 
> >consumed by C++ in addition to C it's preferrable to avoid them.
> >
> >>
> >>I'll consider if I should just bite the bullet and expand all the
> >>macros. 4x duplication.
> >>
> >>>Also, new functions should be marked __rte_experimental
> >>>for a release or two.
> >>
> >>Yes, thanks.
  

Patch

diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
index 449565eeae..9a368724d5 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -2,6 +2,7 @@ 
  * Copyright(c) 2020 Arm Limited
  * Copyright(c) 2010-2019 Intel Corporation
  * Copyright(c) 2023 Microsoft Corporation
+ * Copyright(c) 2024 Ericsson AB
  */
 
 #ifndef _RTE_BITOPS_H_
@@ -11,8 +12,9 @@ 
  * @file
  * Bit Operations
  *
- * This file defines a family of APIs for bit operations
- * without enforcing memory ordering.
+ * This file provides functionality for low-level, single-word
+ * arithmetic and bit-level operations, such as counting or
+ * setting individual bits.
  */
 
 #include <stdint.h>
@@ -105,6 +107,194 @@  extern "C" {
 #define RTE_FIELD_GET64(mask, reg) \
 		((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
 
+/**
+ * Test if a particular bit in a 32-bit word is set.
+ *
+ * This function does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the 32-bit word to query.
+ * @param nr
+ *   The index of the bit (0-31).
+ * @return
+ *   Returns true if the bit is set, and false otherwise.
+ */
+static inline bool
+rte_bit_test32(const uint32_t *addr, unsigned int nr);
+
+/**
+ * Set bit in 32-bit word.
+ *
+ * Set bit specified by @c nr in the 32-bit word pointed to by
+ * @c addr to '1'.
+ *
+ * This function does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the 32-bit word to modify.
+ * @param nr
+ *   The index of the bit (0-31).
+ */
+static inline void
+rte_bit_set32(uint32_t *addr, unsigned int nr);
+
+/**
+ * Clear bit in 32-bit word.
+ *
+ * Set bit specified by @c nr in the 32-bit word pointed to by
+ * @c addr to '0'.
+ *
+ * This function does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the 32-bit word to modify.
+ * @param nr
+ *   The index of the bit (0-31).
+ */
+static inline void
+rte_bit_clear32(uint32_t *addr, unsigned int nr);
+
+/**
+ * Assign a value to bit in a 32-bit word.
+ *
+ * Set bit specified by @c nr in the 32-bit word pointed to by
+ * @c addr to the value indicated by @c value.
+ *
+ * This function does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the 32-bit word to modify.
+ * @param nr
+ *   The index of the bit (0-31).
+ * @param value
+ *   The new value of the bit - true for '1', or false for '0'.
+ */
+static inline void
+rte_bit_assign32(uint32_t *addr, unsigned int nr, bool value)
+{
+	if (value)
+		rte_bit_set32(addr, nr);
+	else
+		rte_bit_clear32(addr, nr);
+}
+
+/**
+ * Test if a particular bit in a 64-bit word is set.
+ *
+ * This function does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the 64-bit word to query.
+ * @param nr
+ *   The index of the bit (0-63).
+ * @return
+ *   Returns true if the bit is set, and false otherwise.
+ */
+static inline bool
+rte_bit_test64(const uint64_t *addr, unsigned int nr);
+
+/**
+ * Set bit in 64-bit word.
+ *
+ * Set bit specified by @c nr in the 64-bit word pointed to by
+ * @c addr to '1'.
+ *
+ * This function does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the 64-bit word to modify.
+ * @param nr
+ *   The index of the bit (0-63).
+ */
+static inline void
+rte_bit_set64(uint64_t *addr, unsigned int nr);
+
+/**
+ * Clear bit in 64-bit word.
+ *
+ * Set bit specified by @c nr in the 64-bit word pointed to by
+ * @c addr to '0'.
+ *
+ * This function does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the 64-bit word to modify.
+ * @param nr
+ *   The index of the bit (0-63).
+ */
+static inline void
+rte_bit_clear64(uint64_t *addr, unsigned int nr);
+
+/**
+ * Assign a value to bit in a 64-bit word.
+ *
+ * Set bit specified by @c nr in the 64-bit word pointed to by
+ * @c addr to the value indicated by @c value.
+ *
+ * This function does not give any guarantees in regards to memory
+ * ordering or atomicity.
+ *
+ * @param addr
+ *   A pointer to the 64-bit word to modify.
+ * @param nr
+ *   The index of the bit (0-63).
+ * @param value
+ *   The new value of the bit - true for '1', or false for '0'.
+ */
+static inline void
+rte_bit_assign64(uint64_t *addr, unsigned int nr, bool value)
+{
+	if (value)
+		rte_bit_set64(addr, nr);
+	else
+		rte_bit_clear64(addr, nr);
+}
+
+#define __RTE_GEN_BIT_TEST(name, size, qualifier)			\
+	static inline bool						\
+	name(const qualifier uint ## size ## _t *addr, unsigned int nr)	\
+	{								\
+		RTE_ASSERT(nr < size);					\
+									\
+		uint ## size ## _t mask = (uint ## size ## _t)1 << nr;	\
+		return *addr & mask;					\
+	}
+
+#define __RTE_GEN_BIT_SET(name, size, qualifier)			\
+	static inline void						\
+	name(qualifier uint ## size ## _t *addr, unsigned int nr)	\
+	{								\
+		RTE_ASSERT(nr < size);					\
+									\
+		uint ## size ## _t mask = (uint ## size ## _t)1 << nr;	\
+		*addr |= mask;						\
+	}								\
+
+#define __RTE_GEN_BIT_CLEAR(name, size, qualifier)			\
+	static inline void						\
+	name(qualifier uint ## size ## _t *addr, unsigned int nr)	\
+	{								\
+		RTE_ASSERT(nr < size);					\
+									\
+		uint ## size ## _t mask = ~((uint ## size ## _t)1 << nr); \
+		(*addr) &= mask;					\
+	}								\
+
+__RTE_GEN_BIT_TEST(rte_bit_test32, 32,)
+__RTE_GEN_BIT_SET(rte_bit_set32, 32,)
+__RTE_GEN_BIT_CLEAR(rte_bit_clear32, 32,)
+
+__RTE_GEN_BIT_TEST(rte_bit_test64, 64,)
+__RTE_GEN_BIT_SET(rte_bit_set64, 64,)
+__RTE_GEN_BIT_CLEAR(rte_bit_clear64, 64,)
+
 /*------------------------ 32-bit relaxed operations ------------------------*/
 
 /**