From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 39AC142D34; Fri, 23 Jun 2023 23:35:48 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BBF4140EE4; Fri, 23 Jun 2023 23:35:47 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id A908240ED7 for ; Fri, 23 Jun 2023 23:35:45 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id A81CB21C253E; Fri, 23 Jun 2023 14:35:44 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com A81CB21C253E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1687556144; bh=UL2YdEMgdPporvDuqFblQz6vNFin3+em1NWdTYg3UZI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=sZbskCNwDxT39GpIbb4lVIk64yedUhMln3c4HoH/DisFbimM9SQsVoVVNvUHS7s3E n7tcN7L3dSMH+eDONpWK0UHmEJhKuUORih48kObyRaSNGd7fDJFQfqrq7VatbMxt26 HheiWXMieWlJ4eu/XP4p5jiKS2SAz7b2D93aK9BQ= Date: Fri, 23 Jun 2023 14:35:44 -0700 From: Tyler Retzlaff To: Patrick Robb Cc: David Marchand , dev@dpdk.org, Olivier Matz , Bruce Richardson , Kevin Laatz , Qiming Yang , Qi Zhang , Wenjun Wu , Tetsuya Mukawa , Honnappa.Nagarahalli@arm.com, thomas@monjalon.net Subject: Re: [PATCH v5 0/6] replace rte atomics with GCC builtin atomics Message-ID: <20230623213544.GA14396@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1679084388-19267-1-git-send-email-roretzla@linux.microsoft.com> <1686087947-15471-1-git-send-email-roretzla@linux.microsoft.com> <20230609151320.GA1770@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > 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 > > > 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