From mboxrd@z Thu Jan 1 00:00:00 1970 From: Honnappa Nagarahalli Subject: Re: [PATCH 1/1] eal: add 128-bit cmpset (x86-64 only) Date: Thu, 31 Jan 2019 05:48:14 +0000 Message-ID: References: <20190128172945.27251-1-gage.eads@intel.com> <20190128172945.27251-2-gage.eads@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "olivier.matz@6wind.com" , "arybchenko@solarflare.com" , "bruce.richardson@intel.com" , "konstantin.ananyev@intel.com" , "Gavin Hu (Arm Technology China)" , nd , "chaozhu@linux.vnet.ibm.com" , "jerinj@marvell.com" , "hemant.agrawal@nxp.com" , nd To: Gage Eads , "dev@dpdk.org" Return-path: Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-eopbgr80084.outbound.protection.outlook.com [40.107.8.84]) by dpdk.org (Postfix) with ESMTP id 294CC1B3A7 for ; Thu, 31 Jan 2019 06:48:16 +0100 (CET) In-Reply-To: <20190128172945.27251-2-gage.eads@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >=20 > This operation can be used for non-blocking algorithms, such as a non- > blocking stack or ring. >=20 > Signed-off-by: Gage Eads > --- > .../common/include/arch/x86/rte_atomic_64.h | 31 +++++++++++ > lib/librte_eal/common/include/generic/rte_atomic.h | 65 > ++++++++++++++++++++++ > 2 files changed, 96 insertions(+) >=20 > diff --git a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h > b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h > index fd2ec9c53..b7b90b83e 100644 > --- a/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h > +++ b/lib/librte_eal/common/include/arch/x86/rte_atomic_64.h > @@ -34,6 +34,7 @@ > /* > * Inspired from FreeBSD src/sys/amd64/include/atomic.h > * Copyright (c) 1998 Doug Rabson > + * Copyright (c) 2019 Intel Corporation > * All rights reserved. > */ >=20 > @@ -46,6 +47,7 @@ >=20 > #include > #include > +#include > #include >=20 > /*------------------------- 64 bit atomic operations -------------------= ------*/ @@ - > 208,4 +210,33 @@ static inline void rte_atomic64_clear(rte_atomic64_t *v)= } > #endif >=20 > +static inline int __rte_experimental > +rte_atomic128_cmpset(volatile rte_int128_t *dst, Does it make sense to call is rte_atomic128_compare_exchange (or ..._cmp_xc= hg) to indicate it is a compare-exchange operation? > + rte_int128_t *exp, rte_int128_t *src, > + unsigned int weak, > + enum rte_atomic_memmodel_t success, > + enum rte_atomic_memmodel_t failure) { > + RTE_SET_USED(weak); > + RTE_SET_USED(success); > + RTE_SET_USED(failure); > + uint8_t res; > + > + asm volatile ( > + MPLOCKED > + "cmpxchg16b %[dst];" > + " sete %[res]" > + : [dst] "=3Dm" (dst->val[0]), > + "=3DA" (exp->val[0]), > + [res] "=3Dr" (res) > + : "c" (src->val[1]), > + "b" (src->val[0]), > + "m" (dst->val[0]), > + "d" (exp->val[1]), > + "a" (exp->val[0]) > + : "memory"); > + > + return res; > +} > + > #endif /* _RTE_ATOMIC_X86_64_H_ */ > diff --git a/lib/librte_eal/common/include/generic/rte_atomic.h > b/lib/librte_eal/common/include/generic/rte_atomic.h > index b99ba4688..8d612d566 100644 > --- a/lib/librte_eal/common/include/generic/rte_atomic.h > +++ b/lib/librte_eal/common/include/generic/rte_atomic.h > @@ -14,6 +14,7 @@ >=20 > #include > #include > +#include >=20 > #ifdef __DOXYGEN__ >=20 > @@ -1082,4 +1083,68 @@ static inline void > rte_atomic64_clear(rte_atomic64_t *v) } #endif >=20 > +/*------------------------ 128 bit atomic operations > +-------------------------*/ > + > +/** > + * 128-bit integer structure. > + */ > +typedef struct { > + uint64_t val[2]; > +} __rte_aligned(16) rte_int128_t; It looks like '__int128' is available from gcc 4.6. I think we should use '= __int128'. We can have it as an internal structure for ease of programming. > + > +/** > + * Memory consistency models used in atomic operations. These control > +the > + * behavior of the operation with respect to memory barriers and > + * thread synchronization. > + * > + * These directly match those in the C++11 standard; for details on > +their > + * behavior, refer to the standard. > + */ > +enum rte_atomic_memmodel_t { > + RTE_ATOMIC_RELAXED, > + RTE_ATOMIC_CONSUME, > + RTE_ATOMIC_ACQUIRE, > + RTE_ATOMIC_RELEASE, > + RTE_ATOMIC_ACQ_REL, > + RTE_ATOMIC_SEQ_CST, > +}; IMO, we can use the GCC provided names. I do not see any advantage to defin= ing our own. > + > +/* Only implemented on x86-64 currently. The ifdef prevents compilation > +from > + * failing for architectures without a definition of this function. > + */ Minor comment. We can skip the above comments, the #if below is pretty obvi= ous. > +#if defined(RTE_ARCH_X86_64) > +/** > + * An atomic compare and set function used by the mutex functions. > + * (atomic) equivalent to: > + * if (*dst =3D=3D exp) > + * *dst =3D src (all 128-bit words) > + * > + * @param dst > + * The destination into which the value will be written. > + * @param exp > + * Pointer to the expected value. If the operation fails, this memory = is > + * updated with the actual value. > + * @param src > + * Pointer to the new value. > + * @param weak > + * A value of true allows the comparison to spuriously fail. > Implementations > + * may ignore this argument and only implement the strong variant. > + * @param success > + * If successful, the operation's memory behavior conforms to this (or= a > + * stronger) model. > + * @param failure > + * If unsuccessful, the operation's memory behavior conforms to this (= or a > + * stronger) model. This argument cannot be RTE_ATOMIC_RELEASE, > + * RTE_ATOMIC_ACQ_REL, or a stronger model than success. > + * @return > + * Non-zero on success; 0 on failure. > + */ > +static inline int __rte_experimental > +rte_atomic128_cmpset(volatile rte_int128_t *dst, > + rte_int128_t *exp, rte_int128_t *src, > + unsigned int weak, > + enum rte_atomic_memmodel_t success, > + enum rte_atomic_memmodel_t failure); #endif > + > #endif /* _RTE_ATOMIC_H_ */ > -- > 2.13.6