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.ronnb...@ericsson.com>
> >Acked-by: Morten Brørup <m...@smartsharesystems.com>
> >Acked-by: Tyler Retzlaff <roret...@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."
"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>
