[v2,1/5] eal: extend bit manipulation functionality
Checks
Commit Message
Add functionality to test and modify the value of 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>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
--
RFC v6:
* Have rte_bit_test() accept const-marked bitsets.
RFC v4:
* Add rte_bit_flip() which, believe it or not, flips the value of a bit.
* Mark macro-generated private functions as experimental.
* Use macros to generate *assign*() functions.
RFC v3:
* Work around lack of C++ support for _Generic (Tyler Retzlaff).
* Fix ','-related checkpatch warnings.
---
lib/eal/include/rte_bitops.h | 259 ++++++++++++++++++++++++++++++++++-
1 file changed, 257 insertions(+), 2 deletions(-)
Comments
On 09/08/2024 10:58, Mattias Rönnblom wrote:
<snip>
> +
> +__RTE_GEN_BIT_TEST(, test,, 32)
> +__RTE_GEN_BIT_SET(, set,, 32)
> +__RTE_GEN_BIT_CLEAR(, clear,, 32)
> +__RTE_GEN_BIT_ASSIGN(, assign,, 32)
> +__RTE_GEN_BIT_FLIP(, flip,, 32)
> +
> +__RTE_GEN_BIT_TEST(, test,, 64)
> +__RTE_GEN_BIT_SET(, set,, 64)
> +__RTE_GEN_BIT_CLEAR(, clear,, 64)
> +__RTE_GEN_BIT_ASSIGN(, assign,, 64)
> +__RTE_GEN_BIT_FLIP(, flip,, 64)
What is the purpose of the `fun` argument? As opposed to just having
these written out in the macro definitions. I notice the atomic
equivalents don't have this.
> /*------------------------ 32-bit relaxed operations ------------------------*/
>
> /**
> <snip>
On 2024-08-12 13:16, Jack Bond-Preston wrote:
> On 09/08/2024 10:58, Mattias Rönnblom wrote:
> <snip>
>> +
>> +__RTE_GEN_BIT_TEST(, test,, 32)
>> +__RTE_GEN_BIT_SET(, set,, 32)
>> +__RTE_GEN_BIT_CLEAR(, clear,, 32)
>> +__RTE_GEN_BIT_ASSIGN(, assign,, 32)
>> +__RTE_GEN_BIT_FLIP(, flip,, 32)
>> +
>> +__RTE_GEN_BIT_TEST(, test,, 64)
>> +__RTE_GEN_BIT_SET(, set,, 64)
>> +__RTE_GEN_BIT_CLEAR(, clear,, 64)
>> +__RTE_GEN_BIT_ASSIGN(, assign,, 64)
>> +__RTE_GEN_BIT_FLIP(, flip,, 64)
>
> What is the purpose of the `fun` argument? As opposed to just having
> these written out in the macro definitions. I notice the atomic
> equivalents don't have this.
>
It has no purpose, any more. I failed to clean that up, after removing
the "once" family of functions.
Thanks.
>> /*------------------------ 32-bit relaxed operations
>> ------------------------*/
>> /**
>> <snip>
On 2024-08-12 14:49, Mattias Rönnblom wrote:
> This patch set represent an attempt to improve and extend the RTE
> bitops API, in particular for functions that operate on individual
> bits.
>
Is there anyone else that has any opinion on this patch set? Details, or
big picture.
<snip>
Hello,
On Tue, Aug 20, 2024 at 7:05 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2024-08-12 14:49, Mattias Rönnblom wrote:
> > This patch set represent an attempt to improve and extend the RTE
> > bitops API, in particular for functions that operate on individual
> > bits.
> >
>
> Is there anyone else that has any opinion on this patch set? Details, or
> big picture.
Tyler, are you ok with this series?
Mattias, there are issues reported by the CI (compilation on Ubuntu
22.04 in GHA, and unit test failure in UNH), please have a look.
On 2024-09-05 10:10, David Marchand wrote:
> Hello,
>
> On Tue, Aug 20, 2024 at 7:05 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>>
>> On 2024-08-12 14:49, Mattias Rönnblom wrote:
>>> This patch set represent an attempt to improve and extend the RTE
>>> bitops API, in particular for functions that operate on individual
>>> bits.
>>>
>>
>> Is there anyone else that has any opinion on this patch set? Details, or
>> big picture.
>
> Tyler, are you ok with this series?
>
> Mattias, there are issues reported by the CI (compilation on Ubuntu
> 22.04 in GHA, and unit test failure in UNH), please have a look.
>
>
Standard practice in DPDK header files is the following:
--
/* rte_bar.h */
#ifdef __cplusplus
extern "C" {
#endif
#include <rte_foo.h>
void
rte_foo_do(void);
/../
--
That seems not like best practice to me, since rte_bar.h is messing
around with linkage of constructs of any files included. In particular,
it prohibits replacing _Generic with C++ function overloading, in C++ TUs.
What one should do is to have extern "C" linkage only on functions which
the include file in question (e.g., rte_foo.h) itself declares.
--
/* rte_bar.h */
#include <rte_foo.h>
#ifdef __cplusplus
extern "C" {
#endif
void
rte_foo_do(void);
/../
--
There are 259 header files in the DPDK repo in need of fixing.
Should the fix be 259 patches, or something smaller? One large patch, or
a patch per library, or something else. Please advise, over.
09/09/2024 14:04, Mattias Rönnblom:
> What one should do is to have extern "C" linkage only on functions which
> the include file in question (e.g., rte_foo.h) itself declares.
>
> --
> /* rte_bar.h */
> #include <rte_foo.h>
>
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> void
> rte_foo_do(void);
>
> /../
> --
>
> There are 259 header files in the DPDK repo in need of fixing.
>
> Should the fix be 259 patches, or something smaller? One large patch, or
> a patch per library, or something else. Please advise, over.
Moving includes in the whole tree can be done in a single patch,
there is nothing specific per library in such a mechanical move.
On Mon, Sep 9, 2024 at 2:05 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> > Mattias, there are issues reported by the CI (compilation on Ubuntu
> > 22.04 in GHA, and unit test failure in UNH), please have a look.
> >
> >
>
> Standard practice in DPDK header files is the following:
>
> --
> /* rte_bar.h */
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> #include <rte_foo.h>
>
> void
> rte_foo_do(void);
>
> /../
> --
>
> That seems not like best practice to me, since rte_bar.h is messing
> around with linkage of constructs of any files included. In particular,
> it prohibits replacing _Generic with C++ function overloading, in C++ TUs.
>
> What one should do is to have extern "C" linkage only on functions which
> the include file in question (e.g., rte_foo.h) itself declares.
This is probably not the best practice, but since you intend to fix
it, it will be perfect afterwards :-).
>
> --
> /* rte_bar.h */
> #include <rte_foo.h>
>
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> void
> rte_foo_do(void);
>
> /../
> --
>
> There are 259 header files in the DPDK repo in need of fixing.
>
> Should the fix be 259 patches, or something smaller? One large patch, or
> a patch per library, or something else. Please advise, over.
The change seems mechanical, so one single change is ok.
On 2024-09-09 14:25, David Marchand wrote:
> On Mon, Sep 9, 2024 at 2:05 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>>> Mattias, there are issues reported by the CI (compilation on Ubuntu
>>> 22.04 in GHA, and unit test failure in UNH), please have a look.
>>>
>>>
>>
>> Standard practice in DPDK header files is the following:
>>
>> --
>> /* rte_bar.h */
>> #ifdef __cplusplus
>> extern "C" {
>> #endif
>>
>> #include <rte_foo.h>
>>
>> void
>> rte_foo_do(void);
>>
>> /../
>> --
>>
>> That seems not like best practice to me, since rte_bar.h is messing
>> around with linkage of constructs of any files included. In particular,
>> it prohibits replacing _Generic with C++ function overloading, in C++ TUs.
>>
>> What one should do is to have extern "C" linkage only on functions which
>> the include file in question (e.g., rte_foo.h) itself declares.
>
> This is probably not the best practice, but since you intend to fix
> it, it will be perfect afterwards :-).
>
Actually, I intended to opt for a less-than-perfect solution, where you
just move the 'extern "C"' to cover everything but includes, rather than
just what is necessary (i.e., functions and global variables).
That change was easily automated, but the perfect solution requires a
more elaborate script or human intervention.
>
>>
>> --
>> /* rte_bar.h */
>> #include <rte_foo.h>
>>
>> #ifdef __cplusplus
>> extern "C" {
>> #endif
>>
>> void
>> rte_foo_do(void);
>>
>> /../
>> --
>>
>> There are 259 header files in the DPDK repo in need of fixing.
>>
>> Should the fix be 259 patches, or something smaller? One large patch, or
>> a patch per library, or something else. Please advise, over.
>
> The change seems mechanical, so one single change is ok.
>
>
@@ -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,12 +12,14 @@
* @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>
+#include <rte_compat.h>
#include <rte_debug.h>
#ifdef __cplusplus
@@ -105,6 +108,196 @@ extern "C" {
#define RTE_FIELD_GET64(mask, reg) \
((typeof(mask))(((reg) & (mask)) >> rte_ctz64(mask)))
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * 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, \
+ const uint32_t *: __rte_bit_test32, \
+ uint64_t *: __rte_bit_test64, \
+ const uint64_t *: __rte_bit_test64)(addr, nr)
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * 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)
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * 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)
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * 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)
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * Flip a bit in word.
+ *
+ * Generic selection macro to change the value of a bit to '0' if '1'
+ * or '1' if '0' 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_flip(addr, nr) \
+ _Generic((addr), \
+ uint32_t *: __rte_bit_flip32, \
+ uint64_t *: __rte_bit_flip64)(addr, nr)
+
+#define __RTE_GEN_BIT_TEST(family, fun, qualifier, size) \
+ __rte_experimental \
+ static inline bool \
+ __rte_bit_ ## family ## fun ## size(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(family, fun, qualifier, size) \
+ __rte_experimental \
+ static inline void \
+ __rte_bit_ ## family ## fun ## size(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(family, fun, qualifier, size) \
+ __rte_experimental \
+ static inline void \
+ __rte_bit_ ## family ## fun ## size(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_ASSIGN(family, fun, qualifier, size) \
+ __rte_experimental \
+ static inline void \
+ __rte_bit_ ## family ## fun ## size(qualifier uint ## size ## _t *addr, \
+ unsigned int nr, bool value) \
+ { \
+ if (value) \
+ __rte_bit_ ## family ## set ## size(addr, nr); \
+ else \
+ __rte_bit_ ## family ## clear ## size(addr, nr); \
+ }
+
+#define __RTE_GEN_BIT_FLIP(family, fun, qualifier, size) \
+ __rte_experimental \
+ static inline void \
+ __rte_bit_ ## family ## fun ## size(qualifier uint ## size ## _t *addr, \
+ unsigned int nr) \
+ { \
+ bool value; \
+ \
+ value = __rte_bit_ ## family ## test ## size(addr, nr); \
+ __rte_bit_ ## family ## assign ## size(addr, nr, !value); \
+ }
+
+__RTE_GEN_BIT_TEST(, test,, 32)
+__RTE_GEN_BIT_SET(, set,, 32)
+__RTE_GEN_BIT_CLEAR(, clear,, 32)
+__RTE_GEN_BIT_ASSIGN(, assign,, 32)
+__RTE_GEN_BIT_FLIP(, flip,, 32)
+
+__RTE_GEN_BIT_TEST(, test,, 64)
+__RTE_GEN_BIT_SET(, set,, 64)
+__RTE_GEN_BIT_CLEAR(, clear,, 64)
+__RTE_GEN_BIT_ASSIGN(, assign,, 64)
+__RTE_GEN_BIT_FLIP(, flip,, 64)
+
/*------------------------ 32-bit relaxed operations ------------------------*/
/**
@@ -787,6 +980,68 @@ rte_log2_u64(uint64_t v)
#ifdef __cplusplus
}
+
+/*
+ * Since C++ doesn't support generic selection (i.e., _Generic),
+ * function overloading is used instead. Such functions must be
+ * defined outside 'extern "C"' to be accepted by the compiler.
+ */
+
+#undef rte_bit_test
+#undef rte_bit_set
+#undef rte_bit_clear
+#undef rte_bit_assign
+#undef rte_bit_flip
+
+#define __RTE_BIT_OVERLOAD_SZ_2(fun, qualifier, size, arg1_type, arg1_name) \
+ static inline void \
+ rte_bit_ ## fun(qualifier uint ## size ## _t *addr, \
+ arg1_type arg1_name) \
+ { \
+ __rte_bit_ ## fun ## size(addr, arg1_name); \
+ }
+
+#define __RTE_BIT_OVERLOAD_2(fun, qualifier, arg1_type, arg1_name) \
+ __RTE_BIT_OVERLOAD_SZ_2(fun, qualifier, 32, arg1_type, arg1_name) \
+ __RTE_BIT_OVERLOAD_SZ_2(fun, qualifier, 64, arg1_type, arg1_name)
+
+#define __RTE_BIT_OVERLOAD_SZ_2R(fun, qualifier, size, ret_type, arg1_type, \
+ arg1_name) \
+ static inline ret_type \
+ rte_bit_ ## fun(qualifier uint ## size ## _t *addr, \
+ arg1_type arg1_name) \
+ { \
+ return __rte_bit_ ## fun ## size(addr, arg1_name); \
+ }
+
+#define __RTE_BIT_OVERLOAD_2R(fun, qualifier, ret_type, arg1_type, arg1_name) \
+ __RTE_BIT_OVERLOAD_SZ_2R(fun, qualifier, 32, ret_type, arg1_type, \
+ arg1_name) \
+ __RTE_BIT_OVERLOAD_SZ_2R(fun, qualifier, 64, ret_type, arg1_type, \
+ arg1_name)
+
+#define __RTE_BIT_OVERLOAD_SZ_3(fun, qualifier, size, arg1_type, arg1_name, \
+ arg2_type, arg2_name) \
+ static inline void \
+ rte_bit_ ## fun(uint ## size ## _t *addr, arg1_type arg1_name, \
+ arg2_type arg2_name) \
+ { \
+ __rte_bit_ ## fun ## size(addr, arg1_name, arg2_name); \
+ }
+
+#define __RTE_BIT_OVERLOAD_3(fun, qualifier, arg1_type, arg1_name, arg2_type, \
+ arg2_name) \
+ __RTE_BIT_OVERLOAD_SZ_3(fun, qualifier, 32, arg1_type, arg1_name, \
+ arg2_type, arg2_name) \
+ __RTE_BIT_OVERLOAD_SZ_3(fun, qualifier, 64, arg1_type, arg1_name, \
+ arg2_type, arg2_name)
+
+__RTE_BIT_OVERLOAD_2R(test, const, bool, unsigned int, nr)
+__RTE_BIT_OVERLOAD_2(set,, unsigned int, nr)
+__RTE_BIT_OVERLOAD_2(clear,, unsigned int, nr)
+__RTE_BIT_OVERLOAD_3(assign,, unsigned int, nr, bool, value)
+__RTE_BIT_OVERLOAD_2(flip,, unsigned int, nr)
+
#endif
#endif /* _RTE_BITOPS_H_ */