<snip> > Subject: Re: [PATCH v4 4/4] eal/atomic: add wrapper for c11 atomics > > On 2020-05-14 10:34, Morten Brørup wrote: > > + Added people from the related discussion regarding the ARM roadmap > [https://protect2.fireeye.com/v1/url?k=10efdd7b-4e4f1ed2-10ef9de0- > 86959e472243-b772fef31e4ae6af&q=1&e=e3b0051e-bb23-4a30-84c7- > 7e5e80f83325&u=https%3A%2F%2Fmails.dpdk.org%2Farchives%2Fdev%2F20 > 20-April%2F162580.html]. > > > >> From: Mattias Rönnblom [mailto:mattias.ronnb...@ericsson.com] > >> Sent: Wednesday, May 13, 2020 10:17 PM > >> > >> On 2020-05-13 21:40, Honnappa Nagarahalli wrote: > >>> <snip> > >>> > >>>>>> Subject: Re: [PATCH v4 4/4] eal/atomic: add wrapper for c11 > >> atomics > >>>>>> On Tue, May 12, 2020 at 4:03 pm, Phil Yang > >> <mailto:phil.y...@arm.com> > >>>>>> wrote: > >>>>>> > >>>>>> parameter. Signed-off-by: Phil Yang <mailto:phil.y...@arm.com> > >>>>>> > >>>>>> > >>>>>> What is the purpose of having rte_atomic at all? > >>>>>> Is this level of indirection really helping? > >>>>>> [HONNAPPA] (not sure why this email has html format, converted to > >>>>>> text > >>>>>> format) > >>>>>> I believe you meant, why not use the __atomic_xxx built-ins > >> directly? > >>>>>> The only reason for now is handling of > >>>>>> __atomic_thread_fence(__ATOMIC_SEQ_CST) for x86. This is > >> equivalent > >>>>>> to rte_smp_mb which has an optimized implementation for x86. > >>>>>> According to Konstantin, the compiler does not generate optimal > >> code. > >>>>>> Wrapping that built-in alone is going to be confusing. > >>>>>> > >>>>>> The wrappers also allow us to have our own implementation using > >>>>>> inline assembly for compilers versions that do not support C11 > >> atomic > >>>>>> built- ins. But, I do not know if there is a need to support > >>>>>> those > >> versions. > >>>>> If I recall correctly, someone mentioned that one (or more) of the > >> aging > >>>> enterprise Linux distributions don't include a compiler with C11 > >> atomics. > >>>>> I think Stephen is onto something here... > >>>>> > >>>>> It is silly to add wrappers like this, if the only purpose is to > >> support > >>>> compilers and distributions that don't properly support an official > >> C standard > >>>> which is nearly a decade old. The quality and quantity of the DPDK > >>>> documentation for these functions (including examples, discussions > >> on Stack > >>>> Overflow, etc.) will be inferior to the documentation of the > >> standard C11 > >>>> atomics, which increases the probability of incorrect use. > >>>> > >>>> > >>>> What's being used in DPDK today, and what's being wrapped here, is > >> not > >>>> standard C11 atomics - it's a bunch of GCC built-ins. Nothing in > >>>> the > >> __ > >>>> namespace is in the standard. It's reserved for the implementation > >> (e.g. > >>>> compiler). > >>> I have tried to understand what it mean by 'built-ins', but I have > >> not got a good answer. So, does it mean that the built-in function > >> (same symbol and API interface) may not be available in another C > >> compiler? IMO, this is what matters for DPDK. > >>> Currently, the same built-in functions are available in GCC and > >> Clang. > >> > >> > >> From what I understand, "built-ins" is GCC terminology for > >> non-standard, implementation-specific intrinsic functions, built into > >> the compiler. They all reside in the __* namespace. > >> > >> > >> Since GCC is the industry standard, other compilers are likely to > >> follow, including built-in functions. > >> > > Timeline: > > > > December 2011: The C11 standard was published > [https://protect2.fireeye.com/v1/url?k=8e23b012-d08373bb-8e23f089- > 86959e472243-a2babe7075f8ac38&q=1&e=e3b0051e-bb23-4a30-84c7- > 7e5e80f83325&u=http%3A%2F%2Fwww.open- > std.org%2Fjtc1%2Fsc22%2Fwg14%2Fwww%2Fstandards.html]. > > > > March 2012: GCC 4.7 was released, introducing the __atomic built-ins > [https://gcc.gnu.org/gcc-4.7/changes.html, > https://www.gnu.org/software/gcc/gcc-4.7/]. > > > > March 2013: GCC 4.8 was released [https://www.gnu.org/software/gcc/gcc- > 4.8/]. > > > > April 2014: GCC 4.9 was released, introducing C11 atomics (incl. > <stdatomic.h>) [https://gcc.gnu.org/gcc-4.9/changes.html, > https://www.gnu.org/software/gcc/gcc-4.9/]. > > > > June 2014: RHEL7 was released > > [https://access.redhat.com/articles/3078]. (RHEL7 Beta was released in > > December 2013, which probably explains why the GA release doesn’t > > include GCC 4.9.) > > > > May 2019 (i.e. one year ago): RHEL8 was released > [https://access.redhat.com/articles/3078]. > > > > > > RHEL7 includes GCC 4.8 only [https://access.redhat.com/solutions/19458], > and apparently RHEL7 has not been updated to GCC 4.9 with any of its minor > releases. > > > > Should the DPDK project be stuck on "industry standard" GCC atomics, > unable to use the decade old "official standard" C11 atomics, only because > we want to support a six year old enterprise Linux distribution? Red Hat > released a new enterprise version a year ago... perhaps it's time for their > customers to upgrade, if they want to use the latest and greatest version of > DPDK. > > > Just to be clear - I wasn't arguing for the direct use of GCC built-ins. > > > The GCC __atomic built-ins (called directly, or via a DPDK wrapper) do have > some advantages over C11 atomics. One is that GCC supports 128-bit atomic > operations, on certain architectures. <rte_atomic.h> already has a 128-bit > compare-exchange. Also, since the GCC built-ins seem not to bother with > architectures where atomics would be implemented by means of a lock, they > are a little easier to use than <stdatomic.h>. IMO, I do not think we should focus on built-ins vs APIs.
1) Built-ins are supported by both GCC and Clang today. If there is a new compiler in the future, most likely it will support these built-ins. 2) I like the fact that the built-ins always require the memory order parameter. stdatomic.h provides some APIs which do not need memory order (just like rte_atomicNN_xxx APIs). This needs us to implement checks in checkpatch script to avoid using such APIs. 3) If we need to replace the built-ins with APIs in the future, it is a simple search and replace. If the decision to go with built-ins, turns out to be a bad decision, it can be corrected easily. I think we should focus on the compiler not generating optimal code for __atomic_thread_fence(__ATOMIC_SEQ_CST) for x86. This is the main reason for these wrappers. From what I have seen, DPDK has tried to provide solutions internally for performance issues caused by compilers. Given that we have provided 'rte_atomic128_cmp_exchange' (provided because both the compilers were not generating the 128b compare-exchange), I would say we should just provide wrapper for '__atomic_thread_fence' built-in. > > > > Are all the other tools required for building DPDK (in the required > > versions) > included in RHEL7, or do we require developers to install/upgrade any other > tools anyway? If so, why not also GCC? DPDK can be used in a cross > compilation environment, so we are not requiring RHEL7 users to replace > their GCC 4.7 default compiler. I have not used RHEL7, Intel CI uses RHEL7, may be they can answer. > > > > > > Furthermore, the DPDK Documentation specifies GCC 4.9+ as a system > requirement [https://protect2.fireeye.com/v1/url?k=339bad56-6d3b6eff- > 339bedcd-86959e472243-cb1bf3934c202e3f&q=1&e=e3b0051e-bb23-4a30- > 84c7- > 7e5e80f83325&u=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Flinux_gsg%2F > sys_reqs.html%23compilation-of-the-dpdk]. If we are stuck on GCC 4.8, the > documentation should be updated. This is interesting. Then the CI systems should be upgraded to use GCC 4.9+. > > > > > >>>>> And if some compiler generates code that is suboptimal for a user, > >> then it > >>>> should be the choice of the user to either accept it or use a > >>>> better > >> compiler. > >>>> Using a suboptimal compiler will not only affect the user's DPDK > >> applications, > >>>> but all applications developed by the user. And if he accepts it > >>>> for > >> his other > >>>> applications, he will also accept it for his DPDK applications. > >>>>> We could introduce some sort of marker or standardized comment to > >>>> indicate when functions only exist for backwards compatibility with > >> ancient > >>>> compilers and similar, with a reference to documentation describing > >> why. And > >>>> when the documented preconditions are no longer relevant, e.g. when > >> those > >>>> particular enterprise Linux distributions become obsolete, these > >> functions > >>>> become obsolete too, and should be removed. However, getting rid of > >>>> obsolete cruft will break the ABI. In other words: Added cruft will > >> never be > >>>> removed again, so think twice before adding. >
