Re: rte_atomic_*_explicit

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-01-26 22:35, Tyler Retzlaff wrote:
> On Fri, Jan 26, 2024 at 11:52:11AM +0100, Morten Brørup wrote:
>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>> Sent: Friday, 26 January 2024 09.07
>>>
>>> On 2024-01-25 23:10, Morten Brørup wrote:
>>>>> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
>>>>> Sent: Thursday, 25 January 2024 19.54
>>>>>
>>>>> Why do rte_stdatomic.h functions have the suffix "_explicit"?
>>>>> Especially
>>>>> since there aren't any wrappers for the implicit variants.
>>>>>
>>>>> More to type, more to read.
>>>>
>>>> They have the "_explicit" suffix to make their names similar to those
>>> in stdatomic.h.
>>>>
>>>
>>> OK, so to avoid a situation where someone accidentally misinterpret
>>> rte_atomic_fetch_add(&counter, 1, rte_memory_order_relaxed);
>>> as what, exactly?
>>>
>>> If you have a wrapper, why not take the opportunity and reap some
>>> benefits from this and fix/extend the standard API, making it a better
>>> fit for your application. Like removing the redundant "_explicit",
>>> "fetch"-less add/sub, maybe and a function for single-writer atomic add
>>> (non-atomic read + non-atomic add + atomic store), etc.
>>>
>>> Prohibiting implicit operations was done already, so it's already now
>>> not a straight-off copy of the standard API.
>>>
>>>> You might consider their existence somewhat temporary until C11
>>> stdatomics can be fully phased in, so there's another argument for
>>> similar names. (This probably does not happen as long as compilers
>>> generate slower code for C11 stdatomics than with their atomic built-
>>> ins.)
>>>>
>>>
>>> To me, it seems reasonable a wrapper API should stay as long as it
>>> provide a benefit over the standard API. One could imagine that being a
>>> long time.
>>>
>>> I imagine some DPDK developers being tired of migrating from one
>>> atomics
>>> API to another. <rte_atomic.h> -> GCC built-ins (-> attempted C11
>>> stdatomics?) -> <rte_stdatomic.h>. Now you are adding a future "-> CXY
>>> atomics" move as well, and with that it also seems natural to add a
>>> change back to a wrapper or complementary API, when CXY didn't turned
>>> out good enough for some particular platform, or when some non-
>>> complaint
>>> compiler comes along.
>>
>> Yes, more migrations seem to be on the roadmap.
>>
>> We can take the opportunity to change direction now, and decide to keep the <rte_stdatomic.h> API long term.
>> Then it would need more documentation (basically copying function descriptions from <stdatomic.h>), and the functions could have the "_explicit" suffix removed (macros with the suffix could be added for backwards compatibility), and more functions - like the ones you suggested above - could be added.
>>
>> What do people think?
>> 1. Keep considering <rte_stdatomic.h> a temporary wrapper for <stdatomic.h> until compilers reach some undefined level of maturity, or
>> 2. Consider <rte_stdatomic.h> stable, clean it up (remove "_explicit" suffix), add documentation to the macros, and extend it.
>>
>> Living in a DPDK-only environment, I would prefer option 2; but if mixing DPDK code with non-DPDK code (that uses <stdatomic.h>) it might be weird.
> 
> rte_stdatomic.h should be considered temporary, but how long temporary
> is depends on when we can deprecate support for distributions and the
> older toolchains they are tied to.
> 
> the macros were introduced to allow a path to gradually moving to
> standard c11 atomics. gcc versions available on distributions we
> promise support for is currently the biggest barrier to direct
> adoption.
> 
>      * older versions of gcc generate sub-optimal code when using c11
>        atomic generics in some instances.
> 
>      * gcc c++23 support is incomplete, in particular at the time we
>        evaluated using the c11 atomics directly but gcc c++23 held us
>        back because the stdatomic.h it shipped didn't interoperate.
> 

So you expect at a particular point in time, both the following will be 
true:

* All currently (at that point) supported and future compilers generate 
correct and efficient code for all currently and future ISA/CPUs.
* There's nothing add, subtract or clean up in the standard API to 
improve it for DPDK-internal and DPDK application code use.

Seeing things like

rte_atomic_fetch_add_explicit(&s->num_mapped_cores, 1, 
rte_memory_order_relaxed);

doesn't exactly make you long for "raw" C11 atomics. It's not the like 
"rte_" being deleted improve much on that mess.

Compare it with the kernel's:

atomic_inc(&s->num_mapped_cores);

What could in DPDK be:

rte_atomic_inc(&s->num_mapped_cores);

or, at a bare minimum

rte_atomic_inc(&->num_mapped_cores, rte_memory_order_relaxed);

>      * the macros allow developers to easily evaluate/compare with and
>        without standard C atomics with a single build-time option
>        -Denable_stdatomic=true
> 
>      * eventually we will want to move directly to the names from the
>        standard, arguably just search and replace of rte_atomic with
>        atomic is the most mechanically trivial - hence there is some
>        value in keeping _explicit suffix.
> 

<rte_stdatomic.h> is a public API, and thus will be picked up by 
applications wanting to leverage DPDK for such services. 
Search-and-replace will only affect DPDK itself.

>>>
>>> I suggested fixing the original <rte_atomic.h> API, or at least have a
>>> wrapper API, already at the point DPDK moved to direct GCC built-in
>>> calls. Then we wouldn't have had this atomics API ping-pong.
>>
>> The decision back then might have been too hasty, and based on incomplete assumptions.
>> Please shout louder next time you think a mistake is in the making.
>>
>>>
>>>>>
>>>>> When was this API introduced? Shouldn't it say "experimental"
>>>>> somewhere?
>>>>
>>>> They were introduced as part of the migration to C11.
>>>> I suppose they were not marked experimental because they replaced
>>> something we didn't want anymore (the compiler built-ins for atomics,
>>> e.g. __atomic_load_n()). I don't recall if we discussed experimental
>>> marking or not.
>>
>> In hindsight, they should probably have been marked "experimental".
> 
> i'm not sure i feel strongly that they need to be marked experimental
> and by marked i assume we're only talking about placing a comment in a
> file rather than __rte_experimental which has no application here.
> 
> typically we do that when we may want to change or remove the api without
> going through the normal deprecation and removal process. for these
> macros it is difficult to imagine why we would change them as that would
> only cause them to deviate from the signature or behavior of the
> standard C generics... why would we do that if our intention is to
> eventually fully migrate to direct use of the standard C names?
> 
> ty