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 2D8F741C56; Thu, 9 Feb 2023 23:04:26 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BA27A40E50; Thu, 9 Feb 2023 23:04:25 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 38B1B4067B for ; Thu, 9 Feb 2023 23:04:24 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 4E2CD20C8AF4; Thu, 9 Feb 2023 14:04:23 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 4E2CD20C8AF4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1675980263; bh=JLORYFPW/rijM2ePwMzndbxKmN6eJiClBjLAV6g09ck=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=j6fhy1+eBJ76grUvfD6K40e2L3xZQbS2aUJBSZv2LH2a0TXMiFxz8IGvQocto5Cvc vpNnumzg5jJb8Q3pwMOO2v1sc8poVbxfrK9R76/S7Zomxw0Qpc/AypTc49wfdHsL2X 1TlUibK64ZVv+Q8r6YsPm53twjab9czIRaszxbFQ= Date: Thu, 9 Feb 2023 14:04:23 -0800 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: dev@dpdk.org, david.marchand@redhat.com, thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, bruce.richardson@intel.com Subject: Re: [PATCH v2] eal: introduce atomics abstraction Message-ID: <20230209220423.GA5398@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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> <98CBD80474FA8B44BF855DF32C47DC35D87727@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87727@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 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 > > > > --- > > > > > > Looks good. A few minor suggestions about implementation only. > > > > > > With or without suggested modifications, > > > > > > Acked-by: Morten Brørup > > [...] > > > > > 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 = 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 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 > > (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. > > > > > 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.