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 5016D430D3; Tue, 22 Aug 2023 20:14:36 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EBFC040689; Tue, 22 Aug 2023 20:14:35 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 10B674021D; Tue, 22 Aug 2023 20:14:34 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 019CE2126CD0; Tue, 22 Aug 2023 11:14:32 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 019CE2126CD0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1692728073; bh=h7E96RFMt5jTmtMi9BG2UtHCuaTRHbXMd9hCdMNYhvo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=PDdxg5rB3yCo7w/AJVpRi6d9s33fO2MZNGGosdEk23HxWLUzzCDHV24rGQpdBwOoD 50QGNFV5N+2okoOVxWhlKXN66SRTYnJ7zOdbPbDxcmixgY24i4RnYYb9K+YUlvJ5zk J6LV44lc8nsn3ulHF9uxo9UgfkaFfTcjGUz3juyI= Date: Tue, 22 Aug 2023 11:14:32 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Bruce Richardson , dev@dpdk.org, techboard@dpdk.org, Honnappa Nagarahalli , Ruifeng Wang , Jerin Jacob , Sunil Kumar Kori , Mattias =?iso-8859-1?Q?R=F6nnblom?= , Joyce Kong , David Christensen , Konstantin Ananyev , David Hunt , Thomas Monjalon , David Marchand Subject: Re: [PATCH v4 6/6] devtools: forbid new direct use of GCC atomic builtins Message-ID: <20230822181432.GA7299@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1691717521-1025-1-git-send-email-roretzla@linux.microsoft.com> <1692221931-32334-1-git-send-email-roretzla@linux.microsoft.com> <1692221931-32334-7-git-send-email-roretzla@linux.microsoft.com> <98CBD80474FA8B44BF855DF32C47DC35D87B16@smartserver.smartshare.dk> <20230817191408.GB12307@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D87B1A@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87B1A@smartserver.smartshare.dk> 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, Aug 18, 2023 at 09:13:29AM +0200, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Thursday, 17 August 2023 21.14 > > > > On Thu, Aug 17, 2023 at 01:57:01PM +0200, Morten Brørup wrote: > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > > > Sent: Wednesday, 16 August 2023 23.39 > > > > > > > > Refrain from using compiler __atomic_xxx builtins DPDK now requires > > > > the use of rte_atomic__explicit macros when operating on DPDK > > > > atomic variables. > > > > > > There is probably no end to how much can be added to checkpatches. > > > > > > You got the important stuff, so below are only further suggestions! > > > > > > [...] > > > > > > > - # refrain from using compiler __atomic_{add,and,nand,or,sub,xor}_fetch() > > > > + # refrain from using compiler __atomic_xxx builtins > > > > awk -v FOLDERS="lib drivers app examples" \ > > > > - -v EXPRESSIONS="__atomic_(add|and|nand|or|sub|xor)_fetch\\\(" \ > > > > + -v EXPRESSIONS="__atomic_.*\\\(" \ > > > > -v RET_ON_FAIL=1 \ > > > > - -v MESSAGE='Using __atomic_op_fetch, prefer __atomic_fetch_op' \ > > > > + -v MESSAGE='Using __atomic_xxx builtins' \ > > > > > > Alternatively: > > > -v MESSAGE='Using __atomic_xxx built-ins, prefer rte_atomic_xxx' > > \ > > > > i can adjust the wording as you suggest, no problem > > > > > > > > > -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \ > > > > "$1" || res=1 > > > > > > > > -- > > > > 1.8.3.1 > > > > > > This could be updated too: > > > > > > # refrain from using compiler __atomic_thread_fence() > > > # It should be avoided on x86 for SMP case. > > > awk -v FOLDERS="lib drivers app examples" \ > > > -v EXPRESSIONS="__atomic_thread_fence\\\(" \ > > > -v RET_ON_FAIL=1 \ > > > - -v MESSAGE='Using __atomic_thread_fence' \ > > > + -v MESSAGE='Using __atomic_thread_fence built-in, prefer > > __rte_atomic_thread_fence' \ > > > -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \ > > > > yeah, i left this one separate i think the advice is actually use > > rte_atomic_thread_fence which may be an inline function that uses > > __rte_atomic_thread_fence > > I now noticed that the comment to this says "# [...] should be avoided on x86 for SMP case." Wouldn't that apply to __rte_atomic_thread_fence too? So would we want a similar warning for __rte_atomic_thread_fence in checkpatches; i.e. warnings for all variants of [__[rte_]]atomic_thread_fence? to understand how this applies we need to look at x86/include/rte_atomic.h static __rte_always_inline void rte_atomic_thread_fence(rte_memory_order memorder) { if (memorder == rte_memory_order_seq_cst) rte_smp_mb(); else __rte_atomic_thread_fence(memorder); } So what i've done is this You *should* always use rte_atomic_thread_fence() because it does the dance to give you what you are supposed to on "x86 for the SMP case" right? > > If the use of [__[rte_]]atomic_thread_fence is only conditionally prohibited, I think that we should move the warning to the definition of __rte_atomic_thread_fence itself, gated by x64 and SMP being defined. The CI would catch its use in DPDK, but still allow application developers to use it (for targets not being x86 SMP). What do others think? I'll tweak the patch to warn about __rte_atomic and __atomic since the correct usage is always using rte_atomic_xxx > > > > > > "$1" || res=1 > > > > > > You could also add C11 variants of these tests... > > > > > atomic_(load|store|exchange|compare_exchange_(strong|weak)|fetch_(add|sub|and| > > xor|or|nand)|flag_(test_and_set|clear))[_explicit], and > > > atomic_thread_fence. > > > > > > And a test for using "_Atomic". > > > > direct use would cause early compilation in the CI so it would be caught > > fairly early. i'm not sure i want to get into the business of trying to > > add redundant (albiet cheaper) earlier checks. > > > > though if there is a general call for this from the reviewers i'll add > > them.