[v4,6/6] devtools: forbid new direct use of GCC atomic builtins

  Refrain from using compiler __atomic_xxx builtins DPDK now requires
the use of rte_atomic_<op>_explicit macros when operating on DPDK
atomic variables.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
Suggested-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 devtools/checkpatches.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

> 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' \

>  		-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 \
		"$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".

-Morten
  

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

> 		"$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.

> 
> -Morten
  

> 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?

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.
  

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.