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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Wed, Mar 22, 2023 at 12:28:44PM +0100, Morten Brørup wrote:
> > 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.

i think the maintainers need to take some responsibility, if they see
optimizations they missed when previously writing the code they need to
follow up with a patch themselves. i can't do everything for them and
marking things i'm not sure about will only lead to me having to churn
patch series to remove the unwanted comments later.

keep in mind i have to touch each of these again when converting to
standard so that's a better time to review ~everything in more detail
because when converting to standard that's when suddenly you get a bunch
of code generation that is "fallback" to seq_cst that isn't happening now.

the series that converts to standard needs to be up for review as soon
as possible to maximize available time for feedback before 23.11 so it
would be better to get the simpler cut & paste normalizing the code out
of the way to unblock that series submission.

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

i know you don't mean to say i selected the types and rather that the
types that were selected are not quite correct for their usage. again
on the review that actually adopts std atomics is a better place to make
any potential type changes since we are "breaking" the API for 23.11
anyway. further, the std atomics series technically changes all the
types so it's probably better to make one type change then rather than
one now and one later.

i think it would be best to get these validated and merged asap so we
can get to the std atomics review. when that series is up let's discuss
further how i can mark areas of concern, with that series i expect there
will have to be some changes in order to avoid minor regressions.

thanks!