> From: Tyler Retzlaff [mailto:roret...@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 <roret...@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.
