Message ID | 1686087947-15471-1-git-send-email-roretzla@linux.microsoft.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id EA77C42C44; Tue, 6 Jun 2023 23:46:01 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CD90C42D0B; Tue, 6 Jun 2023 23:45:53 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id B8FB440223 for <dev@dpdk.org>; Tue, 6 Jun 2023 23:45:49 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id CDDAF20BE494; Tue, 6 Jun 2023 14:45:48 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com CDDAF20BE494 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1686087948; bh=kjJ5Vj4vPD4vNYDe8JMq/EwhPf1kHSAAUSkKLp+NO3k=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=C1wHhsRt1W3/Pv+lwNm/DraSpqnKxFSYcrKpFZKAyS1g9X8fu8i64lAQy6a58Ahws I3f2Gsc2WaV9qsB/OyfeAOmkwNVBKrUkNT3GsbuN2QE0TwbnA29OW7kC2quEDlmc+F mIf/Gs65j+CoX2E4Wo+8yxL/5osgQi9Fh6Ar2TrQ= From: Tyler Retzlaff <roretzla@linux.microsoft.com> To: dev@dpdk.org, david.marchand@redhat.com Cc: Olivier Matz <olivier.matz@6wind.com>, Bruce Richardson <bruce.richardson@intel.com>, Kevin Laatz <kevin.laatz@intel.com>, Qiming Yang <qiming.yang@intel.com>, Qi Zhang <qi.z.zhang@intel.com>, Wenjun Wu <wenjun1.wu@intel.com>, Tetsuya Mukawa <mtetsuyah@gmail.com>, Honnappa.Nagarahalli@arm.com, thomas@monjalon.net, Tyler Retzlaff <roretzla@linux.microsoft.com> Subject: [PATCH v5 0/6] replace rte atomics with GCC builtin atomics Date: Tue, 6 Jun 2023 14:45:41 -0700 Message-Id: <1686087947-15471-1-git-send-email-roretzla@linux.microsoft.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <1679084388-19267-1-git-send-email-roretzla@linux.microsoft.com> References: <1679084388-19267-1-git-send-email-roretzla@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org |
Series |
replace rte atomics with GCC builtin atomics
|
|
Message
Tyler Retzlaff
June 6, 2023, 9:45 p.m. UTC
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(-)
Comments
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. I am merging this series so we can progress on the $SUBJECT. Series applied, thanks. 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.
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
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 >
Hello Patrick, On Thu, Jun 22, 2023 at 10:00 PM Patrick Robb <probb@iol.unh.edu> 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. (reduced the cc list a bit) This is probably the same issue than what was reported by Intel validation: https://bugs.dpdk.org/show_bug.cgi?id=1249 A fix has been merged in next-net-intel, it will reach the main repo soon. https://git.dpdk.org/next/dpdk-next-net-intel/commit/?id=fe4ce0aee766969a0e27fe28ced8ee7c761a2c4e
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
On Fri, Jun 23, 2023 at 10:53:22AM +0200, David Marchand wrote: > Hello Patrick, > > On Thu, Jun 22, 2023 at 10:00 PM Patrick Robb <probb@iol.unh.edu> 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. > > (reduced the cc list a bit) > > This is probably the same issue than what was reported by Intel > validation: https://bugs.dpdk.org/show_bug.cgi?id=1249 > Thanks David I should have read the next thread in the mail chain before replying. > A fix has been merged in next-net-intel, it will reach the main repo soon. > https://git.dpdk.org/next/dpdk-next-net-intel/commit/?id=fe4ce0aee766969a0e27fe28ced8ee7c761a2c4e Patrick please let me know if after this integration I still need to investigate further. Thanks > > > -- > David Marchand
Thanks David, Tyler, I ran the next-net-intel branch through DTS with the nic utilizing the ixgbe driver, and everything is passing now. When this reaches the main repo I will return the nic in question to UNH CI testing. Best, Patrick On Fri, Jun 23, 2023 at 5:37 PM Tyler Retzlaff <roretzla@linux.microsoft.com> wrote: > On Fri, Jun 23, 2023 at 10:53:22AM +0200, David Marchand wrote: > > Hello Patrick, > > > > On Thu, Jun 22, 2023 at 10:00 PM Patrick Robb <probb@iol.unh.edu> 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. > > > > (reduced the cc list a bit) > > > > This is probably the same issue than what was reported by Intel > > validation: https://bugs.dpdk.org/show_bug.cgi?id=1249 > > > > Thanks David > > I should have read the next thread in the mail chain before replying. > > > A fix has been merged in next-net-intel, it will reach the main repo > soon. > > > https://git.dpdk.org/next/dpdk-next-net-intel/commit/?id=fe4ce0aee766969a0e27fe28ced8ee7c761a2c4e > > Patrick please let me know if after this integration I still need to > investigate further. > > Thanks > > > > > > > -- > > David Marchand >
On Wed, Jun 28, 2023 at 4:01 PM Patrick Robb <probb@iol.unh.edu> wrote: > > Thanks David, Tyler, > > I ran the next-net-intel branch through DTS with the nic utilizing the ixgbe driver, and everything is passing now. When this reaches the main repo I will return the nic in question to UNH CI testing. Thomas pulled the fix in main and tagged v23.07-rc2. At the time I write this mail, main, next-baseband, next-net, next-net-intel and next-virtio branches are fine. Other subtree repositories will get updated soon. Ping Ajit, Jerin, Raslan, Akhil for their respective subtrees. Thanks.