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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> Sent: Friday, 17 March 2023 22.49
> 
> On Fri, Mar 17, 2023 at 02:42:26PM -0700, Stephen Hemminger wrote:
> > On Fri, 17 Mar 2023 13:19:41 -0700
> > 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 don't think all these cmpset need to use SEQ_CST.
> > Especially for the places where it is used a loop, might
> > be more efficient with some of the other memory models.
> 
> i agree.
> 
> however, i'm not trying to improve the code with this change, just
> decouple it from rte_atomics.h so trying my best to avoid any
> unnecessary semantic change.
> 
> certainly if the maintainers of this code wish to weaken the ordering
> where appropriate after the change is merged they can do so and handily
> this change has enabled them to do so easily allowing them to test just
> their change in isolation.

I agree with the two-step approach, where this first step is a simple search-and-replacement; but I insist that you add a FIXME or similar note where you have blindly used SEQ_CST, indicating that the memory order needs to be reviewed and potentially corrected.

Also, in a couple of the drivers, you are using int64_t for packet counters. These cannot be negative and should be uint64_t. And AFAIK, such counters can use RELAXED memory order.