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 CA156423AA; Wed, 11 Jan 2023 08:45:22 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B3F1E40691; Wed, 11 Jan 2023 08:45:22 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 71F964014F for ; Wed, 11 Jan 2023 08:45:21 +0100 (CET) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: RFC abstracting atomics Date: Wed, 11 Jan 2023 08:45:20 +0100 X-MimeOLE: Produced By Microsoft Exchange V6.5 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8764B@smartserver.smartshare.dk> In-Reply-To: <20230110203124.GD21476@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: RFC abstracting atomics Thread-Index: AdklMoU7HLZOC5H2RwOeuN1AoUmdFgAWzSyg References: <20230109225604.GA25566@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D87648@smartserver.smartshare.dk> <20230110203124.GD21476@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Tyler Retzlaff" Cc: "Bruce Richardson" , 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 > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > Sent: Tuesday, 10 January 2023 21.31 >=20 > On Tue, Jan 10, 2023 at 12:45:05PM +0100, Morten Br=F8rup wrote: > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > > > Sent: Tuesday, 10 January 2023 10.17 > > > > > > On Mon, Jan 09, 2023 at 02:56:04PM -0800, Tyler Retzlaff wrote: > > > > hi folks, > > > > > > > > i would like to introduce a layer of abstraction that would = allow > > > > optional use of standard C11 atomics when the platform / > toolchain > > > > combination has them available. > > > > It would be great if that abstraction layer could expose the exact > same (or very similar) interface as standard C11 atomics, so the API = is > given in advance, and developers don't have to learn the API of yet > another shim. It would also make it easier getting rid of the > abstraction layer at a later time. >=20 > the patch series i have prototype exactly mirrors the stdandard = atomics > from C11. it requires a few fixups to fill convenience builtins that > the > standard doesn't provide e.g. `__atomic_add_fetch(&o, v, order)` > becomes > `__atomic_fetch_add(&o, v, order) + v;` sort of stuff, mostly minor. Excellent! >=20 > > > > > > > > > > making the option usable would be a phased approach intended to > focus > > > > review and minimize dealing with churn on such a broad change. > > > > > > > > 1. provide an initial series to add the abstraction and the > ability > > > > control enablement with a meson option = enable_stdatomics=3Dfalse > > > will > > > > be the default. > > > > > > > > for all existing platform / toolchain combinations the = default > > > would > > > > remain false. i.e. i have no plans to enable it for existing > > > platforms > > > > toolchain combinations but leaves a change of default open to > the > > > > community as a future discussion if it is desired. > > > > > > > > 2. once the initial abstraction is integrated a series will be > > > introduced to > > > > port the tree to the abstraction with = enable_stdatomics=3Dfalse. > the > > > goal > > > > being low or no change to the current use of gcc builtin = C++11 > > > memory > > > > model atomics. > > > > > > > > 3. once the tree is ported a final series will be introduced to > > > introduce > > > > the remaining change to allow the use of > enable_stdatomics=3Dtrue. > > > > > > > > would appreciate any assistance / suggestions you can provide to > > > > introduce the abstraction smoothly. > > > > > > > > > > Plan generally sounds ok. However, beyond point #3, would there > then be > > > plans to remove the option and always use stdatomics in future? > > > > We are already trying to migrate atomics in DPDK from using __sync > built-ins (used in rte_atomic) to using __atomic built-ins; e.g. new > contributions are recommended to use the latter (and not rte_atomic). > When DPDK migrates to C11, I assume stdatomics will become the new > standard for atomics in DPDK, and the process for migrating to > stdatomics will resemble the currently ongoing process for migrating > from rte_atomic to __atomic built-ins, which seems to be relatively > uncomplicated (but slow). >=20 > the few __sync left in use are easily converted. there might be some > argument against converting __sync -> __atomic because of sub-optimal > codegen on older gcc for the semantically equivalent replacements. It would be easy to simply replace __sync with similar __atomic = functions in the rte_atomic functions, but that is not the challenge. The challenge is that the use of rte_atomic assumes the very strict = order imposed by __sync (a full barrier), but the performance of some = use cases could be improved by using the specific order required for = those use cases (e.g. acquire/release semantics). >=20 > the only comment i have on current/existing rte_atomic is they're > annoyingly stealing a little bit of the rte_ namespace that would = allow > convenient alignment with the standard names. also some of them = provide > only signed or unsigned variations which forces kind of non-standardy > things to be done in the code but are perfectly acceptable when using > the gcc implementation. Yes, their signedness is slightly annoying, and makes some code look = weird. >=20 > with the set of macros i'm introducing migrating to direct use of C11 > atomics would likely be just a big search and replace (or that is my > goal). Again, excellent! >=20 > > Another thing: The proposed layer of abstraction should only live > until DPDK supports C11, which is hopefully relatively soon. So is = such > a (hopefully) short-lived abstraction layer worth the effort? >=20 > i kept hoping we would just baseline on C11 but after prototyping the > abstraction i realize there may be some valid arguments for not moving > to stdatomics now or possibly ever for some platform/toolchain > combinations (so long as they are declared supported). given the drawn > out discussion about RHEL-7 has shown that the migration path may be > long term. more possible headwinds were listed in my reply to Bruce. No objections from me - especially with the stdatomics roadmap you = described. >=20 > > > > > > > > To slightly expand the scope of the discussion - would it be > worthwhile > > > putting these abstractions in a new library in DPDK other than = EAL, > to > > > start the process of splitting out some of the lower-level = material > > > from > > > that library? > > > > While I applaud this idea, I think we first need to discuss in = detail > how to split up EAL, and especially what new structure we want to end > up with instead. E.g. we already have architecture specific code in > various libraries and drivers, which proves that architecture specific > stuff does not necessarily need to go into the EAL. We also need to > discuss challenges/barriers to separating stuff from the EAL, and how > we can solve those challenges; e.g. some parts are inside the EAL only > due to startup sequence requirements, which might be solvable by > offering better startup sequencing support for libraries in the EAL. > Perhaps the EAL has even outlived itself, and could be replaced by set > of DPDK Core libraries instead - some could be optional (at build > time). But let's make this a separate discussion. >=20 > i gave some detail about this to my reply with Bruce. i think when i > post the series we can discuss it more. but for atomics i expect it > isn't worth separating out. I think we're all in agreement at this point. But as Bruce just did, we should think twice every time something is = being proposed to be added to the EAL.