[dpdk-dev] atomic operations

[dpdk-dev] atomic operations

[Thomas Monjalon]

In the deprecation notices of DPDK 21.05, we can still read this:
"
* rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
  not allow for writing optimized code for all the CPU architectures supported
  in DPDK. DPDK will adopt C11 atomic operations semantics and provide wrappers
  using C11 atomic built-ins. These wrappers must be used for patches that
  need to be merged in 20.08 onwards. This change will not introduce any
  performance degradation.

* rte_smp_*mb: These APIs provide full barrier functionality. However, many
  use cases do not require full barriers. To support such use cases, DPDK will
  adopt C11 barrier semantics and provide wrappers using C11 atomic built-ins.
  These wrappers must be used for patches that need to be merged in 20.08
  onwards. This change will not introduce any performance degradation.
"

Should we keep these notifications forever?

It is very difficult to find which wrapper to use.

This is the guide we have:
https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#locks-and-atomic-operations
There are 2 blog posts:
https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/

Basically it says we should use "__atomic builtins" but there is example
for simple situations like counters, memory barriers, etc.
Please who could work on improving the documentation?

Re: [dpdk-dev] atomic operations

[Stephen Hemminger]

On Sat, 03 Jul 2021 13:29:38 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> In the deprecation notices of DPDK 21.05, we can still read this:
> "
> * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
>   not allow for writing optimized code for all the CPU architectures supported
>   in DPDK. DPDK will adopt C11 atomic operations semantics and provide wrappers
>   using C11 atomic built-ins. These wrappers must be used for patches that
>   need to be merged in 20.08 onwards. This change will not introduce any
>   performance degradation.
> 
> * rte_smp_*mb: These APIs provide full barrier functionality. However, many
>   use cases do not require full barriers. To support such use cases, DPDK will
>   adopt C11 barrier semantics and provide wrappers using C11 atomic built-ins.
>   These wrappers must be used for patches that need to be merged in 20.08
>   onwards. This change will not introduce any performance degradation.
> "
> 
> Should we keep these notifications forever?
> 
> It is very difficult to find which wrapper to use.
> 
> This is the guide we have:
> https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#locks-and-atomic-operations
> There are 2 blog posts:
> https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
> https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/
> 
> Basically it says we should use "__atomic builtins" but there is example
> for simple situations like counters, memory barriers, etc.
> Please who could work on improving the documentation?
> 
> 

Maybe there needs to be a DPDK version of the infamous kernel documentation
of memory barriers.

Re: [dpdk-dev] atomic operations

[Thomas Monjalon]

03/07/2021 13:29, Thomas Monjalon:
> In the deprecation notices of DPDK 21.05, we can still read this:
> "
> * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
>   not allow for writing optimized code for all the CPU architectures supported
>   in DPDK. DPDK will adopt C11 atomic operations semantics and provide wrappers
>   using C11 atomic built-ins. These wrappers must be used for patches that
>   need to be merged in 20.08 onwards. This change will not introduce any
>   performance degradation.
> 
> * rte_smp_*mb: These APIs provide full barrier functionality. However, many
>   use cases do not require full barriers. To support such use cases, DPDK will
>   adopt C11 barrier semantics and provide wrappers using C11 atomic built-ins.
>   These wrappers must be used for patches that need to be merged in 20.08
>   onwards. This change will not introduce any performance degradation.
> "

The only new wrapper is rte_atomic_thread_fence(). What else?
We are missing clear recommendations.

> Should we keep these notifications forever?
> 
> It is very difficult to find which wrapper to use.

We should make function names explicit instead of "These".

> This is the guide we have:
> https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#locks-and-atomic-operations
> There are 2 blog posts:
> https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
> https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/
> 
> Basically it says we should use "__atomic builtins" but there is example
> for simple situations like counters, memory barriers, etc.

Precision: I meant "there is *no* example".

> Please who could work on improving the documentation?

One simple example: increment a counter atomically.
__atomic_fetch_add(&counter, 1, __ATOMIC_RELAXED);
or
__atomic_add_fetch(&counter, 1, __ATOMIC_RELAXED);

Re: [dpdk-dev] atomic operations

[Thomas Monjalon]

03/07/2021 19:30, Stephen Hemminger:
> Thomas Monjalon <thomas@monjalon.net> wrote:
> > It is very difficult to find which wrapper to use.
> > 
> > This is the guide we have:
> > https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#locks-and-atomic-operations
> > There are 2 blog posts:
> > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
> > https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/
> > 
> > Basically it says we should use "__atomic builtins" but there is example
> > for simple situations like counters, memory barriers, etc.
> > Please who could work on improving the documentation?
> 
> Maybe there needs to be a DPDK version of the infamous kernel documentation
> of memory barriers.

No need to provide the whole theory. We can link to good resources
like the kernel documentation you mention, and insist on which function
we should use in DPDK, and explain why/when.

Re: [dpdk-dev] atomic operations

[Ruifeng Wang]

Removed invalid mail address.

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Sunday, July 4, 2021 8:38 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Phil Yang <Phil.Yang@arm.com>
> Cc: dev@dpdk.org; bruce.richardson@intel.com;
> konstantin.ananyev@intel.com; dev@dpdk.org;
> david.marchand@redhat.com; David Christensen <drc@linux.vnet.ibm.com>
> Subject: Re: [dpdk-dev] atomic operations
> 
> 03/07/2021 13:29, Thomas Monjalon:
> > In the deprecation notices of DPDK 21.05, we can still read this:
> > "
> > * rte_atomicNN_xxx: These APIs do not take memory order parameter.
> This does
> >   not allow for writing optimized code for all the CPU architectures
> supported
> >   in DPDK. DPDK will adopt C11 atomic operations semantics and provide
> wrappers
> >   using C11 atomic built-ins. These wrappers must be used for patches that
> >   need to be merged in 20.08 onwards. This change will not introduce any
> >   performance degradation.
> >
> > * rte_smp_*mb: These APIs provide full barrier functionality. However,
> many
> >   use cases do not require full barriers. To support such use cases, DPDK will
> >   adopt C11 barrier semantics and provide wrappers using C11 atomic built-
> ins.
> >   These wrappers must be used for patches that need to be merged in
> 20.08
> >   onwards. This change will not introduce any performance degradation.
> > "
> 
> The only new wrapper is rte_atomic_thread_fence(). What else?
Yes. The decision was to use GCC atomic built-ins directly. 
And rte_atomic_thread_fence() is an exception. It is a wrapper of __atomic_thread_fence(), because mem order __ATOMIC_SEQ_CST has an optimized implementation for x86.

> We are missing clear recommendations.
> 
> > Should we keep these notifications forever?
Targeting to obsolete APIs rte_atomicNN_xxx and rte_smp_*mb. 
Arm is working on replace occurrences with equivalent atomic built-ins.
There is still a lot work to do in drivers.

> >
> > It is very difficult to find which wrapper to use.
> 
> We should make function names explicit instead of "These".
> 
> > This is the guide we have:
> > https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#loc
> > ks-and-atomic-operations
> > There are 2 blog posts:
> > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-
> model/
> > https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/
> >
> > Basically it says we should use "__atomic builtins" but there is
> > example for simple situations like counters, memory barriers, etc.
> 
> Precision: I meant "there is *no* example".
> 
> > Please who could work on improving the documentation?
Agree that the documentation needs improve.
Add link to list of atomic built-ins and the above mentioned blog posts can be part of the improvement.

> 
> One simple example: increment a counter atomically.
> __atomic_fetch_add(&counter, 1, __ATOMIC_RELAXED); or
> __atomic_add_fetch(&counter, 1, __ATOMIC_RELAXED);
> 

Re: [dpdk-dev] atomic operations

[Thomas Monjalon]

05/07/2021 09:00, Ruifeng Wang:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 03/07/2021 13:29, Thomas Monjalon:
> > > In the deprecation notices of DPDK 21.05, we can still read this:
> > > "
> > > * rte_atomicNN_xxx: These APIs do not take memory order parameter.
> > This does
> > >   not allow for writing optimized code for all the CPU architectures
> > supported
> > >   in DPDK. DPDK will adopt C11 atomic operations semantics and provide
> > wrappers
> > >   using C11 atomic built-ins. These wrappers must be used for patches that
> > >   need to be merged in 20.08 onwards. This change will not introduce any
> > >   performance degradation.
> > >
> > > * rte_smp_*mb: These APIs provide full barrier functionality. However,
> > many
> > >   use cases do not require full barriers. To support such use cases, DPDK will
> > >   adopt C11 barrier semantics and provide wrappers using C11 atomic built-
> > ins.
> > >   These wrappers must be used for patches that need to be merged in
> > 20.08
> > >   onwards. This change will not introduce any performance degradation.
> > > "
> > 
> > The only new wrapper is rte_atomic_thread_fence(). What else?
> 
> Yes. The decision was to use GCC atomic built-ins directly. 
> And rte_atomic_thread_fence() is an exception. It is a wrapper of __atomic_thread_fence(), because mem order __ATOMIC_SEQ_CST has an optimized implementation for x86.

Then above deprecation is wrong.

> > We are missing clear recommendations.
> > 
> > > Should we keep these notifications forever?
> 
> Targeting to obsolete APIs rte_atomicNN_xxx and rte_smp_*mb. 
> Arm is working on replace occurrences with equivalent atomic built-ins.
> There is still a lot work to do in drivers.

This is an ongoing work.
In the meantime we need clear recommendation what to use.

> > > It is very difficult to find which wrapper to use.
> > 
> > We should make function names explicit instead of "These".
> > 
> > > This is the guide we have:
> > > https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#loc
> > > ks-and-atomic-operations
> > > There are 2 blog posts:
> > > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-
> > model/
> > > https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/
> > >
> > > Basically it says we should use "__atomic builtins" but there is
> > > example for simple situations like counters, memory barriers, etc.
> > 
> > Precision: I meant "there is *no* example".
> > 
> > > Please who could work on improving the documentation?
> 
> Agree that the documentation needs improve.
> Add link to list of atomic built-ins and the above mentioned blog posts can be part of the improvement.

It should be more than a link.
We need to know when to use what.

First thing, please fix the deprecation notice.

> > One simple example: increment a counter atomically.
> > __atomic_fetch_add(&counter, 1, __ATOMIC_RELAXED); or
> > __atomic_add_fetch(&counter, 1, __ATOMIC_RELAXED);

I really hate how atomics are "documented" in GCC doc.
For instance, it doesn't say what is returned (old or new value) in above functions.

Re: [dpdk-dev] atomic operations

[Ruifeng Wang]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, July 5, 2021 3:30 PM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>
> Cc: dev@dpdk.org; bruce.richardson@intel.com;
> konstantin.ananyev@intel.com; dev@dpdk.org;
> david.marchand@redhat.com; David Christensen <drc@linux.vnet.ibm.com>;
> nd <nd@arm.com>
> Subject: Re: [dpdk-dev] atomic operations
> 
> 05/07/2021 09:00, Ruifeng Wang:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 03/07/2021 13:29, Thomas Monjalon:
> > > > In the deprecation notices of DPDK 21.05, we can still read this:
> > > > "
> > > > * rte_atomicNN_xxx: These APIs do not take memory order parameter.
> > > This does
> > > >   not allow for writing optimized code for all the CPU
> > > > architectures
> > > supported
> > > >   in DPDK. DPDK will adopt C11 atomic operations semantics and
> > > > provide
> > > wrappers
> > > >   using C11 atomic built-ins. These wrappers must be used for patches
> that
> > > >   need to be merged in 20.08 onwards. This change will not introduce
> any
> > > >   performance degradation.
> > > >
> > > > * rte_smp_*mb: These APIs provide full barrier functionality.
> > > > However,
> > > many
> > > >   use cases do not require full barriers. To support such use cases, DPDK
> will
> > > >   adopt C11 barrier semantics and provide wrappers using C11
> > > > atomic built-
> > > ins.
> > > >   These wrappers must be used for patches that need to be merged
> > > > in
> > > 20.08
> > > >   onwards. This change will not introduce any performance degradation.
> > > > "
> > >
> > > The only new wrapper is rte_atomic_thread_fence(). What else?
> >
> > Yes. The decision was to use GCC atomic built-ins directly.
> > And rte_atomic_thread_fence() is an exception. It is a wrapper of
> __atomic_thread_fence(), because mem order __ATOMIC_SEQ_CST has an
> optimized implementation for x86.
> 
> Then above deprecation is wrong.
> 
> > > We are missing clear recommendations.
> > >
> > > > Should we keep these notifications forever?
> >
> > Targeting to obsolete APIs rte_atomicNN_xxx and rte_smp_*mb.
> > Arm is working on replace occurrences with equivalent atomic built-ins.
> > There is still a lot work to do in drivers.
> 
> This is an ongoing work.
> In the meantime we need clear recommendation what to use.
> 
> > > > It is very difficult to find which wrapper to use.
> > >
> > > We should make function names explicit instead of "These".
> > >
> > > > This is the guide we have:
> > > > https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html
> > > > #loc
> > > > ks-and-atomic-operations
> > > > There are 2 blog posts:
> > > > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-
> memory-
> > > model/
> > > > https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/
> > > >
> > > > Basically it says we should use "__atomic builtins" but there is
> > > > example for simple situations like counters, memory barriers, etc.
> > >
> > > Precision: I meant "there is *no* example".
> > >
> > > > Please who could work on improving the documentation?
> >
> > Agree that the documentation needs improve.
> > Add link to list of atomic built-ins and the above mentioned blog posts can
> be part of the improvement.
> 
> It should be more than a link.
> We need to know when to use what.
> 
> First thing, please fix the deprecation notice.
Sure. Will update deprecation notice with more accurate information.

> 
> > > One simple example: increment a counter atomically.
> > > __atomic_fetch_add(&counter, 1, __ATOMIC_RELAXED); or
> > > __atomic_add_fetch(&counter, 1, __ATOMIC_RELAXED);
> 
> I really hate how atomics are "documented" in GCC doc.
> For instance, it doesn't say what is returned (old or new value) in above
> functions.
> 
Returns are described as "the result of the operation" and "the value that had previously been in *ptr".
It can be more concise.

Re: [dpdk-dev] atomic operations

[Stephen Hemminger]

On Mon, 5 Jul 2021 08:33:21 +0000
Ruifeng Wang <Ruifeng.Wang@arm.com> wrote:

> >   
> > > > One simple example: increment a counter atomically.
> > > > __atomic_fetch_add(&counter, 1, __ATOMIC_RELAXED); or
> > > > __atomic_add_fetch(&counter, 1, __ATOMIC_RELAXED);  
> > 
> > I really hate how atomics are "documented" in GCC doc.
> > For instance, it doesn't say what is returned (old or new value) in above
> > functions.
> >   
> Returns are described as "the result of the operation" and "the value that had previously been in *ptr".
> It can be more concise.

Since English language parses left to right, the function name seem really obvious
to me. __atomic_fetch_add returns the result before the add, and __atomic_add_fetch returns
the result after the add.  The one non-obvious question is which way is faster if the
result is not being used?

Re: [dpdk-dev] atomic operations

[Honnappa Nagarahalli]

<snip>

> 
> In the deprecation notices of DPDK 21.05, we can still read this:
> "
> * rte_atomicNN_xxx: These APIs do not take memory order parameter. This
> does
>   not allow for writing optimized code for all the CPU architectures supported
>   in DPDK. DPDK will adopt C11 atomic operations semantics and provide
> wrappers
>   using C11 atomic built-ins. These wrappers must be used for patches that
>   need to be merged in 20.08 onwards. This change will not introduce any
>   performance degradation.
> 
> * rte_smp_*mb: These APIs provide full barrier functionality. However, many
>   use cases do not require full barriers. To support such use cases, DPDK will
>   adopt C11 barrier semantics and provide wrappers using C11 atomic built-ins.
>   These wrappers must be used for patches that need to be merged in 20.08
>   onwards. This change will not introduce any performance degradation.
> "
> 
> Should we keep these notifications forever?
I do not think we need to keep them forever (unless the precedence is to keep all the older deprecations).

> 
> It is very difficult to find which wrapper to use.
Actually, the deprecations are incorrect on the 'wrappers'.
When the deprecations were added, the understanding was we will develop wrappers, the discussion was not concluded. When we made the decision, we decided to use the C++11 atomic built-ins. Only wrapper developed was rte_atomic_thread_fence. This is documented in the following blog.

> 
> This is the guide we have:
> https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#locks-
> and-atomic-operations
> There are 2 blog posts:
> https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-
> model/
> https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/
> 
> Basically it says we should use "__atomic builtins" but there is example for
> simple situations like counters, memory barriers, etc.
> Please who could work on improving the documentation?
There is good amount of information on how to use the __atomic builtins for counters, memory barriers etc.
It would make sense to document something in DPDK if we implement our own wrappers.

> 

Re: [dpdk-dev] atomic operations

[Thomas Monjalon]

07/07/2021 21:04, Honnappa Nagarahalli:
> <snip>
> 
> > 
> > In the deprecation notices of DPDK 21.05, we can still read this:
> > "
> > * rte_atomicNN_xxx: These APIs do not take memory order parameter. This
> > does
> >   not allow for writing optimized code for all the CPU architectures supported
> >   in DPDK. DPDK will adopt C11 atomic operations semantics and provide
> > wrappers
> >   using C11 atomic built-ins. These wrappers must be used for patches that
> >   need to be merged in 20.08 onwards. This change will not introduce any
> >   performance degradation.
> > 
> > * rte_smp_*mb: These APIs provide full barrier functionality. However, many
> >   use cases do not require full barriers. To support such use cases, DPDK will
> >   adopt C11 barrier semantics and provide wrappers using C11 atomic built-ins.
> >   These wrappers must be used for patches that need to be merged in 20.08
> >   onwards. This change will not introduce any performance degradation.
> > "
> > 
> > Should we keep these notifications forever?
> I do not think we need to keep them forever (unless the precedence is to keep all the older deprecations).
> 
> > 
> > It is very difficult to find which wrapper to use.
> Actually, the deprecations are incorrect on the 'wrappers'.
> When the deprecations were added, the understanding was we will develop wrappers, the discussion was not concluded. When we made the decision, we decided to use the C++11 atomic built-ins. Only wrapper developed was rte_atomic_thread_fence. This is documented in the following blog.
> 
> > 
> > This is the guide we have:
> > https://doc.dpdk.org/guides/prog_guide/writing_efficient_code.html#locks-
> > and-atomic-operations
> > There are 2 blog posts:
> > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-
> > model/
> > https://www.dpdk.org/blog/2021/06/09/reader-writer-concurrency/
> > 
> > Basically it says we should use "__atomic builtins" but there is example for
> > simple situations like counters, memory barriers, etc.
> > Please who could work on improving the documentation?
> There is good amount of information on how to use the __atomic builtins for counters, memory barriers etc.
> It would make sense to document something in DPDK if we implement our own wrappers.

Please fix deprecation notice and think about
how to document the compiler builtin choice.