On Thu, Feb 09, 2023 at 08:19:14PM +0100, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > Sent: Thursday, 9 February 2023 19.15 > > > > On Thu, Feb 09, 2023 at 09:05:46AM +0100, Morten Brørup wrote: > > > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > > > Sent: Wednesday, 8 February 2023 22.44 > > > > > > > > Introduce atomics abstraction that permits optional use of standard > > C11 > > > > atomics when meson is provided the new enable_stdatomics=true > > option. > > > > > > > > Signed-off-by: Tyler Retzlaff <roret...@linux.microsoft.com> > > > > --- > > > > > > Looks good. A few minor suggestions about implementation only. > > > > > > With or without suggested modifications, > > > > > > Acked-by: Morten Brørup <m...@smartsharesystems.com> > > [...] > > > > > diff --git a/lib/eal/include/generic/rte_atomic.h > > > > b/lib/eal/include/generic/rte_atomic.h > > > > index f5c49a9..392d928 100644 > > > > --- a/lib/eal/include/generic/rte_atomic.h > > > > +++ b/lib/eal/include/generic/rte_atomic.h > > > > @@ -110,6 +110,100 @@ > > > > > > > > #endif /* __DOXYGEN__ */ > > > > > > > > +#ifdef RTE_STDC_ATOMICS > > > > + > > > > +#if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L || > > > > defined(__STDC_NO_ATOMICS__) > > > > +#error compiler does not support C11 standard atomics > > > > +#else > > > > +#include <stdatomic.h> > > > > +#endif > > > > + > > > > +#define __rte_atomic _Atomic > > > > + > > > > +typedef int rte_memory_order; > > > > > > I would prefer enum for rte_memory_order: > > > > > > typedef enum { > > > rte_memory_order_relaxed = memory_order_relaxed, > > > rte_memory_order_consume = memory_order_consume, > > > rte_memory_order_acquire = memory_order_acquire, > > > rte_memory_order_release = memory_order_release, > > > rte_memory_order_acq_rel = memory_order_acq_rel, > > > rte_memory_order_seq_cst = memory_order_seq_cst > > > } rte_memory_order; > > > > the reason for not using enum type is abi related. the c standard has > > this little gem. > > > > ISO/IEC 9899:2011 > > > > 6.7.2.2 (4) > > Each enumerated type shall be compatible with char, a signed > > integer > > type, or an unsigned integer type. The choice of type is > > implementation-defined, 128) but shall be capable of representing > > the > > values of all the members of the enumeration. > > > > 128) An implementation may delay the choice of which integer type > > until > > all enumeration constants have been seen. > > > > so i'm just being overly protective of maintaining the forward > > compatibility of the abi. > > > > probably i'm being super unnecessarily cautious in this case since i > > think > > in practice even if an implementation chose sizeof(char) i doubt very > > much > > that enough enumerated values would get added to this enumeration > > within > > the lifetime of the API to suddenly cause the compiler to choose > > > sizeof(char). > > I am under the impression that compilers usually instantiate enum as int, and > you can make it use a smaller size by decorating it with the "packed" > attribute - I have benefited from that in the past.
generally i think most implementations choose int as a default but i think there is potential for char to get used if the compiler is asked to optimize for size. not a likely optimization preference when using dpdk. > > The only risk I am effectively trying to avoid is someone calling an > rte_atomic() function with "order" being another value than one of these > values. Probably not ever going to happen. > > Your solution also addresses an academic risk (of the compiler using another > type than int for the enum), which will have unwanted side effects - > especially if the "order" parameter to the rte_atomic() functions becomes > char instead of int. > > I can now conclude that your proposed type (int) is stronger/safer than the > type (enum) I suggested. So please keep what you have. > > > > > incidentally this is also > > > > [...] > > > > > +#define rte_atomic_compare_exchange_strong_explicit(obj, expected, > > > > desired, success, fail) \ > > > > + __atomic_compare_exchange_n(obj, expected, desired, 0, success, > > > > fail) > > > > > > The type of the "weak" parameter to __atomic_compare_exchange_n() is > > bool, not int, so use "false" instead of 0. There is probably no > > practical difference, so I'll leave it up to you. > > > > > > You might need to include <stdbool.h> for this... I haven't checked. > > > > strictly speaking you are correct the intrinsic does take bool > > according > > to documentation. > > > > ISO/IEC 9899:2011 > > > > 7.18 Boolean type and values <stdbool.h> > > (1) The header <stdbool.h> defines four macros. > > (2) The macro bool expands to _Bool. > > (3) The remaining three macros are suitable for use in #if > > preprocessing > > directives. They are `true' which expands to the integer constant > > 1, > > `false' which expands to the integer constant 0, and > > __bool_true_false_are_defined which expands to the integer > > constant 1. > > Thank you for this reference. I wasn't aware that the two boolean values > explicitly expanded to those two integer constants. I had never thought about > it, but simply assumed that the constant "true" had the same meaning as "not > false", like "if (123)" evaluates "123" as true. > > So I learned something new today. > > > > > so i could include the header, to expand a macro which expands to > > integer constant 0 or 1 as appropriate for weak vs strong. do you think > > i should? (serious question) if you answer yes, i'll make the change. > > Then no. Please keep the 1's and 0's. thanks! will leave it as is.
