Re: [PATCH v2] eal: introduce atomics abstraction

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Thu, Feb 09, 2023 at 08:19:14PM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@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:roretzla@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 <roretzla@linux.microsoft.com>
> > > > ---
> > >
> > > Looks good. A few minor suggestions about implementation only.
> > >
> > > With or without suggested modifications,
> > >
> > > Acked-by: Morten Brørup <mb@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.