RE: rte_atomic_*_explicit

["Morten Brørup" <mb@smartsharesystems.com>]

> 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.

> 
> 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".