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

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

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Thursday, 23 March 2023 23.54
> 
> 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.
> 

I didn't see the v3 when ack'ing the v2, so in case v2 is quickly skipped by maintainers...

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
  

On Fri, Mar 24, 2023 at 08:09:50AM +0100, Morten Brørup wrote:
> > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > Sent: Thursday, 23 March 2023 23.54
> > 
> > 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.
> > 
> 
> I didn't see the v3 when ack'ing the v2, so in case v2 is quickly skipped by maintainers...

yeah, my fault. i hammed up the comment style used and needed to quickly
submit v3 to satisfy checkpatches.

thanks!

> 
> Series-acked-by: Morten Brørup <mb@smartsharesystems.com>
  

Hello Tyler,

On Thu, Mar 23, 2023 at 11:54 PM Tyler Retzlaff
<roretzla@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.

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

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

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

On Wed, May 24, 2023 at 5:47 PM Tyler Retzlaff
<roretzla@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@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.
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.
I'll reply individually on the patches.
  

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

Is your request that the comments now be removed?

Thanks!

> I'll reply individually on the patches.
> 
> 
> -- 
> David Marchand
  

> -----Original Message-----
> From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Sent: Wednesday, May 24, 2023 5:51 PM
> To: David Marchand <david.marchand@redhat.com>
> Cc: dev@dpdk.org; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> stephen@networkplumber.org; mb@smartsharesystems.com; Ferruh Yigit
> <ferruh.yigit@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@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@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
  

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@linux.microsoft.com>
> > Sent: Wednesday, May 24, 2023 5:51 PM
> > To: David Marchand <david.marchand@redhat.com>
> > Cc: dev@dpdk.org; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> > Ruifeng Wang <Ruifeng.Wang@arm.com>; thomas@monjalon.net;
> > stephen@networkplumber.org; mb@smartsharesystems.com; Ferruh Yigit
> > <ferruh.yigit@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@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@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
  

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Thursday, 25 May 2023 02.03
> 
> 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:
> >
> > > From: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > Sent: Wednesday, May 24, 2023 5:51 PM
> > >
> > > 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@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@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.

[...]

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

This patch series is an important step towards the more flexible C11 atomics, and I consider further optimization "nice to have", not "must have".

So I don't think we should hold back these patches and require of the maintainers to optimize the atomic accesses before. I would rather leave the notes in the code, so they can be optimized by anyone with the required skills and/or testing facilities at a later time.

I agree that it would be ideal if anyone (e.g. the maintainers) can make optimize the affected libraries/drivers in time for the coming release, but they can be separate patches after this series.

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

I vote against using SEQ_CST as a note. SEQ_CST might be the correct memory order in some locations, so it would require a note in those locations that SEQ_CST has been reviewed and is the optimal memory order there. I would rather have notes where we know that further consideration for optimization is warranted.

If atomics are used in the control plane, the memory order still need to be correct (i.e. not causing failure, which SEQ_CST should assure). So the note should remain, if not reviewed for optimization. A reviewer can add to the note that this is control plane only, so optimization is not important. Alternatively, if those control plane variables don't need to be atomics, they can be replaced by non-atomic types and accesses - such a modification can also be considered an optimization.

PS: If someone spotted an opportunity for optimization anywhere in DPDK, but was unable to implement and/or test it himself, adding a note about it in the source code could be an alternative. On the other hand, such ideas might belong in Bugzilla instead... (Just arguing for keeping the notes. Not trying to broaden the discussion!)