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

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

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