From: Tyler Retzlaff <roretzla@linux.microsoft.com>
To: "Morten Brørup" <mb@smartsharesystems.com>
Cc: "Bruce Richardson" <bruce.richardson@intel.com>,
dev@dpdk.org, techboard@dpdk.org,
"Honnappa Nagarahalli" <honnappa.nagarahalli@arm.com>,
"Ruifeng Wang" <ruifeng.wang@arm.com>,
"Jerin Jacob" <jerinj@marvell.com>,
"Sunil Kumar Kori" <skori@marvell.com>,
"Mattias Rönnblom" <mattias.ronnblom@ericsson.com>,
"Joyce Kong" <joyce.kong@arm.com>,
"David Christensen" <drc@linux.vnet.ibm.com>,
"Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru>,
"David Hunt" <david.hunt@intel.com>,
"Thomas Monjalon" <thomas@monjalon.net>,
"David Marchand" <david.marchand@redhat.com>
Subject: Re: [PATCH v4 6/6] devtools: forbid new direct use of GCC atomic builtins
Date: Tue, 22 Aug 2023 11:14:32 -0700 [thread overview]
Message-ID: <20230822181432.GA7299@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87B1A@smartserver.smartshare.dk>
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_<op>_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.
next prev parent reply other threads:[~2023-08-22 18:14 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-11 1:31 [PATCH 0/6] RFC optional rte optional stdatomics API Tyler Retzlaff
2023-08-11 1:31 ` [PATCH 1/6] eal: provide rte stdatomics optional atomics API Tyler Retzlaff
2023-08-11 8:56 ` Bruce Richardson
2023-08-11 9:42 ` Morten Brørup
2023-08-11 15:54 ` Tyler Retzlaff
2023-08-14 9:04 ` Morten Brørup
2023-08-11 1:31 ` [PATCH 2/6] eal: adapt EAL to present rte " Tyler Retzlaff
2023-08-11 1:31 ` [PATCH 3/6] eal: add rte atomic qualifier with casts Tyler Retzlaff
2023-08-11 1:31 ` [PATCH 4/6] distributor: adapt for EAL optional atomics API changes Tyler Retzlaff
2023-08-11 1:32 ` [PATCH 5/6] bpf: " Tyler Retzlaff
2023-08-11 1:32 ` [PATCH 6/6] devtools: forbid new direct use of GCC atomic builtins Tyler Retzlaff
2023-08-11 8:57 ` Bruce Richardson
2023-08-11 9:51 ` Morten Brørup
2023-08-11 15:56 ` Tyler Retzlaff
2023-08-14 6:37 ` Morten Brørup
2023-08-11 17:32 ` [PATCH v2 0/6] RFC optional rte optional stdatomics API Tyler Retzlaff
2023-08-11 17:32 ` [PATCH v2 1/6] eal: provide rte stdatomics optional atomics API Tyler Retzlaff
2023-08-14 7:06 ` Morten Brørup
2023-08-11 17:32 ` [PATCH v2 2/6] eal: adapt EAL to present rte " Tyler Retzlaff
2023-08-14 8:00 ` Morten Brørup
2023-08-14 17:47 ` Tyler Retzlaff
2023-08-16 20:13 ` Morten Brørup
2023-08-16 20:32 ` Tyler Retzlaff
2023-08-11 17:32 ` [PATCH v2 3/6] eal: add rte atomic qualifier with casts Tyler Retzlaff
2023-08-14 8:05 ` Morten Brørup
2023-08-11 17:32 ` [PATCH v2 4/6] distributor: adapt for EAL optional atomics API changes Tyler Retzlaff
2023-08-14 8:07 ` Morten Brørup
2023-08-11 17:32 ` [PATCH v2 5/6] bpf: " Tyler Retzlaff
2023-08-14 8:11 ` Morten Brørup
2023-08-11 17:32 ` [PATCH v2 6/6] devtools: forbid new direct use of GCC atomic builtins Tyler Retzlaff
2023-08-14 8:12 ` Morten Brørup
2023-08-16 19:19 ` [PATCH v3 0/6] RFC optional rte optional stdatomics API Tyler Retzlaff
2023-08-16 19:19 ` [PATCH v3 1/6] eal: provide rte stdatomics optional atomics API Tyler Retzlaff
2023-08-16 20:55 ` Morten Brørup
2023-08-16 21:04 ` Tyler Retzlaff
2023-08-16 21:08 ` Morten Brørup
2023-08-16 21:10 ` Tyler Retzlaff
2023-08-16 19:19 ` [PATCH v3 2/6] eal: adapt EAL to present rte " Tyler Retzlaff
2023-08-16 19:19 ` [PATCH v3 3/6] eal: add rte atomic qualifier with casts Tyler Retzlaff
2023-08-16 19:19 ` [PATCH v3 4/6] distributor: adapt for EAL optional atomics API changes Tyler Retzlaff
2023-08-16 19:19 ` [PATCH v3 5/6] bpf: " Tyler Retzlaff
2023-08-16 19:19 ` [PATCH v3 6/6] devtools: forbid new direct use of GCC atomic builtins Tyler Retzlaff
2023-08-16 21:38 ` [PATCH v4 0/6] RFC optional rte optional stdatomics API Tyler Retzlaff
2023-08-16 21:38 ` [PATCH v4 1/6] eal: provide rte stdatomics optional atomics API Tyler Retzlaff
2023-08-17 11:45 ` Morten Brørup
2023-08-17 19:09 ` Tyler Retzlaff
2023-08-18 6:55 ` Morten Brørup
2023-08-16 21:38 ` [PATCH v4 2/6] eal: adapt EAL to present rte " Tyler Retzlaff
2023-08-16 21:38 ` [PATCH v4 3/6] eal: add rte atomic qualifier with casts Tyler Retzlaff
2023-08-16 21:38 ` [PATCH v4 4/6] distributor: adapt for EAL optional atomics API changes Tyler Retzlaff
2023-08-16 21:38 ` [PATCH v4 5/6] bpf: " Tyler Retzlaff
2023-08-16 21:38 ` [PATCH v4 6/6] devtools: forbid new direct use of GCC atomic builtins Tyler Retzlaff
2023-08-17 11:57 ` Morten Brørup
2023-08-17 19:14 ` Tyler Retzlaff
2023-08-18 7:13 ` Morten Brørup
2023-08-22 18:14 ` Tyler Retzlaff [this message]
2023-08-17 21:42 ` [PATCH v5 0/6] optional rte optional stdatomics API Tyler Retzlaff
2023-08-17 21:42 ` [PATCH v5 1/6] eal: provide rte stdatomics optional atomics API Tyler Retzlaff
2023-08-17 21:42 ` [PATCH v5 2/6] eal: adapt EAL to present rte " Tyler Retzlaff
2023-08-17 21:42 ` [PATCH v5 3/6] eal: add rte atomic qualifier with casts Tyler Retzlaff
2023-08-17 21:42 ` [PATCH v5 4/6] distributor: adapt for EAL optional atomics API changes Tyler Retzlaff
2023-08-17 21:42 ` [PATCH v5 5/6] bpf: " Tyler Retzlaff
2023-08-17 21:42 ` [PATCH v5 6/6] devtools: forbid new direct use of GCC atomic builtins Tyler Retzlaff
2023-08-21 22:27 ` [PATCH v5 0/6] optional rte optional stdatomics API Konstantin Ananyev
2023-08-22 21:00 ` [PATCH v6 0/6] rte atomics API for optional stdatomic Tyler Retzlaff
2023-08-22 21:00 ` [PATCH v6 1/6] eal: provide rte stdatomics optional atomics API Tyler Retzlaff
2023-09-28 8:06 ` Thomas Monjalon
2023-09-29 8:04 ` David Marchand
2023-09-29 8:54 ` Morten Brørup
2023-09-29 9:02 ` David Marchand
2023-09-29 9:26 ` Bruce Richardson
2023-09-29 9:34 ` David Marchand
2023-09-29 10:26 ` Thomas Monjalon
2023-09-29 11:38 ` David Marchand
2023-09-29 11:51 ` Thomas Monjalon
2023-08-22 21:00 ` [PATCH v6 2/6] eal: adapt EAL to present rte " Tyler Retzlaff
2023-08-22 21:00 ` [PATCH v6 3/6] eal: add rte atomic qualifier with casts Tyler Retzlaff
2023-08-22 21:00 ` [PATCH v6 4/6] distributor: adapt for EAL optional atomics API changes Tyler Retzlaff
2023-08-22 21:00 ` [PATCH v6 5/6] bpf: " Tyler Retzlaff
2023-08-22 21:00 ` [PATCH v6 6/6] devtools: forbid new direct use of GCC atomic builtins Tyler Retzlaff
2023-08-29 15:57 ` [PATCH v6 0/6] rte atomics API for optional stdatomic Tyler Retzlaff
2023-09-29 14:09 ` David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230822181432.GA7299@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net \
--to=roretzla@linux.microsoft.com \
--cc=bruce.richardson@intel.com \
--cc=david.hunt@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=drc@linux.vnet.ibm.com \
--cc=honnappa.nagarahalli@arm.com \
--cc=jerinj@marvell.com \
--cc=joyce.kong@arm.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=mattias.ronnblom@ericsson.com \
--cc=mb@smartsharesystems.com \
--cc=ruifeng.wang@arm.com \
--cc=skori@marvell.com \
--cc=techboard@dpdk.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).