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 9064643064; Mon, 14 Aug 2023 19:47:09 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 80E7C410F1; Mon, 14 Aug 2023 19:47:09 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 95A0440A8A; Mon, 14 Aug 2023 19:47:07 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id C67F42106700; Mon, 14 Aug 2023 10:47:06 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com C67F42106700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1692035226; bh=u0eFUXWgJImODrH0HCYKK+BaxVnMl0Z/54MJvjVvVtM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ATio9EmbSUx9YNlB40HumWy89vbs2MREihA63raC42mTF9LoFNcwz51EbpOOavlOY JkK6/Pvfr5qPWKlnUwLetpRluuVAWTy5Nq++HxIEtw8S2zJEb+JgC9jOPOR/xIXmlQ Ysr3Ug3l2FfRK+BllozbBboHpA6yb6HHYmMJDJrM= Date: Mon, 14 Aug 2023 10:47:06 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: dev@dpdk.org, techboard@dpdk.org, Bruce Richardson , Honnappa Nagarahalli , Ruifeng Wang , Jerin Jacob , Sunil Kumar Kori , Mattias =?iso-8859-1?Q?R=F6nnblom?= , Joyce Kong , David Christensen , Konstantin Ananyev , David Hunt , Thomas Monjalon , David Marchand Subject: Re: [PATCH v2 2/6] eal: adapt EAL to present rte optional atomics API Message-ID: <20230814174706.GB12422@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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87AEF@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 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. > > 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 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. 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 :) thanks! > > 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. > > > } rte_spinlock_t; > > > > /** > > [...] > > > --- a/lib/eal/include/rte_mcslock.h > > +++ b/lib/eal/include/rte_mcslock.h > > @@ -33,8 +33,8 @@ > > * The rte_mcslock_t type. > > */ > > typedef struct rte_mcslock { > > - struct rte_mcslock *next; > > - int locked; /* 1 if the queue locked, 0 otherwise */ > > + struct rte_mcslock * __rte_atomic next; > > Correct, the pointer is atomic, not the struct. > > > + int __rte_atomic locked; /* 1 if the queue locked, 0 otherwise */ > > Again, I think __rte_atomic should be before the type: > __rte_atomic int locked; /* 1 if the queue locked, 0 otherwise */ > > > } rte_mcslock_t; > > > > [...] > > > @@ -101,34 +101,34 @@ > > * A pointer to the node of MCS lock passed in rte_mcslock_lock. > > */ > > static inline void > > -rte_mcslock_unlock(rte_mcslock_t **msl, rte_mcslock_t *me) > > +rte_mcslock_unlock(rte_mcslock_t * __rte_atomic *msl, rte_mcslock_t * > > __rte_atomic me) > > { > > /* Check if there are more nodes in the queue. */ > > - if (likely(__atomic_load_n(&me->next, __ATOMIC_RELAXED) == NULL)) { > > + if (likely(rte_atomic_load_explicit(&me->next, rte_memory_order_relaxed) > > == NULL)) { > > /* No, last member in the queue. */ > > - rte_mcslock_t *save_me = __atomic_load_n(&me, __ATOMIC_RELAXED); > > + rte_mcslock_t *save_me = rte_atomic_load_explicit(&me, > > rte_memory_order_relaxed); > > > > /* Release the lock by setting it to NULL */ > > - if (likely(__atomic_compare_exchange_n(msl, &save_me, NULL, 0, > > - __ATOMIC_RELEASE, __ATOMIC_RELAXED))) > > + if (likely(rte_atomic_compare_exchange_strong_explicit(msl, > > &save_me, NULL, > > + rte_memory_order_release, > > rte_memory_order_relaxed))) > > return; > > > > /* Speculative execution would be allowed to read in the > > * while-loop first. This has the potential to cause a > > * deadlock. Need a load barrier. > > */ > > - __atomic_thread_fence(__ATOMIC_ACQUIRE); > > + __rte_atomic_thread_fence(rte_memory_order_acquire); > > /* More nodes added to the queue by other CPUs. > > * Wait until the next pointer is set. > > */ > > - uintptr_t *next; > > - next = (uintptr_t *)&me->next; > > + uintptr_t __rte_atomic *next; > > + next = (uintptr_t __rte_atomic *)&me->next; > > This way around, I think: > __rte_atomic uintptr_t *next; > next = (__rte_atomic uintptr_t *)&me->next; > > [...] > > > --- a/lib/eal/include/rte_pflock.h > > +++ b/lib/eal/include/rte_pflock.h > > @@ -41,8 +41,8 @@ > > */ > > struct rte_pflock { > > struct { > > - uint16_t in; > > - uint16_t out; > > + uint16_t __rte_atomic in; > > + uint16_t __rte_atomic out; > > Again, I think __rte_atomic should be before the type: > __rte_atomic uint16_t in; > __rte_atomic uint16_t out; > > > } rd, wr; > > }; > > [...] > > > --- a/lib/eal/include/rte_seqcount.h > > +++ b/lib/eal/include/rte_seqcount.h > > @@ -32,7 +32,7 @@ > > * The RTE seqcount type. > > */ > > typedef struct { > > - uint32_t sn; /**< A sequence number for the protected data. */ > > + uint32_t __rte_atomic sn; /**< A sequence number for the protected data. > > */ > > Again, I think __rte_atomic should be before the type: > __rte_atomic uint32_t sn; /**< A sequence [...] > > > } rte_seqcount_t; > > [...] > > > --- a/lib/eal/include/rte_ticketlock.h > > +++ b/lib/eal/include/rte_ticketlock.h > > @@ -30,10 +30,10 @@ > > * The rte_ticketlock_t type. > > */ > > typedef union { > > - uint32_t tickets; > > + uint32_t __rte_atomic tickets; > > struct { > > - uint16_t current; > > - uint16_t next; > > + uint16_t __rte_atomic current; > > + uint16_t __rte_atomic next; > > Again, I think __rte_atomic should be before the type: > __rte_atomic uint16_t current; > __rte_atomic uint16_t next; > > > } s; > > } rte_ticketlock_t; > > > > > @@ -127,7 +129,7 @@ > > > > typedef struct { > > rte_ticketlock_t tl; /**< the actual ticketlock */ > > - int user; /**< core id using lock, TICKET_LOCK_INVALID_ID for unused */ > > + int __rte_atomic user; /**< core id using lock, TICKET_LOCK_INVALID_ID > > for unused */ > > Again, I think __rte_atomic should be before the type: > __rte_atomic int user; /**< core id [...] > > > unsigned int count; /**< count of time this lock has been called */ > > } rte_ticketlock_recursive_t; > > [...] > > > --- a/lib/eal/include/rte_trace_point.h > > +++ b/lib/eal/include/rte_trace_point.h > > @@ -33,7 +33,7 @@ > > #include > > > > /** The tracepoint object. */ > > -typedef uint64_t rte_trace_point_t; > > +typedef uint64_t __rte_atomic rte_trace_point_t; > > Again, I think __rte_atomic should be before the type: > typedef __rte_atomic uint64_t rte_trace_point_t; > > [...] > > At the risk of having gone "speed blind" by all the search-replaces along the way... > > Reviewed-by: Morten Brørup >