RE: [PATCH v2] eal: introduce atomics abstraction

["Morten Brørup" <mb@smartsharesystems.com>]

> 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.

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 why you can't forward declare enums in c.
> 

[...]

> > > +#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.