From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 90DC625D9 for ; Tue, 22 Jan 2019 23:25:43 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Jan 2019 14:25:42 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,508,1539673200"; d="scan'208";a="116598968" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga007.fm.intel.com with ESMTP; 22 Jan 2019 14:25:42 -0800 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 22 Jan 2019 14:25:41 -0800 Received: from fmsmsx108.amr.corp.intel.com ([169.254.9.99]) by fmsmsx122.amr.corp.intel.com ([169.254.5.2]) with mapi id 14.03.0415.000; Tue, 22 Jan 2019 14:25:41 -0800 From: "Eads, Gage" To: Honnappa Nagarahalli , "dev@dpdk.org" CC: "olivier.matz@6wind.com" , "arybchenko@solarflare.com" , "Richardson, Bruce" , "Ananyev, Konstantin" , nd , "chaozhu@linux.vnet.ibm.com" , "jerinj@marvell.com" , "hemant.agrawal@nxp.com" , nd Thread-Topic: [dpdk-dev] [PATCH v3 1/2] eal: add 128-bit cmpset (x86-64 only) Thread-Index: AQHUra7wYmJ+w7UMKEWkA1HXhAc3iqWy9AnAgAD6ulCAAI2KgIABGauAgAYqIlCAABqPMA== Date: Tue, 22 Jan 2019 22:25:41 +0000 Message-ID: <9184057F7FC11744A2107296B6B8EB1E541CA4CE@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> <9184057F7FC11744A2107296B6B8EB1E541C94D9@FMSMSX108.amr.corp.intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiM2JmODZjYzUtNTRhMi00MGZmLTkxMDgtYjMwZDM2YTFjNDhjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiYnl5bjVHWHlKWmVmaWlaNXh6c0hMbk5MMEpZdkNtbHgwWDl4UWp6OEZ6cnVjYnl3bXhmTVwvbFVcL2k5aVNRVFFDIn0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.1.200.106] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 1/2] eal: add 128-bit cmpset (x86-64 only) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Jan 2019 22:25:44 -0000 > -----Original Message----- > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] > Sent: Tuesday, January 22, 2019 2:31 PM > To: Eads, Gage ; dev@dpdk.org > Cc: olivier.matz@6wind.com; arybchenko@solarflare.com; Richardson, Bruce > ; Ananyev, Konstantin > ; nd ; > chaozhu@linux.vnet.ibm.com; jerinj@marvell.com; hemant.agrawal@nxp.com; > nd > Subject: RE: [dpdk-dev] [PATCH v3 1/2] eal: add 128-bit cmpset (x86-64 on= ly) >=20 > Added other platform owners. >=20 > > > > > > @@ -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 's= rc' > > > > > 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 > > > > > > > > > > > > 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 couple > > > 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 all 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 later > > > 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 supported 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- > > Builtins.html > (following is based on my reading, not based on experiments) My understan= ding > is that the __atomic_load_n/store_n were implemented using a compare-and- > swap HW instruction (due to lack of HW 128b atomic load and store > instructions). This introduced the side effect or store/load respectively= . Where > as the user does not expect such side effects. As suggested in [1], it lo= oks like > GCC delegated the implementation to libatomic which 'it seems' uses locks= to > implement 128b __atomic intrinsics (needs to be verified) > > If __atomic intrinsics, for any of the supported platforms, do not have a= n > optimal implementation, I prefer to add a DPDK API as an abstraction. A g= iven > platform can choose to implement such an API using __atomic intrinsics if= it > wants. The DPDK API can be similar to __atomic_compare_exchange_n. >=20 Certainly. From the linked discussions, I see how this would affect the des= ign of (hypothetical functions) rte_atomic128_read() and rte_atomic128_set(= ), but I don't see anything that suggests (for the architectures being disc= ussed) that __atomic_compare_exchange_n is suboptimal. > [1] https://patchwork.ozlabs.org/patch/721686/ > [2] https://gcc.gnu.org/ml/gcc/2017-01/msg00167.html >=20 > > > > > 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 been proven to provide best > > > flexibility for > > all the platforms supported by DPDK.