[PATCH v3 0/7] replace rte atomics with GCC builtin atomics

[Tyler Retzlaff]

Morten,

David and Honnappa are discussing the /* NOTE: */ comments that were
added. If the three of you could come to conclusion about keeping or
removing them it would be appreciated.

Thanks!

On Wed, May 24, 2023 at 10:56:01PM +0000, Honnappa Nagarahalli wrote:
> 
> 
> > -----Original Message-----
> > From: Tyler Retzlaff <roretzla at linux.microsoft.com>
> > Sent: Wednesday, May 24, 2023 5:51 PM
> > To: David Marchand <david.marchand at redhat.com>
> > Cc: dev at dpdk.org; Honnappa Nagarahalli <Honnappa.Nagarahalli at arm.com>;
> > Ruifeng Wang <Ruifeng.Wang at arm.com>; thomas at monjalon.net;
> > stephen at networkplumber.org; mb at smartsharesystems.com; Ferruh Yigit
> > <ferruh.yigit at amd.com>
> > Subject: Re: [PATCH v3 0/7] replace rte atomics with GCC builtin atomics
> > 
> > On Wed, May 24, 2023 at 10:06:24PM +0200, David Marchand wrote:
> > > On Wed, May 24, 2023 at 5:47 PM Tyler Retzlaff
> > > <roretzla at linux.microsoft.com> wrote:
> > > > On Wed, May 24, 2023 at 02:40:43PM +0200, David Marchand wrote:
> > > > > Hello Tyler,
> > > > >
> > > > > On Thu, Mar 23, 2023 at 11:54 PM Tyler Retzlaff
> > > > > <roretzla at linux.microsoft.com> wrote:
> > > > > >
> > > > > > Replace the use of rte_atomic.h types and functions, instead use
> > > > > > GCC supplied C++11 memory model builtins.
> > > > > >
> > > > > > This series covers the libraries and drivers that are built on Windows.
> > > > > >
> > > > > > The code has be converted to use the __atomic builtins but there
> > > > > > are additional during conversion i notice that there may be some
> > > > > > issues that need to be addressed.
> > > > > >
> > > > > > I'll comment in the patches where my concerns are so the
> > > > > > maintainers may comment.
> > > > > >
> > > > > > v3:
> > > > > >   * style, don't use c99 comments
> > > > > >
> > > > > > v2:
> > > > > >   * comment code where optimizations may be possible now that
> > memory
> > > > > >     order can be specified.
> > > > > >   * comment code where operations should potentially be atomic so that
> > > > > >     maintainers can review.
> > > > > >   * change a couple of variables labeled as counters to be unsigned.
> > > > > >
> > > > > > Tyler Retzlaff (7):
> > > > > >   ring: replace rte atomics with GCC builtin atomics
> > > > > >   stack: replace rte atomics with GCC builtin atomics
> > > > > >   dma/idxd: replace rte atomics with GCC builtin atomics
> > > > > >   net/ice: replace rte atomics with GCC builtin atomics
> > > > > >   net/ixgbe: replace rte atomics with GCC builtin atomics
> > > > > >   net/null: replace rte atomics with GCC builtin atomics
> > > > > >   net/ring: replace rte atomics with GCC builtin atomics
> > > > > >
> > > > > >  drivers/dma/idxd/idxd_internal.h |  3 +--
> > > > > >  drivers/dma/idxd/idxd_pci.c      |  8 +++++---
> > > > > >  drivers/net/ice/ice_dcf.c        |  1 -
> > > > > >  drivers/net/ice/ice_dcf_ethdev.c |  1 -
> > > > > >  drivers/net/ice/ice_ethdev.c     | 12 ++++++++----
> > > > > >  drivers/net/ixgbe/ixgbe_bypass.c |  1 -
> > > > > > drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++------
> > > > > > drivers/net/ixgbe/ixgbe_ethdev.h |  3 ++-
> > > > > >  drivers/net/ixgbe/ixgbe_flow.c   |  1 -
> > > > > >  drivers/net/ixgbe/ixgbe_rxtx.c   |  1 -
> > > > > >  drivers/net/null/rte_eth_null.c  | 28
> > > > > > ++++++++++++++++++----------  drivers/net/ring/rte_eth_ring.c  | 26
> > ++++++++++++++++----------
> > > > > >  lib/ring/rte_ring_core.h         |  1 -
> > > > > >  lib/ring/rte_ring_generic_pvt.h  | 12 ++++++++----
> > > > > > lib/stack/rte_stack_lf_generic.h | 16 +++++++++-------
> > > > > >  15 files changed, 79 insertions(+), 53 deletions(-)
> > > > > >
> > > > >
> > > > > There is still some code using the DPDK "legacy" atomic API, but I
> > > > > guess this will be converted later.
> > > >
> > > > Yes, it will be converted later.
> > > >
> > > > If I did it correctly... the series was an attempt to move away from
> > > > the legacy API where there was a dependency on EAL that would change
> > > > when moving to stdatomic. I'm hoping that the remaining use of the
> > > > legacy API are not sensitive to the theoretical ABI surface changing
> > > > when that move is complete.
> > >
> > > Ok.
> > >
> > >
> > > > > As you proposed, I dropped patch 1 on the ring library (waiting
> > > > > for ARM to provide an alternative) and applied this series, thanks.
> > > > >
> > > > > Note: Thomas, Ferruh, we will have to be careful when merging
> > > > > subtrees to make sure we are not reintroducing those again (like
> > > > > for example net/ice).
> > >
> > > Well, I have some second thought about this series so I did not push
> > > it to dpdk.org yet.
> > 
> > Understood. It's very important to have these reviewed well so no objection just
> > hope we can get them reviewed properly soon.
> > 
> > > Drivers maintainers were not copied so I would like another pair of
> > > eyes on the series: ideally no /* Note: */ should be left when merging
> > > those patches.
> > 
> > The /* Note: */ was explicitly requested by other reviewers as they were
> > concerned we would lose track of opportunities to weaken ordering after
> > switching from __sync to __atomic.
> Note that some of the changes that I checked are in control plane. While it is good to optimize those, but the benefits might not be much. The presence of SEQ_CST also can act as a note.
> 
> > 
> > Is your request that the comments now be removed?
> > 
> > Thanks!
> > 
> > > I'll reply individually on the patches.
> > >
> > >
> > > --
> > > David Marchand