On Tue, Mar 05, 2024 at 09:02:34PM +0100, Mattias Rönnblom wrote:
> 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.ronnb...@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.
I agree, I don't think it should be prohibited. If C++ poses a problem
we can work to find solutions.
>
> >>
> >>Thanks.
> >>
> >>>Acked-by: Tyler Retzlaff <roret...@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
