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 3C13D42C6F; Fri, 9 Jun 2023 17:13:24 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1BF7442FC4; Fri, 9 Jun 2023 17:13:24 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id A840442D8F for ; Fri, 9 Jun 2023 17:13:21 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id B437A20C147C; Fri, 9 Jun 2023 08:13:20 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com B437A20C147C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1686323600; bh=DJLLwHNIlApwHHK2tWz1qjw9urhePSZdXOoDReNo5ck=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ASEh9pTCs0T4jXoUcolR5BJTbbcWgibB6Pw0j8swvP/vXtXuaEM7v0NZ6jidmtSxy +4CGQM5kaKt7PDQe/GUtT7FI0ZBErG18tvHJb2ADO6DAKKNScb19nj3YapgofhB9iU 2cx0ccF+tlKaLR6iQs9ERS59muroyZwTo5dCuwlM= Date: Fri, 9 Jun 2023 08:13:20 -0700 From: Tyler Retzlaff To: David Marchand Cc: 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: <20230609151320.GA1770@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> 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 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