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

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

Hi Patrick,

I will take a look at this as a priority asap.

Thanks for bringing it to my attention.

On Thu, Jun 22, 2023 at 03:59:42PM -0400, Patrick Robb wrote:
> I want to report a possible regression from this patch series seen from CI
> testing on our Intel 82599ES 10G NIC, which we failed to report to
> patchwork when this initially went under CI due to a bug in our Jenkins
> reporting scripts. Use of the ixgbe driver appears to be affected. Tyler I
> apologize for the issues seen with reporting. We've made some temporary
> changes to avoid this happening again, and are currently reworking our
> reporting process entirely to provide greater reliability.
> 
> Here is a DTS snippet showing the issue, and the full log for the
> failing virtio_smoke test can be downloaded here:
> https://dpdkdashboard.iol.unh.edu/results/dashboard/patchsets/26560/
> 
> 06/06/2023 18:22:58                TestVirtioSmoke: Start send packets and
> verify
> 06/06/2023 18:22:58                         tester: ifconfig enp134s0f0 mtu
> 9000
> 06/06/2023 18:22:58                         tester:
> 06/06/2023 18:42:59                TestVirtioSmoke: Test Case
> test_virtio_pvp Result FAILED: TIMEOUT on port start 0
> 06/06/2023 18:42:59                TestVirtioSmoke: port start 0
> 
> ixgbe_dev_wait_setup_link_complete(): IXGBE link thread not complete too
> long time!
> ixgbe_dev_wait_setup_link_complete(): IXGBE link thread not complete too
> long time!
> ixgbe_dev_wait_setup_link_complete(): IXGBE link thread not complete too
> long time!
> 
> We initially took this Intel10G testing offline to investigate as we
> thought it was a lab infra failure. Obviously that wasn't the case, so
> ideally we will bring this back online when appropriate. But, I don't want
> to do so right now and start failing everyone's patchseries which are
> obviously unrelated to this. Comments on this are welcome, otherwise of
> course I will just return this test coverage to our CI when the state of
> the git tree allows for it.
> 
> Apologies for the missing report and the timeline on this. We are taking
> action to deliver results more reliably going forward.
> 
> 
> On Fri, Jun 9, 2023 at 11:13 AM Tyler Retzlaff <roretzla@linux.microsoft.com>
> wrote:
> 
> > On Fri, Jun 09, 2023 at 05:01:53PM +0200, David Marchand wrote:
> > > On Tue, Jun 6, 2023 at 11:45 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.
> > > >
> > > > v5:
> > > >   * use relaxed ordering for counter increments in net/ring patch
> > > >   * remove note comments from net/ring patch
> > > >
> > > > v4:
> > > >
> > > >   * drop patch for lib/ring it will be provided by ARM / Honnappa
> > > >   * rebase for changes in dma/idxd merge
> > > >   * adapt __atomic_fetch_sub(...) - 1 == 0 to be
> > (__atomic_fetch_sub(...) == 1)
> > > >     as per feedback.
> > > >   * drop one /* NOTE: review for potential ordering optimization */
> > since
> > > >     the note reference non-critical to perf control path.
> > > >
> > > >   note:
> > > >
> > > >   Remainder of the NOTE comments have been retained since there
> > > >   seems to be no consensus but stronger opinion/argument to keep
> > > >   expressed. while I generally agree that changes should not
> > > >   include ``TODO'' style comments I also agree that without these
> > > >   comments in your face people are very unlikely to feel compelled
> > > >   to make the review they are trying to solicit without them. if
> > > >   it is absolute that the series won't be merged with them then I
> > > >   will remove them, but please be explicit soon.
> > > >
> > > > 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 (6):
> > > >   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      | 11 ++++++-----
> > > >  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  | 20 ++++++++++----------
> > > >  lib/stack/rte_stack_lf_generic.h | 16 +++++++++-------
> > > >  13 files changed, 66 insertions(+), 50 deletions(-)
> > >
> > > I am not really enthousiastic about those NOTE:.
> > > I would prefer we get an explicit go/nogo from each maintainers, but
> > > this did not happen.
> > > I think that this indicates that those NOTE: will rot in the code now.
> > >
> > > Thomas proposed to track those NOTE: in the release announce mail and
> > > that we ping maintainers regularly.
> > > Let's see how it goes.
> >
> > Let's leave it for one release cycle, if with the the announce mail
> > maintainers take no action within that time I'll commit to going
> > through and cleaning them out before 23.11 rc1.
> >
> > >
> > > I am merging this series so we can progress on the $SUBJECT.
> > > Series applied, thanks.
> >
> > Thanks David, this will allow forward progress.
> >
> > >
> > >
> > > Tyler, about the patch on the ring library that was dropped by got no
> > > viable alternative, I'll wait for a decision from ARM and you.
> >
> > I'll wait for Honnappa to follow up and we'll decide what to do when he
> > does.
> >
> > >
> > > --
> > > David Marchand
> >
> 
> 
> -- 
> 
> Patrick Robb
> 
> Technical Service Manager
> 
> UNH InterOperability Laboratory
> 
> 21 Madbury Rd, Suite 100, Durham, NH 03824
> 
> www.iol.unh.edu