> From: Tyler Retzlaff [mailto:roret...@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:roret...@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? 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? > > > "$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.
