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 CF12A42B92; Thu, 25 May 2023 02:02:42 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 91DC040FDF; Thu, 25 May 2023 02:02:42 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id E900B40A82 for ; Thu, 25 May 2023 02:02:40 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id D6D1520FBA78; Wed, 24 May 2023 17:02:39 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com D6D1520FBA78 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1684972959; bh=HWBu2jgqK3a1OnUT8u1K9CqP6IaA8z77nkmGsTruIYM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fqBM38HtS6YlHG7WNszhCPD2d5j/PQff3XNGtBxI8dDCNmOv885CfHV05gpAyVacY v+7LO4lN5zcP93EyNHSzsSNupvNVkjyy+44Pl+DjNdtOdd9I4E6x8wTbtvXJTT6/K2 myzwZ7mk7zvbrtdwSUOHHAFuhJqe9l35Qd59lqhU= Date: Wed, 24 May 2023 17:02:39 -0700 From: Tyler Retzlaff To: Honnappa Nagarahalli Cc: David Marchand , "dev@dpdk.org" , Ruifeng Wang , "thomas@monjalon.net" , "stephen@networkplumber.org" , "mb@smartsharesystems.com" , Ferruh Yigit , nd Subject: Re: [PATCH v3 0/7] replace rte atomics with GCC builtin atomics Message-ID: <20230525000239.GB5524@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1679084388-19267-1-git-send-email-roretzla@linux.microsoft.com> <1679612036-30773-1-git-send-email-roretzla@linux.microsoft.com> <20230524154701.GA7766@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <20230524225035.GA5524@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 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: > > > > -----Original Message----- > > From: Tyler Retzlaff > > Sent: Wednesday, May 24, 2023 5:51 PM > > To: David Marchand > > Cc: dev@dpdk.org; Honnappa Nagarahalli ; > > Ruifeng Wang ; thomas@monjalon.net; > > stephen@networkplumber.org; mb@smartsharesystems.com; Ferruh Yigit > > > > Subject: Re: [PATCH v3 0/7] replace rte atomics with GCC builtin atomics > > > > On Wed, May 24, 2023 at 10:06:24PM +0200, David Marchand wrote: > > > On Wed, May 24, 2023 at 5:47 PM Tyler Retzlaff > > > 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 > > > > > 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. > > > > > > > > > > > > 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 (7): > > > > > > ring: replace rte atomics with GCC builtin atomics > > > > > > 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 | 8 +++++--- > > > > > > 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 | 26 > > ++++++++++++++++---------- > > > > > > lib/ring/rte_ring_core.h | 1 - > > > > > > lib/ring/rte_ring_generic_pvt.h | 12 ++++++++---- > > > > > > lib/stack/rte_stack_lf_generic.h | 16 +++++++++------- > > > > > > 15 files changed, 79 insertions(+), 53 deletions(-) > > > > > > > > > > > > > > > > There is still some code using the DPDK "legacy" atomic API, but I > > > > > guess this will be converted later. > > > > > > > > Yes, it will be converted later. > > > > > > > > If I did it correctly... the series was an attempt to move away from > > > > the legacy API where there was a dependency on EAL that would change > > > > when moving to stdatomic. I'm hoping that the remaining use of the > > > > legacy API are not sensitive to the theoretical ABI surface changing > > > > when that move is complete. > > > > > > Ok. > > > > > > > > > > > As you proposed, I dropped patch 1 on the ring library (waiting > > > > > for ARM to provide an alternative) and applied this series, thanks. > > > > > > > > > > Note: Thomas, Ferruh, we will have to be careful when merging > > > > > subtrees to make sure we are not reintroducing those again (like > > > > > for example net/ice). > > > > > > 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. > 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. > > > > > Is your request that the comments now be removed? > > > > Thanks! > > > > > I'll reply individually on the patches. > > > > > > > > > -- > > > David Marchand