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 0138541C41; Wed, 8 Feb 2023 17:35:24 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D642C410EE; Wed, 8 Feb 2023 17:35:23 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 699CF40141; Wed, 8 Feb 2023 17:35:22 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 7B5C220C8ACF; Wed, 8 Feb 2023 08:35:21 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 7B5C220C8ACF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1675874121; bh=9fWam7HmWbjPX5H1ZGQAgKbkm5a4bP/YrshInBpBJow=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=aJKA+U0iK0yWwPT/+3onGVn2wo9AwkxLpp/LsRhIXsfHg1Qi6gJ7oUFw58Rncbv3q 4rtn5xApBm6DBTt0jW7soieP8OjnaESqYwWQLds1Z7NIUE7xh0BnmCj2gkD4XZO9fU BM3RAwJjnq/TsJ9fzBqUvHHSwpsB0bV6udE4rBNw= Date: Wed, 8 Feb 2023 08:35:21 -0800 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Honnappa Nagarahalli , thomas@monjalon.net, dev@dpdk.org, bruce.richardson@intel.com, david.marchand@redhat.com, jerinj@marvell.com, konstantin.ananyev@huawei.com, ferruh.yigit@amd.com, nd , techboard@dpdk.org Subject: Re: [PATCH] eal: introduce atomics abstraction Message-ID: <20230208163521.GB5117@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1673558785-24992-1-git-send-email-roretzla@linux.microsoft.com> <1673558785-24992-2-git-send-email-roretzla@linux.microsoft.com> <1844463.CQOukoFCf9@thomas> <20230201214111.GA30564@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20230208012040.GA22219@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D8770D@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: <98CBD80474FA8B44BF855DF32C47DC35D8770D@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 Wed, Feb 08, 2023 at 09:31:32AM +0100, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Wednesday, 8 February 2023 02.21 > > > > On Tue, Feb 07, 2023 at 11:34:14PM +0000, Honnappa Nagarahalli wrote: > > > > > > > > > > > > > > > > > > Honnappa, please could you give your view on the future of > > atomics in > > > > DPDK? > > > > > Thanks Thomas, apologies it has taken me a while to get to this > > discussion. > > > > > > > > > > IMO, we do not need DPDK's own abstractions. APIs from > > stdatomic.h > > > > (stdatomics as is called here) already serve the purpose. These > > APIs are well > > > > understood and documented. > > > > > > > > i agree that whatever atomics APIs we advocate for should align > > with the > > > > standard C atomics for the reasons you state including implied > > semantics. > > > Another point I want to make is, we need 'xxx_explicit' APIs only, as > > we want memory ordering explicitly provided at each call site. (This > > can be discussed later). > > > > i don't have any issue with removing the non-explicit versions. they're > > just just convenience for seq_cst anyway. if people don't want them we > > don't have to have them. > > I agree with Honnappa on this point. > > The non-explicit versions are for lazy (or not so experienced) developers, and might impact performance if used instead of the correct explicit versions. > > I'm working on porting some of our application code from DPDK's rte_atomic32 operations to modern atomics, and I'm temporarily using acq_rel with a FIXME comment on each operation until I have the overview to determine if another memory order is better for each operation. And if I don't get around to fixing the memory order, it is still a step in the right direct direction to get rid of the old __sync based atomics; and the FIXME's remain to be fixed in a later release. > > So here's an idea: Alternatively to omitting the non-explicit versions, we could include them for application developers, but document them as placeholders for "memory order to be determined later" and emit a warning when used. It might speed up the transition away from old atomic operations. Alternatively, we risk thoughtless use of seq_cst with the explicit versions, which might be difficult to detect in code reviews. i think it may be cleaner to ust remove the non-explicit versions. if we are publishing api in the rte_xxx namespace then there are no pre-existing expectations that they are present. it also reduces the api surface that eventually gets retired ~years from now when all ports and compilers in the matrix are std=C11. i'll update the patch accordingly just so we have a visual. > > Either way, with or without non-explicit versions, is fine with me. > > > > > > > > > > > > > > > > > > > > For environments where stdatomics are not supported, we could > > have a > > > > stdatomic.h in DPDK implementing the same APIs (we have to support > > only > > > > _explicit APIs). This allows the code to use stdatomics APIs and > > when we move > > > > to minimum supported standard C11, we just need to get rid of the > > file in DPDK > > > > repo. > > > > > > > > my concern with this is that if we provide a stdatomic.h or > > introduce names > > > > from stdatomic.h it's a violation of the C standard. > > > > > > > > references: > > > > * ISO/IEC 9899:2011 sections 7.1.2, 7.1.3. > > > > * GNU libc manual > > > > https://www.gnu.org/software/libc/manual/html_node/Reserved- > > > > Names.html > > > > > > > > in effect the header, the names and in some instances namespaces > > introduced > > > > are reserved by the implementation. there are several reasons in > > the GNU libc > > > Wouldn't this apply only after the particular APIs were introduced? > > i.e. it should not apply if the compiler does not support stdatomics. > > > > yeah, i agree they're being a bit wishy washy in the wording, but i'm > > not convinced glibc folks are documenting this as permissive guidance > > against. > > > > > > > > > manual that explain the justification for these reservations and if > > if we think > > > > about ODR and ABI compatibility we can conceive of others. > > > > > > > > i'll also remark that the inter-mingling of names from the POSIX > > standard > > > > implicitly exposed as a part of the EAL public API has been > > problematic for > > > > portability. > > > These should be exposed as EAL APIs only when compiled with a > > compiler that does not support stdatomics. > > > > you don't necessarily compile dpdk, the application or its other > > dynamically linked dependencies with the same compiler at the same > > time. > > i.e. basically the model of any dpdk-dev package on any linux > > distribution. > > > > if dpdk is built without real stdatomic types but the application has > > to > > interoperate with a different kit or library that does they would be > > forced > > to dance around dpdk with their own version of a shim to hide our > > faked up stdatomics. > > > > So basically, if we want a binary DPDK distribution to be compatible with a separate application build environment, they both have to implement atomics the same way, i.e. agree on the ABI for atomics. > > Summing up, this leaves us with only two realistic options: > > 1. Go all in on C11 stdatomics, also requiring the application build environment to support C11 stdatomics. > 2. Provide our own DPDK atomics library. > > (As mentioned by Tyler, the third option - using C11 stdatomics inside DPDK, and requiring a build environment without C11 stdatomics to implement a shim - is not realistic!) > > I strongly want atomics to be available for use across inline and compiled code; i.e. it must be possible for both compiled DPDK functions and inline functions to perform atomic transactions on the same atomic variable. i consider it a mandatory requirement. i don't see practically how we could withdraw existing use and even if we had clean way i don't see why we would want to. so this item is defintely settled if you were concerned. > > So either we upgrade the DPDK build requirements to support C11 (including the optional stdatomics), or we provide our own DPDK atomics. i think the issue of requiring a toolchain conformant to a specific standard is a separate matter because any adoption of C11 standard atomics is a potential abi break from the current use of intrinsics. the abstraction (whatever namespace it resides) allows the existing toolchain/platform combinations to maintain compatibility by defaulting to current non-standard intrinsics. once in place it provides an opportunity to introduce new toolchain/platform combinations and enables an opt-in capability to use stdatomics on existing toolchain/platform combinations subject to community discussion on how/if/when. it would be good to get more participants into the discussion so i'll cc techboard for some attention. i feel like the only area that isn't decided is to do or not do this in rte_ namespace. i'm strongly in favor of rte_ namespace after discussion, mainly due to to disadvantages of trying to overlap with the standard namespace while not providing a compatible api/abi and because it provides clear disambiguation of that difference in semantics and compatibility with the standard api. so far i've noted the following * we will not provide the non-explicit apis. * we will make no attempt to support operate on struct/union atomics with our apis. * we will mirror the standard api potentially in the rte_ namespace to - reference the standard api documentation. - assume compatible semantics (sans exceptions from first 2 points). my vote is to remove 'potentially' from the last point above for reasons previously discussed in postings to the mail thread. thanks all for the discussion, i'll send up a patch removing non-explicit apis for viewing. ty