Re: [dpdk-dev] atomic operations

[Ruifeng Wang <Ruifeng.Wang@arm.com>]

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