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 976C843086; Wed, 16 Aug 2023 22:32:34 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4B9EF410E3; Wed, 16 Aug 2023 22:32:34 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id E20D34003C; Wed, 16 Aug 2023 22:32:31 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 3521A211F615; Wed, 16 Aug 2023 13:32:30 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3521A211F615 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1692217950; bh=uHwfqb0V1UptWSCmoWTgzlEPqaU4TMzQHzvWz8MvOyo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=nzsb6e0u7nnpw+ilUYaYI6Bt7Xkejv9X+RLveu/LBFgPkZQqunaBrEMc6DhdxLuvY Ni7oaV1zyFl6+yd04a6Bbsi9E9fFrTHPb6q2Eajd2DrQIcX2C5nVCrAJ/iYORpRDwP vCbEJX5KxJSr4pCz3ygIjq43lnLYkZOwwGC8Dya8= Date: Wed, 16 Aug 2023 13:32:30 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: thomas@monjalon.net, bruce.richardson@intel.com, dev@dpdk.org, techboard@dpdk.org, Honnappa Nagarahalli , Ruifeng Wang , Jerin Jacob , Sunil Kumar Kori , Mattias =?iso-8859-1?Q?R=F6nnblom?= , Joyce Kong , David Christensen , Konstantin Ananyev , David Hunt , David Marchand Subject: Re: [PATCH v2 2/6] eal: adapt EAL to present rte optional atomics API Message-ID: <20230816203230.GA19981@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> 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> <98CBD80474FA8B44BF855DF32C47DC35D87B0F@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: <98CBD80474FA8B44BF855DF32C47DC35D87B0F@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 Wed, Aug 16, 2023 at 10:13:22PM +0200, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Monday, 14 August 2023 19.47 > > > > On Mon, Aug 14, 2023 at 10:00:49AM +0200, Morten Brørup 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 > > > > --- > > > > > > [...] > > > > > > > will fix the comments identified. > > > > > > > > [...] > > > > > > > --- 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 = unlocked, 1 = locked */ > > > > + volatile int __rte_atomic locked; /**< lock status 0 = unlocked, 1 > > = > > > > 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 yes, i'm very familiar with where it can appear in the syntax and applied. but it's always good to have someone summarize it like this for the discussion. > > 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. it more often is the convention in c++, but i agree in c conventionally people put the const first. > > > > > > > Maybe your order is also correct, so it is a matter of preference. > > > > 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. > > > > the rte_mcslock is a good example for illustration. > > > > // original struct > > typedef struct rte_mcslock { > > struct rte_mcslock *next; > > ... > > }; > > > > it simply doesn't work / won't compile (at least with clang) which is > > what drove me to use the less-often used syntax. > > > > typedef struct rte_mcslock { > > _Atomic struct rte_mcslock *next; > > ... > > }; > > > > 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. > > > > 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. > > > > 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. > > > > // 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; > ... > }; this is what my v2 in the patch had. but following your const example you indicated you preferred the equivalent of `const T' over `T const` i was trying to illustrate that if you replace T = struct foo * the compiler can't disambiguate between type and pointer to type and produces an error. > > Like "const", the convention should be putting it before any type, but after the "*" for pointers. i see, thank you for this clarification. I had not understood that you were suggesting that for pointer types specifically i should use one placement and for non-pointer types i should use another. > > 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; > ... > }; actually I think for C11 atomics i think you can actually do this because you can declare an entire struct object to be atomic. However, since we need to intersect with what non-C11 gcc builtin atomics do we would not be able to make struct objects atomic as gcc only let's you do atomic things with integer and pointer types. > > > ... > > }; > > > > 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. > > > > e.g. > > void foo(int *v) > > > > 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. :-) > > > > > so i propose we retain > > > > #define __rte_atomic _Atomic > > > > 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. > > > > but also introduce the following macro > > > > #define RTE_ATOMIC(type) _Atomic(type) > > require it be used in the contexts that we are using it as a type- > > specifier. > > > > 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. yeah, I think it really helps clarify the pointer vs regular type specification by whacking the ( ) around what we are talking about instead of using positioning of _Atomic in two different places. > > 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. :-) yeah, i knew about this which is why i was being really careful about 'qualification' vs 'specification' in my mails. > > > > > thanks! > > Sorry about the late response, Tyler. Other work prevented me from setting aside coherent time to review your updated proposal. meh it's okay, based on the other thread i kind of guessed you might agree with using _Atomic(T) so i just submitted a new version an hour ago with the changes. i hope it meets your approval, one thing i'm kind of edgy about is the actual macro name itself RTE_ATOMIC(type) it seems kinda ugly, so if someone has an opinion there i'm open to it. > > > > > > > > > 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 *". > > > > 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. :-) i'll look into doing it once we get this series merged. thanks!