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 76FD043086; Wed, 16 Aug 2023 22:13:26 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EED8040DDA; Wed, 16 Aug 2023 22:13:25 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id AB7924003C; Wed, 16 Aug 2023 22:13:24 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 689A9205E5; Wed, 16 Aug 2023 22:13:24 +0200 (CEST) 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 2/6] eal: adapt EAL to present rte optional atomics API Date: Wed, 16 Aug 2023 22:13:22 +0200 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87B0F@smartserver.smartshare.dk> In-Reply-To: <20230814174706.GB12422@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v2 2/6] eal: adapt EAL to present rte optional atomics API Thread-Index: AdnO11i0nAqSLVI5R8KxP3tYScs93ABnjisQ References: <1691717521-1025-1-git-send-email-roretzla@linux.microsoft.com> <1691775136-6460-1-git-send-email-roretzla@linux.microsoft.com> <1691775136-6460-3-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D87AEF@smartserver.smartshare.dk> <20230814174706.GB12422@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Tyler Retzlaff" , , Cc: , , "Honnappa Nagarahalli" , "Ruifeng Wang" , "Jerin Jacob" , "Sunil Kumar Kori" , =?iso-8859-1?Q?Mattias_R=F6nnblom?= , "Joyce Kong" , "David Christensen" , "Konstantin Ananyev" , "David Hunt" , "David Marchand" 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: Monday, 14 August 2023 19.47 >=20 > On Mon, Aug 14, 2023 at 10:00:49AM +0200, Morten Br=F8rup wrote: > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > Sent: Friday, 11 August 2023 19.32 > > > > > > Adapt the EAL public headers to use rte optional atomics API = instead > of > > > directly using and exposing toolchain specific atomic builtin > intrinsics. > > > > > > Signed-off-by: Tyler Retzlaff > > > --- > > > > [...] > > >=20 > will fix the comments identified. >=20 > > > > [...] > > > > > --- a/lib/eal/include/generic/rte_spinlock.h > > > +++ b/lib/eal/include/generic/rte_spinlock.h > > > @@ -29,7 +29,7 @@ > > > * The rte_spinlock_t type. > > > */ > > > typedef struct __rte_lockable { > > > - volatile int locked; /**< lock status 0 =3D unlocked, 1 =3D = locked */ > > > + volatile int __rte_atomic locked; /**< lock status 0 =3D = unlocked, 1 > =3D > > > locked */ > > > > I think __rte_atomic should be before the type: > > volatile __rte_atomic int locked; /**< lock status [...] > > Alternatively (just mentioning it, I know we don't use this form): > > volatile __rte_atomic(int) locked; /**< lock status [...] > > > > Thinking of where you would put "const" might help. Regarding "const", I use the mental trick of reading from right-to-left = when pointers are involved, e.g.: const int * * const x; ----5---- 4 3 --2-- 1 x(1) is a const(2) pointer(3) to a pointer(4) to a const int(5). And yes, treating "const int" as one word is cheating... formally it = should be "int" "const", i.e. the reverse order; but that is not the = convention, so I have learned to accept it. > > > > Maybe your order is also correct, so it is a matter of preference. >=20 > so for me what you suggest is the canonical convention for c and i did > initially try to make the change with this convention but ran into > trouble when using the keyword in a context used as a type specifier > and the type was incomplete. >=20 > the rte_mcslock is a good example for illustration. >=20 > // original struct > typedef struct rte_mcslock { > struct rte_mcslock *next; > ... > }; >=20 > it simply doesn't work / won't compile (at least with clang) which = is > what drove me to use the less-often used syntax. >=20 > typedef struct rte_mcslock { > _Atomic struct rte_mcslock *next; > ... > }; >=20 > In file included from ../app/test/test_mcslock.c:19: > ..\lib\eal\include\rte_mcslock.h:36:2: error: _Atomic cannot be > applied > to incomplete type 'struct rte_mcslock' > _Atomic struct rte_mcslock *next; > ^ > ..\lib\eal\include\rte_mcslock.h:35:16: note: definition of 'struct > rte_mcslock' is not complete until the closing '}' > typedef struct rte_mcslock { > ^ > 1 error generated. >=20 > so i ended up choosing to use a single syntax by convention = consistently > rather than using one for the exceptional case and one everywhere = else. >=20 > i think (based on our other thread of discussion) i would recommend we > use adopt and require the use of the _Atomic(T) macro to disambiguate = it > also has the advantage of not being churned later when we can do = c++23. >=20 > // using macro > typedef struct rte_mcslock { > _Atomic(struct rte_mcslock *) next; This makes it an atomic pointer. Your example above tried making the = struct rts_mcslock atomic. Probably what you wanted was: typedef struct rte_mcslock { struct rte_mcslock * _Atomic next; ... }; Like "const", the convention should be putting it before any type, but = after the "*" for pointers. I suppose clang doesn't accept applying _Atomic to incomplete types, = regardless where you put it... I.e. this should also fail, I guess: typedef struct rte_mcslock { struct rte_mcslock _Atomic * next; ... }; > ... > }; >=20 > this is much easier at a glance to know when the specified type is the = T > or the T * similarly in parameter lists it becomes more clear too. >=20 > e.g. > void foo(int *v) >=20 > that it is either void foo(_Atomic(int) *v) or void foo(_Atomic(int *) > v) becomes > much clearer without having to do mental gymnastics. The same could be said about making "const" clearer: void foo(const(int) * v) instead of void foo(const int * v), and void foo(const(int *) v) instead of void foo(int * const v). Luckily, we don't need toolchain specific handling of "const", so let's = just leave that the way it is. :-) >=20 > so i propose we retain >=20 > #define __rte_atomic _Atomic >=20 > allow it to be used in contexts where we need a type-qualifier. > note: > most of the cases where _Atomic is used as a type-qualifier it > is a red flag that we are sensitive to an implementation detail > of the compiler. in time i hope most of these will go away as we > remove deprecated rte_atomic_xx apis. >=20 > but also introduce the following macro >=20 > #define RTE_ATOMIC(type) _Atomic(type) > require it be used in the contexts that we are using it as a type- > specifier. >=20 > if folks agree with this please reply back positively and i'll update > the series. feel free to propose alternate names or whatever, but = sooner > than later so i don't have to churn things too much :) +1 to Tyler's updated proposal, with macro names as suggested. If anyone disagrees, please speak up soon! If in doubt, please read https://en.cppreference.com/w/c/language/atomic = carefully. It says: (1) _Atomic(type-name) (since C11): Use as a type specifier; this = designates a new atomic type. (2) _Atomic type-name (since C11): Use as a type qualifier; this = designates the atomic version of type-name. In this role, it may be = mixed with const, volatile, and restrict, although unlike other = qualifiers, the atomic version of type-name may have a different size, = alignment, and object representation. NB: I hadn't noticed this before, otherwise I had probably suggested = using _Atomic(T) earlier on. We learn something new every day. :-) >=20 > thanks! Sorry about the late response, Tyler. Other work prevented me from = setting aside coherent time to review your updated proposal. >=20 > > > > The DPDK coding style guidelines doesn't mention where to place > "const", but looking at the code, it seems to use "const unsigned int" > and "const char *". >=20 > we probably should document it as a convention and most likely we = should > adopt what is already in use more commonly. +1, but not as part of this series. :-)