On Wed, Feb 08, 2023 at 09:31:32AM +0100, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roret...@linux.microsoft.com] > > Sent: Wednesday, 8 February 2023 02.21 > > > > On Tue, Feb 07, 2023 at 11:34:14PM +0000, Honnappa Nagarahalli wrote: > > > <snip> > > > > > > > > > > > > > > > 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
