From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Eads, Gage" Subject: Re: [PATCH v3 1/2] eal: add 128-bit cmpset (x86-64 only) Date: Fri, 18 Jan 2019 22:01:51 +0000 Message-ID: <9184057F7FC11744A2107296B6B8EB1E541C94D9@FMSMSX108.amr.corp.intel.com> References: <20190115223232.31866-1-gage.eads@intel.com> <20190116151835.22424-1-gage.eads@intel.com> <20190116151835.22424-2-gage.eads@intel.com> <9184057F7FC11744A2107296B6B8EB1E541C8BCA@FMSMSX108.amr.corp.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" , "Richardson, Bruce" , "Ananyev, Konstantin" , nd , nd To: Honnappa Nagarahalli , "dev@dpdk.org" Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id C5701201 for ; Fri, 18 Jan 2019 23:01:53 +0100 (CET) In-Reply-To: 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" > -----Original Message----- > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > Sent: Thursday, January 17, 2019 11:28 PM > To: Eads, Gage ; dev@dpdk.org > Cc: olivier.matz@6wind.com; arybchenko@solarflare.com; Richardson, Bruce > ; Ananyev, Konstantin > ; nd ; nd > Subject: RE: [dpdk-dev] [PATCH v3 1/2] eal: add 128-bit cmpset (x86-64 on= ly) >=20 > > > > > > > > This operation can be used for non-blocking algorithms, such as a > > > > non- blocking stack or ring. > > > > > > > > Signed-off-by: Gage Eads > > > > --- > > > > .../common/include/arch/x86/rte_atomic_64.h | 22 > > > > ++++++++++++++++++++++ > > > > 1 file changed, 22 insertions(+) > > > > > > > > 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..34c2addf8 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 > > > Since this is a 128b operation should there be a new file created > > > with the name rte_atomic_128.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. > > > > */ > > > > > > > > @@ -208,4 +209,25 @@ static inline void > > > > rte_atomic64_clear(rte_atomic64_t > > > > *v) } #endif > > > > > > > > +static inline int > > > > +rte_atomic128_cmpset(volatile uint64_t *dst, uint64_t *exp, > > > > +uint64_t > > > > +*src) { > > > The API name suggests it is a 128b operation. 'dst', 'exp' and 'src' > > > should be pointers to 128b (__int128)? Or we could define our own > > > data > > type. > > > > I agree, I'm not a big fan of the 64b pointers here. I avoided > > __int128 originally because it fails to compile with -pedantic, but on > > second thought (and with your suggestion of a separate data type), we > > can resolve that with this typedef: > > > > typedef struct { > > RTE_STD_C11 __int128 val; > > } rte_int128_t; > ok >=20 > > > > > Since, it is a new API, can we define it with memory orderings which > > > will be more conducive to relaxed memory ordering based architectures= ? > > > You can refer to [1] and [2] for guidance. > > > > I certainly see the value in controlling the operation's memory > > ordering, like in the __atomic intrinsics, but I'm not sure this > > patchset is the right place to address that. I see that work going a co= uple > ways: > > 1. Expand the existing rte_atomicN_* interfaces with additional > > arguments. In that case, I'd prefer this be done in a separate > > patchset that addresses all the atomic operations, not just cmpset, so > > the interface changes are chosen according to the needs of the full > > set of atomic operations. If this approach is taken then there's no > > need to solve this while rte_atomic128_cmpset is experimental, since al= l the > other functions are non-experimental anyway. > > > > - Or - > > > > 2. Don't modify the existing rte_atomicN_* interfaces (or their > > strongly ordered behavior), and instead create new versions of them > > that take additional arguments. In this case, we can implement > > rte_atomic128_cmpset() as is and create a more flexible version in a la= ter > patchset. > > > > Either way, I think the current interface (w.r.t. memory ordering > > options) can work and still leaves us in a good position for future > changes/improvements. > > > I do not see the need to modify/extend the existing rte_atomicN_* APIs as= the > corresponding __atomic intrinsics serve as replacements. I expect that at= some > point, DPDK code base will not be using rte_atomicN_* APIs. > However, __atomic intrinsics do not support 128b wide parameters. Hence I don't think that's correct. From the GCC docs: "16-byte integral types are also allowed if `__int128' (see __int128) is su= pported by the architecture." This works with x86 -64 -- I assume aarch64 also, but haven't confirmed. Source: https://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/_005f_005fatomic-Built= ins.html > DPDK needs to write its own. Since this is the first API in that regard, = I prefer that > we start with a signature that resembles __atomic intrinsics which have b= een > proven to provide best flexibility for all the platforms supported by DPD= K.