rte_atomic_*_explicit

rte_atomic_*_explicit

[Mattias Rönnblom]

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.

When was this API introduced? Shouldn't it say "experimental" somewhere?

RE: rte_atomic_*_explicit

[Morten Brørup]

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

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

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


Reverse paper trail:
https://git.dpdk.org/dpdk/log/lib/eal/include/rte_stdatomic.h
https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-send-email-roretzla@linux.microsoft.com/
https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-send-email-roretzla@linux.microsoft.com/

Re: rte_atomic_*_explicit

[Tyler Retzlaff]

On Thu, Jan 25, 2024 at 11:10:47PM +0100, 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.
> 
> 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.)

yes, there was feedback at the time it was.

* we should *not* have non-explicit versions of the macros
* the atomic generic functions should be named to match C11 standard
  with a rte_ prefix.

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

i don't think we discussed it since they're wrapper macros.

> 
> 
> Reverse paper trail:
> https://git.dpdk.org/dpdk/log/lib/eal/include/rte_stdatomic.h
> https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-send-email-roretzla@linux.microsoft.com/
> https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-send-email-roretzla@linux.microsoft.com/
> 

RE: rte_atomic_*_explicit

[Honnappa Nagarahalli]

<snip>

> 
> On Thu, Jan 25, 2024 at 11:10:47PM +0100, 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.
> >
> > 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.)
> 
> yes, there was feedback at the time it was.
> 
> * we should *not* have non-explicit versions of the macros
> * the atomic generic functions should be named to match C11 standard
>   with a rte_ prefix.
This was mainly done to ensure that users think through the memory ordering they want to use. This also matches with the compiler atomic built-ins. Without explicit, it is sequentially consistent memory order.

> 
> >
> > >
> > > 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.
> 
> i don't think we discussed it since they're wrapper macros.
> 
> >
> >
> > Reverse paper trail:
> > https://git.dpdk.org/dpdk/log/lib/eal/include/rte_stdatomic.h
> > https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-
> send-email-roretzla@linux.microsoft.com/
> > https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-
> send-email-roretzla@linux.microsoft.com/
> >

Re: rte_atomic_*_explicit

[Mattias Rönnblom]

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.

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.

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



> Reverse paper trail:
> https://git.dpdk.org/dpdk/log/lib/eal/include/rte_stdatomic.h
> https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-send-email-roretzla@linux.microsoft.com/
> https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-send-email-roretzla@linux.microsoft.com/
> 

Re: rte_atomic_*_explicit

[Mattias Rönnblom]

On 2024-01-26 02:37, Honnappa Nagarahalli wrote:
> <snip>
> 
>>
>> On Thu, Jan 25, 2024 at 11:10:47PM +0100, 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.
>>>
>>> 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.)
>>
>> yes, there was feedback at the time it was.
>>
>> * we should *not* have non-explicit versions of the macros
>> * the atomic generic functions should be named to match C11 standard
>>    with a rte_ prefix.
> This was mainly done to ensure that users think through the memory ordering they want to use. This also matches with the compiler atomic built-ins. Without explicit, it is sequentially consistent memory order.
> 

"This" is referring to the first bullet only, correct?

You don't have to distinguish between implicit and explicit if you only 
have explicit.

>>
>>>
>>>>
>>>> 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.
>>
>> i don't think we discussed it since they're wrapper macros.
>>
>>>
>>>
>>> Reverse paper trail:
>>> https://git.dpdk.org/dpdk/log/lib/eal/include/rte_stdatomic.h
>>> https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-
>> send-email-roretzla@linux.microsoft.com/
>>> https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git-
>> send-email-roretzla@linux.microsoft.com/
>>>

RE: rte_atomic_*_explicit

[Morten Brørup]

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

RE: rte_atomic_*_explicit

[Honnappa Nagarahalli]

<snip>

> >
> >>
> >> On Thu, Jan 25, 2024 at 11:10:47PM +0100, 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.
> >>>
> >>> 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.)
> >>
> >> yes, there was feedback at the time it was.
> >>
> >> * we should *not* have non-explicit versions of the macros
> >> * the atomic generic functions should be named to match C11 standard
> >>    with a rte_ prefix.
> > This was mainly done to ensure that users think through the memory
> ordering they want to use. This also matches with the compiler atomic built-
> ins. Without explicit, it is sequentially consistent memory order.
> >
> 
> "This" is referring to the first bullet only, correct?
> 
> You don't have to distinguish between implicit and explicit if you only have
> explicit.
Agree on your thought.

The '_explicit' was added to be aligned with the standard atomic API naming. The thought was - if we are aligned on the names, it needs less explanation for users.

> 
> >>
> >>>
> >>>>
> >>>> 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.
> >>
> >> i don't think we discussed it since they're wrapper macros.
> >>
> >>>
> >>>
> >>> Reverse paper trail:
> >>> https://git.dpdk.org/dpdk/log/lib/eal/include/rte_stdatomic.h
> >>> https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git
> >>> -
> >> send-email-roretzla@linux.microsoft.com/
> >>> https://patchwork.dpdk.org/project/dpdk/patch/1692738045-32363-2-git
> >>> -
> >> send-email-roretzla@linux.microsoft.com/
> >>>

Re: rte_atomic_*_explicit

[Tyler Retzlaff]

On Fri, Jan 26, 2024 at 04:58:54PM +0000, Honnappa Nagarahalli wrote:
> <snip>
> 
> > >
> > >>
> > >> On Thu, Jan 25, 2024 at 11:10:47PM +0100, 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.
> > >>>
> > >>> 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.)
> > >>
> > >> yes, there was feedback at the time it was.
> > >>
> > >> * we should *not* have non-explicit versions of the macros
> > >> * the atomic generic functions should be named to match C11 standard
> > >>    with a rte_ prefix.
> > > This was mainly done to ensure that users think through the memory
> > ordering they want to use. This also matches with the compiler atomic built-
> > ins. Without explicit, it is sequentially consistent memory order.
> > >
> > 
> > "This" is referring to the first bullet only, correct?
> > 
> > You don't have to distinguish between implicit and explicit if you only have
> > explicit.
> Agree on your thought.
> 
> The '_explicit' was added to be aligned with the standard atomic API naming. The thought was - if we are aligned on the names, it needs less explanation for users.

well another benefit is that in the future if/when a decision is made to
directly use the names from the standard it is most trivial to just
strip the rte_ prefix tree wide.

Re: rte_atomic_*_explicit

[Tyler Retzlaff]

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.

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

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

Re: rte_atomic_*_explicit

[Mattias Rönnblom]

On 2024-01-26 11:52, 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.
> 

I think DPDK should have its own atomics API, with C11-style naming and 
semantics, where there is an overlap, and keep it until the heat death 
of the universe.

Is there an example of a large-scale project of the OS kernel or network 
stack kind that *doesn't* have its own atomics API (which wraps/extends 
C11 atomics, and/or uses compiler built-ins or in-line assembler)?

fd.io VPP, OpenDataPlane (ODP) and the Linux kernel all have custom APIs.

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

Re: rte_atomic_*_explicit

[Mattias Rönnblom]

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

Re: rte_atomic_*_explicit

[Tyler Retzlaff]

On Sat, Jan 27, 2024 at 09:34:24PM +0100, Mattias Rönnblom wrote:
> 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.

i can't predict the future, but one hopes gcc/clang/msvc eventually does
yes.

> * There's nothing add, subtract or clean up in the standard API to
> improve it for DPDK-internal and DPDK application code use.

i can't make judgement here, surely proposals to augment the standard
set may be made at any time on the mailing list.

> 
> 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);

it's difficult to extract what you're saying here. it sounds like

* you dislike the standard C generics names.
* you prefer the kernel atomics names.

the implication then is we should rename the macros to look like Linux
kernel atomics?

i suppose one of the pitfalls of this is now you have a set of names
where you have to document that they match a memory model layed out in
the standard but look like atomics that don't follow that model which
would also be confusing.

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

it achieves the intended purpose by introducing a reasonable level of
abstraction when choosing to enable or disable standard atomics whereby
the application code itself does not need to introduce its own conditional
compilation crutch.

it also retains the ability for applicaiton authors to continue to use gcc
built-in atomics when operating with dpdk apis should they decide the
gcc standard C atomics aren't up to scratch.

i guess your concern is that an application designer may choose to use
our rte_stdatomic.h beyond their interaction with the dpdk api, it's
their choice but the negative impact of doing so is minimal. the only
downside is that eventually we may choose to deprecate the macros which
is a risk that is shared by using any dpdk api.

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

Re: rte_atomic_*_explicit

[Mattias Rönnblom]

On 2024-01-30 19:36, Tyler Retzlaff wrote:
> On Sat, Jan 27, 2024 at 09:34:24PM +0100, Mattias Rönnblom wrote:
>> 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.
> 
> i can't predict the future, but one hopes gcc/clang/msvc eventually does
> yes.
> 
>> * There's nothing add, subtract or clean up in the standard API to
>> improve it for DPDK-internal and DPDK application code use.
> 
> i can't make judgement here, surely proposals to augment the standard
> set may be made at any time on the mailing list.
> 
>>
>> 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);
> 
> it's difficult to extract what you're saying here. it sounds like
> 
> * you dislike the standard C generics names.
> * you prefer the kernel atomics names.
> 

I tried to visualize, by example, how the use of <rte_stdatomic.h> 
results in verbose code, and how a better API could result in something 
much more succinct.

I guess you are trying to reduce it to something entirely subjective.

> the implication then is we should rename the macros to look like Linux
> kernel atomics?
> 

I can't say I have a fully formed opinion on what DPDK atomics should 
look like at this point.

Just taking C11 as-is seem not to be a good idea, and here I guess you 
agree, since you left out big chunks of the standard API in the wrapper.

> i suppose one of the pitfalls of this is now you have a set of names
> where you have to document that they match a memory model layed out in
> the standard but look like atomics that don't follow that model which
> would also be confusing.
> 

Sure, if one would have rte_atomic_inc() without any memory_model 
parameter, which would default to relaxed rather than CST, that would be 
a surprised to people with C11 atomics experience.

I wasn't saying I thought it was a good idea, but rather just something 
that should be considered. The majority of atomic operations in DPDK 
does not require any particular ordering (i.e., are relaxed), so in that 
sense it would make sense. However, in such a scenario, you are back 
something like <op>_explicit() to solve the 
no-function-overloading-problem the C11 standard had to deal with.

>>
>>>      * 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.
> 
> it achieves the intended purpose by introducing a reasonable level of
> abstraction when choosing to enable or disable standard atomics whereby
> the application code itself does not need to introduce its own conditional
> compilation crutch.
> 
> it also retains the ability for applicaiton authors to continue to use gcc
> built-in atomics when operating with dpdk apis should they decide the
> gcc standard C atomics aren't up to scratch.
> 
> i guess your concern is that an application designer may choose to use
> our rte_stdatomic.h beyond their interaction with the dpdk api, it's
> their choice but the negative impact of doing so is minimal. the only
> downside is that eventually we may choose to deprecate the macros which
> is a risk that is shared by using any dpdk api.
> 

My impression is that there basically are two types of applications; one 
that tries to minimize DPDK API exposure, using DPDK only for I/O. 
Another category use DPDK as "the" data plane framework, also leveraging 
non-driver/HAL type services, such as rings, atomics, timers, or 
anything else the full DPDK API has to offer.

Developers involved in applications of the latter category will go look 
in DPDK for examples and APIs, including on things like atomics.

If <rte_stdatomic.h> and/or <rte_atomic.h> aren't properly developed and 
maintained, it will be to the detriment of them.

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

RE: rte_atomic_*_explicit

[Morten Brørup]

> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Wednesday, 31 January 2024 16.53
> 
> On 2024-01-30 19:36, Tyler Retzlaff wrote:
> > On Sat, Jan 27, 2024 at 09:34:24PM +0100, Mattias Rönnblom wrote:
> >> 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.
> >
> > i can't predict the future, but one hopes gcc/clang/msvc eventually
> does
> > yes.
> >
> >> * There's nothing add, subtract or clean up in the standard API to
> >> improve it for DPDK-internal and DPDK application code use.
> >
> > i can't make judgement here, surely proposals to augment the standard
> > set may be made at any time on the mailing list.
> >
> >>
> >> 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);
> >
> > it's difficult to extract what you're saying here. it sounds like
> >
> > * you dislike the standard C generics names.
> > * you prefer the kernel atomics names.
> >
> 
> I tried to visualize, by example, how the use of <rte_stdatomic.h>
> results in verbose code, and how a better API could result in something
> much more succinct.
> 
> I guess you are trying to reduce it to something entirely subjective.
> 
> > the implication then is we should rename the macros to look like
> Linux
> > kernel atomics?
> >
> 
> I can't say I have a fully formed opinion on what DPDK atomics should
> look like at this point.
> 
> Just taking C11 as-is seem not to be a good idea, and here I guess you
> agree, since you left out big chunks of the standard API in the
> wrapper.
> 
> > i suppose one of the pitfalls of this is now you have a set of names
> > where you have to document that they match a memory model layed out
> in
> > the standard but look like atomics that don't follow that model which
> > would also be confusing.
> >
> 
> Sure, if one would have rte_atomic_inc() without any memory_model
> parameter, which would default to relaxed rather than CST, that would
> be
> a surprised to people with C11 atomics experience.
> 
> I wasn't saying I thought it was a good idea, but rather just something
> that should be considered. The majority of atomic operations in DPDK
> does not require any particular ordering (i.e., are relaxed), so in
> that
> sense it would make sense. However, in such a scenario, you are back
> something like <op>_explicit() to solve the
> no-function-overloading-problem the C11 standard had to deal with.

It is important to recall the back-then decision to always require memory order, as Honnappa mentioned. The reason is to force people to think about the optimum memory order, and be able to argue for it in the review process. This is also the reason why the functions with implicit memory order don't exist in <rte_stdatomic.h>.

> 
> >>
> >>>      * 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.
> >
> > it achieves the intended purpose by introducing a reasonable level of
> > abstraction when choosing to enable or disable standard atomics
> whereby
> > the application code itself does not need to introduce its own
> conditional
> > compilation crutch.
> >
> > it also retains the ability for applicaiton authors to continue to
> use gcc
> > built-in atomics when operating with dpdk apis should they decide the
> > gcc standard C atomics aren't up to scratch.
> >
> > i guess your concern is that an application designer may choose to
> use
> > our rte_stdatomic.h beyond their interaction with the dpdk api, it's
> > their choice but the negative impact of doing so is minimal. the only
> > downside is that eventually we may choose to deprecate the macros
> which
> > is a risk that is shared by using any dpdk api.
> >
> 
> My impression is that there basically are two types of applications;
> one
> that tries to minimize DPDK API exposure, using DPDK only for I/O.
> Another category use DPDK as "the" data plane framework, also
> leveraging
> non-driver/HAL type services, such as rings, atomics, timers, or
> anything else the full DPDK API has to offer.
> 
> Developers involved in applications of the latter category will go look
> in DPDK for examples and APIs, including on things like atomics.
> 
> If <rte_stdatomic.h> and/or <rte_atomic.h> aren't properly developed
> and
> maintained, it will be to the detriment of them.

I'm gradually being convinced that we could be better off considering <rte_stdatomic.h> non-temporary, so we can set it free to evolve, instead of considering it a temporary API on the path to C11 perfection, keeping it restricted to a certain subset of <stdatomic.h> for easy search-replace.

This would allow us to add convenience macros for popular use cases, such as rte_atomic_add_relaxed(), where the memory order is specified through the macro name. But rte_atomic_add() with some implicit memory order should still be prohibited, regardless how likely that implicit memory order is.

If we think it is not too confusing for C11 developers, we could also get rid of the _explicit postfix in the function names (while keeping the required memory_order parameter), to get shorter names.

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