From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 584C941C55; Thu, 9 Feb 2023 20:19:20 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3F43A410F9; Thu, 9 Feb 2023 20:19:20 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id B8FEB4067B for ; Thu, 9 Feb 2023 20:19:18 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v2] eal: introduce atomics abstraction Date: Thu, 9 Feb 2023 20:19:14 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87727@smartserver.smartshare.dk> In-Reply-To: <20230209181510.GD21854@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v2] eal: introduce atomics abstraction Thread-Index: Adk8snadzFo0YANoTt6px4GdK66zfgAA3h4w References: <1673558785-24992-1-git-send-email-roretzla@linux.microsoft.com> <1675892618-31755-1-git-send-email-roretzla@linux.microsoft.com> <1675892618-31755-2-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D87718@smartserver.smartshare.dk> <20230209181510.GD21854@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Tyler Retzlaff" Cc: , , , , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Thursday, 9 February 2023 19.15 >=20 > On Thu, Feb 09, 2023 at 09:05:46AM +0100, Morten Br=F8rup 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=3Dtrue > option. > > > > > > Signed-off-by: Tyler Retzlaff > > > --- > > > > Looks good. A few minor suggestions about implementation only. > > > > With or without suggested modifications, > > > > Acked-by: Morten Br=F8rup [...] > > > 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 > > > +#endif > > > + > > > +#define __rte_atomic _Atomic > > > + > > > +typedef int rte_memory_order; > > > > I would prefer enum for rte_memory_order: > > > > typedef enum { > > rte_memory_order_relaxed =3D memory_order_relaxed, > > rte_memory_order_consume =3D memory_order_consume, > > rte_memory_order_acquire =3D memory_order_acquire, > > rte_memory_order_release =3D memory_order_release, > > rte_memory_order_acq_rel =3D memory_order_acq_rel, > > rte_memory_order_seq_cst =3D memory_order_seq_cst > > } rte_memory_order; >=20 > the reason for not using enum type is abi related. the c standard has > this little gem. >=20 > ISO/IEC 9899:2011 >=20 > 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. >=20 > 128) An implementation may delay the choice of which integer type > until > all enumeration constants have been seen. >=20 > so i'm just being overly protective of maintaining the forward > compatibility of the abi. >=20 > 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. >=20 > incidentally this is also why you can't forward declare enums in c. >=20 [...] > > > +#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 for this... I haven't checked. >=20 > strictly speaking you are correct the intrinsic does take bool > according > to documentation. >=20 > ISO/IEC 9899:2011 >=20 > 7.18 Boolean type and values > (1) The header 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. >=20 > 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.