[v8,2/3] devtools: prevent use of rte atomic APIs in future patches
Checks
Commit Message
In order to deprecate the rte_atomic and rte_smp barrier APIs, prevent
the patches from using these APIs in the converted modules and compilers
__sync built-ins in all modules.
The converted modules:
lib/librte_distributor
lib/librte_hash
lib/librte_kni
lib/librte_lpm
lib/librte_rcu
lib/librte_ring
lib/librte_stack
lib/librte_vhost
lib/librte_timer
lib/librte_ipsec
drivers/event/octeontx
drivers/event/octeontx2
drivers/event/opdl
drivers/net/bnx2x
drivers/net/hinic
drivers/net/hns3
drivers/net/memif
drivers/net/thunderx
drivers/net/virtio
examples/l2fwd-event
On x86 the __atomic_thread_fence(__ATOMIC_SEQ_CST) is quite expensive
for SMP case. Flag the new code which use __atomic_thread_fence API.
Signed-off-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
devtools/checkpatches.sh | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
Comments
On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com> wrote:
>
> In order to deprecate the rte_atomic and rte_smp barrier APIs, prevent
> the patches from using these APIs in the converted modules and compilers
> __sync built-ins in all modules.
builtins* for the whole patch.
>
> The converted modules:
> lib/librte_distributor
> lib/librte_hash
> lib/librte_kni
> lib/librte_lpm
> lib/librte_rcu
> lib/librte_ring
> lib/librte_stack
> lib/librte_vhost
> lib/librte_timer
> lib/librte_ipsec
> drivers/event/octeontx
> drivers/event/octeontx2
> drivers/event/opdl
> drivers/net/bnx2x
> drivers/net/hinic
> drivers/net/hns3
> drivers/net/memif
> drivers/net/thunderx
> drivers/net/virtio
> examples/l2fwd-event
>
> On x86 the __atomic_thread_fence(__ATOMIC_SEQ_CST) is quite expensive
> for SMP case. Flag the new code which use __atomic_thread_fence API.
>
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
> devtools/checkpatches.sh | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 58021aa..fb288cf 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -51,6 +51,13 @@ print_usage () {
>
> check_forbidden_additions() { # <patch>
> res=0
> + c11_atomics_dir="lib/librte_distributor lib/librte_hash lib/librte_kni
> + lib/librte_lpm lib/librte_rcu lib/librte_ring
> + lib/librte_stack lib/librte_vhost
> + drivers/event/octeontx drivers/event/octeontx2
> + drivers/event/opdl drivers/net/bnx2x drivers/net/hinic
> + drivers/net/hns3 drivers/net/memif drivers/net/thunderx
> + drivers/net/virtio examples/l2fwd-event"
I prefer a form like:
+ c11_atomics_dir=""
+ c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
+ c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
+ c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
+ c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
+ c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
+ c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
+ c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
+ c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
+ c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
+ c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
+ c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
+ c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
+ c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
+ c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
+ c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
+ c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
+ c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
+ c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
Easier to read and update.
>
> # refrain from new additions of rte_panic() and rte_exit()
> # multiple folders and expressions are separated by spaces
> @@ -74,6 +81,39 @@ check_forbidden_additions() { # <patch>
> -v EXPRESSIONS='for[[:space:]]*\\((char|u?int|unsigned|s?size_t)' \
> -v RET_ON_FAIL=1 \
> -v MESSAGE='Declaring a variable inside for()' \
This hunk breaks the script.
This is not seen by the CI, as the CI uses its local copy of the script.
We are missing a:
+ "$1" || res=1
> +
> + # refrain from new additions of 16/32/64 bits rte_atomic_xxx()
> + # multiple folders and expressions are separated by spaces
> + awk -v FOLDERS="$c11_atomics_dir" \
> + -v EXPRESSIONS="rte_atomic[0-9][0-9]_.*\\\(" \
> + -v RET_ON_FAIL=1 \
> + -v MESSAGE='Use of rte_atomicNN_xxx APIs not allowed, use __atomic_xxx built-ins' \
Other messages in this script report what is wrong.
Example:
-v MESSAGE='Using rte_panic/rte_exit' \
-v MESSAGE='Using compiler attribute directly' \
-v MESSAGE='Declaring a variable inside for()' \
So to be consistent,
+ -v MESSAGE='Using rte_atomicNN_xxx' \
> + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> + "$1" || res=1
> +
> + # refrain from new additions of rte_smp_XXmb()
> + # multiple folders and expressions are separated by spaces
> + awk -v FOLDERS="$c11_atomics_dir" \
> + -v EXPRESSIONS="rte_smp_(r|w)?mb\\\(" \
> + -v RET_ON_FAIL=1 \
> + -v MESSAGE='Use of rte_smp_[r/w]mb not allowed, use __atomic_xxx built-ins' \
+ -v MESSAGE='Using rte_smp_[r/w]mb' \
> + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> + "$1" || res=1
> +
> + # refrain from using compiler __sync built-ins
> + awk -v FOLDERS="lib drivers app examples" \
> + -v EXPRESSIONS="__sync_.*\\\(" \
> + -v RET_ON_FAIL=1 \
> + -v MESSAGE='Use of __sync_xxx built-ins not allowed, use __atomic_xxx built-ins' \
+ -v MESSAGE='Using __sync_xxx builtins' \
> + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> + "$1" || res=1
> +
> + # 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='The __atomic_thread_fence is not allowed, use rte_atomic_thread_fence' \
+ -v MESSAGE='Using __atomic_thread_fence' \
> -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> "$1" || res=1
>
16/07/2020 12:48, David Marchand:
> On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com> wrote:
> > check_forbidden_additions() { # <patch>
> > res=0
> > + c11_atomics_dir="lib/librte_distributor lib/librte_hash lib/librte_kni
> > + lib/librte_lpm lib/librte_rcu lib/librte_ring
> > + lib/librte_stack lib/librte_vhost
> > + drivers/event/octeontx drivers/event/octeontx2
> > + drivers/event/opdl drivers/net/bnx2x drivers/net/hinic
> > + drivers/net/hns3 drivers/net/memif drivers/net/thunderx
> > + drivers/net/virtio examples/l2fwd-event"
>
> I prefer a form like:
>
> + c11_atomics_dir=""
> + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
> + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
> + c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
> + c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
>
> Easier to read and update.
Why do we need this list at all?
Are we allowed to add new code with old atomics
in other directories?
How bad it is to have a warning on non-converted libs?
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, July 16, 2020 6:49 PM
> To: Phil Yang <Phil.Yang@arm.com>
> Cc: thomas@monjalon.net; Mcnamara, John <john.mcnamara@intel.com>;
> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; David Christensen
> <drc@linux.vnet.ibm.com>; dev <dev@dpdk.org>; jerinj@marvell.com;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; Ola Liljedahl
> <Ola.Liljedahl@arm.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Ruifeng Wang <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v8 2/3] devtools: prevent use of rte atomic APIs in
> future patches
>
> On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com> wrote:
> >
> > In order to deprecate the rte_atomic and rte_smp barrier APIs, prevent
> > the patches from using these APIs in the converted modules and compilers
> > __sync built-ins in all modules.
>
> builtins* for the whole patch.
Will do.
>
>
> >
> > The converted modules:
> > lib/librte_distributor
> > lib/librte_hash
> > lib/librte_kni
> > lib/librte_lpm
> > lib/librte_rcu
> > lib/librte_ring
> > lib/librte_stack
> > lib/librte_vhost
> > lib/librte_timer
> > lib/librte_ipsec
> > drivers/event/octeontx
> > drivers/event/octeontx2
> > drivers/event/opdl
> > drivers/net/bnx2x
> > drivers/net/hinic
> > drivers/net/hns3
> > drivers/net/memif
> > drivers/net/thunderx
> > drivers/net/virtio
> > examples/l2fwd-event
> >
> > On x86 the __atomic_thread_fence(__ATOMIC_SEQ_CST) is quite
> expensive
> > for SMP case. Flag the new code which use __atomic_thread_fence API.
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> > devtools/checkpatches.sh | 40
> ++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> > index 58021aa..fb288cf 100755
> > --- a/devtools/checkpatches.sh
> > +++ b/devtools/checkpatches.sh
> > @@ -51,6 +51,13 @@ print_usage () {
> >
> > check_forbidden_additions() { # <patch>
> > res=0
> > + c11_atomics_dir="lib/librte_distributor lib/librte_hash lib/librte_kni
> > + lib/librte_lpm lib/librte_rcu lib/librte_ring
> > + lib/librte_stack lib/librte_vhost
> > + drivers/event/octeontx drivers/event/octeontx2
> > + drivers/event/opdl drivers/net/bnx2x drivers/net/hinic
> > + drivers/net/hns3 drivers/net/memif drivers/net/thunderx
> > + drivers/net/virtio examples/l2fwd-event"
>
> I prefer a form like:
>
> + c11_atomics_dir=""
> + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
> + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
> + c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
> + c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
> + c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
> + c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
>
> Easier to read and update.
>
>
> >
> > # refrain from new additions of rte_panic() and rte_exit()
> > # multiple folders and expressions are separated by spaces
> > @@ -74,6 +81,39 @@ check_forbidden_additions() { # <patch>
> > -v EXPRESSIONS='for[[:space:]]*\\((char|u?int|unsigned|s?size_t)'
> \
> > -v RET_ON_FAIL=1 \
> > -v MESSAGE='Declaring a variable inside for()' \
>
> This hunk breaks the script.
> This is not seen by the CI, as the CI uses its local copy of the script.
>
> We are missing a:
> + "$1" || res=1
Good catch. I will fix it in this patch.
>
>
> > +
> > + # refrain from new additions of 16/32/64 bits rte_atomic_xxx()
> > + # multiple folders and expressions are separated by spaces
> > + awk -v FOLDERS="$c11_atomics_dir" \
> > + -v EXPRESSIONS="rte_atomic[0-9][0-9]_.*\\\(" \
> > + -v RET_ON_FAIL=1 \
> > + -v MESSAGE='Use of rte_atomicNN_xxx APIs not allowed, use
> __atomic_xxx built-ins' \
>
> Other messages in this script report what is wrong.
> Example:
> -v MESSAGE='Using rte_panic/rte_exit' \
> -v MESSAGE='Using compiler attribute directly' \
> -v MESSAGE='Declaring a variable inside for()' \
>
> So to be consistent,
> + -v MESSAGE='Using rte_atomicNN_xxx' \
Will do.
>
>
> > + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > + "$1" || res=1
> > +
> > + # refrain from new additions of rte_smp_XXmb()
> > + # multiple folders and expressions are separated by spaces
> > + awk -v FOLDERS="$c11_atomics_dir" \
> > + -v EXPRESSIONS="rte_smp_(r|w)?mb\\\(" \
> > + -v RET_ON_FAIL=1 \
> > + -v MESSAGE='Use of rte_smp_[r/w]mb not allowed, use
> __atomic_xxx built-ins' \
>
> + -v MESSAGE='Using rte_smp_[r/w]mb' \
Will do.
>
> > + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > + "$1" || res=1
> > +
> > + # refrain from using compiler __sync built-ins
> > + awk -v FOLDERS="lib drivers app examples" \
> > + -v EXPRESSIONS="__sync_.*\\\(" \
> > + -v RET_ON_FAIL=1 \
> > + -v MESSAGE='Use of __sync_xxx built-ins not allowed, use
> __atomic_xxx built-ins' \
>
> + -v MESSAGE='Using __sync_xxx builtins' \
Will do.
>
>
> > + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > + "$1" || res=1
> > +
> > + # 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='The __atomic_thread_fence is not allowed, use
> rte_atomic_thread_fence' \
>
> + -v MESSAGE='Using __atomic_thread_fence' \
Will do.
>
>
> > -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> > "$1" || res=1
> >
>
>
> --
> David Marchand
Hello,
Thomas Monjalon <thomas@monjalon.net> writes:
> Subject: Re: [dpdk-dev] [PATCH v8 2/3] devtools: prevent use of rte atomic
> APIs in future patches
>
> 16/07/2020 12:48, David Marchand:
> > On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com> wrote:
> > > check_forbidden_additions() { # <patch>
> > > res=0
> > > + c11_atomics_dir="lib/librte_distributor lib/librte_hash lib/librte_kni
> > > + lib/librte_lpm lib/librte_rcu lib/librte_ring
> > > + lib/librte_stack lib/librte_vhost
> > > + drivers/event/octeontx drivers/event/octeontx2
> > > + drivers/event/opdl drivers/net/bnx2x drivers/net/hinic
> > > + drivers/net/hns3 drivers/net/memif drivers/net/thunderx
> > > + drivers/net/virtio examples/l2fwd-event"
> >
> > I prefer a form like:
> >
> > + c11_atomics_dir=""
> > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
> > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
> > + c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
> > + c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
> > + c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
> > + c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
> > + c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
> > + c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
> > + c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
> > + c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
> > + c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
> > + c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
> > + c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
> > + c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
> > + c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
> > + c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
> > + c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
> > + c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
> >
> > Easier to read and update.
>
> Why do we need this list at all?
> Are we allowed to add new code with old atomics
> in other directories?
> How bad it is to have a warning on non-converted libs?
From my perspective, the pros of this list are :
1. Avoid introducing false warnings in non-converted modules. Otherwise, the maintainers have to wonder if that module is converted or not.
2. Keep non-converted modules compatible. C11 atomic builtins cannot be used directly for rte_atomicXX_t variables.
The cons are :
1. The list needs updating every time we convert a module.
2. The script is not elegant as before.
Thanks,
Phil
16/07/2020 18:37, Phil Yang:
> Hello,
>
> Thomas Monjalon <thomas@monjalon.net> writes:
>
> > Subject: Re: [dpdk-dev] [PATCH v8 2/3] devtools: prevent use of rte atomic
> > APIs in future patches
> >
> > 16/07/2020 12:48, David Marchand:
> > > On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com> wrote:
> > > > check_forbidden_additions() { # <patch>
> > > > res=0
> > > > + c11_atomics_dir="lib/librte_distributor lib/librte_hash lib/librte_kni
> > > > + lib/librte_lpm lib/librte_rcu lib/librte_ring
> > > > + lib/librte_stack lib/librte_vhost
> > > > + drivers/event/octeontx drivers/event/octeontx2
> > > > + drivers/event/opdl drivers/net/bnx2x drivers/net/hinic
> > > > + drivers/net/hns3 drivers/net/memif drivers/net/thunderx
> > > > + drivers/net/virtio examples/l2fwd-event"
> > >
> > > I prefer a form like:
> > >
> > > + c11_atomics_dir=""
> > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
> > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
> > > + c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
> > > + c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
> > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
> > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
> > > + c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
> > > + c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
> > > + c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
> > > + c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
> > > + c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
> > > + c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
> > > + c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
> > > + c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
> > > + c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
> > > + c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
> > > + c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
> > > + c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
> > >
> > > Easier to read and update.
> >
> > Why do we need this list at all?
> > Are we allowed to add new code with old atomics
> > in other directories?
> > How bad it is to have a warning on non-converted libs?
>
> From my perspective, the pros of this list are :
> 1. Avoid introducing false warnings in non-converted modules. Otherwise, the maintainers have to wonder if that module is converted or not.
Don't we want to convert all libs?
If we are adding one more rte_atomic in a lib,
we should ask the question why not converting to C11, no?
> 2. Keep non-converted modules compatible. C11 atomic builtins cannot be used directly for rte_atomicXX_t variables.
>
> The cons are :
> 1. The list needs updating every time we convert a module.
> 2. The script is not elegant as before.
<snip>
> >
> > > Subject: Re: [dpdk-dev] [PATCH v8 2/3] devtools: prevent use of rte
> > > atomic APIs in future patches
> > >
> > > 16/07/2020 12:48, David Marchand:
> > > > On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com> wrote:
> > > > > check_forbidden_additions() { # <patch>
> > > > > res=0
> > > > > + c11_atomics_dir="lib/librte_distributor lib/librte_hash
> lib/librte_kni
> > > > > + lib/librte_lpm lib/librte_rcu lib/librte_ring
> > > > > + lib/librte_stack lib/librte_vhost
> > > > > + drivers/event/octeontx drivers/event/octeontx2
> > > > > + drivers/event/opdl drivers/net/bnx2x drivers/net/hinic
> > > > > + drivers/net/hns3 drivers/net/memif
> drivers/net/thunderx
> > > > > + drivers/net/virtio examples/l2fwd-event"
> > > >
> > > > I prefer a form like:
> > > >
> > > > + c11_atomics_dir=""
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
> > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
> > > > + c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
> > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
> > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
> > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
> > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
> > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
> > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
> > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
> > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
> > > >
> > > > Easier to read and update.
> > >
> > > Why do we need this list at all?
> > > Are we allowed to add new code with old atomics in other
> > > directories?
> > > How bad it is to have a warning on non-converted libs?
> >
> > From my perspective, the pros of this list are :
> > 1. Avoid introducing false warnings in non-converted modules. Otherwise,
> the maintainers have to wonder if that module is converted or not.
>
> Don't we want to convert all libs?
The goal is to convert all the libs.
> If we are adding one more rte_atomic in a lib, we should ask the question
> why not converting to C11, no?
Agree, I am fine with this approach. That will kind of distribute the conversion work as well.
>
> > 2. Keep non-converted modules compatible. C11 atomic builtins cannot be
> used directly for rte_atomicXX_t variables.
> >
> > The cons are :
> > 1. The list needs updating every time we convert a module.
This list will go away once all the modules are converted.
> > 2. The script is not elegant as before.
>
>
On Thu, 16 Jul 2020 16:59:11 +0000
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> <snip>
>
> > >
> > > > Subject: Re: [dpdk-dev] [PATCH v8 2/3] devtools: prevent use of rte
> > > > atomic APIs in future patches
> > > >
> > > > 16/07/2020 12:48, David Marchand:
> > > > > On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com> wrote:
> > > > > > check_forbidden_additions() { # <patch>
> > > > > > res=0
> > > > > > + c11_atomics_dir="lib/librte_distributor lib/librte_hash
> > lib/librte_kni
> > > > > > + lib/librte_lpm lib/librte_rcu lib/librte_ring
> > > > > > + lib/librte_stack lib/librte_vhost
> > > > > > + drivers/event/octeontx drivers/event/octeontx2
> > > > > > + drivers/event/opdl drivers/net/bnx2x drivers/net/hinic
> > > > > > + drivers/net/hns3 drivers/net/memif
> > drivers/net/thunderx
> > > > > > + drivers/net/virtio examples/l2fwd-event"
> > > > >
> > > > > I prefer a form like:
> > > > >
> > > > > + c11_atomics_dir=""
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
> > > > > + c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
> > > > >
> > > > > Easier to read and update.
> > > >
> > > > Why do we need this list at all?
> > > > Are we allowed to add new code with old atomics in other
> > > > directories?
> > > > How bad it is to have a warning on non-converted libs?
> > >
> > > From my perspective, the pros of this list are :
> > > 1. Avoid introducing false warnings in non-converted modules. Otherwise,
> > the maintainers have to wonder if that module is converted or not.
> >
> > Don't we want to convert all libs?
> The goal is to convert all the libs.
>
> > If we are adding one more rte_atomic in a lib, we should ask the question
> > why not converting to C11, no?
> Agree, I am fine with this approach. That will kind of distribute the conversion work as well.
>
> >
> > > 2. Keep non-converted modules compatible. C11 atomic builtins cannot be
> > used directly for rte_atomicXX_t variables.
> > >
> > > The cons are :
> > > 1. The list needs updating every time we convert a module.
> This list will go away once all the modules are converted.
>
> > > 2. The script is not elegant as before.
> >
> >
>
Can't the conversion just be automated with something coccinelle?
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> writes:
<snip>
>
> > >
> > > > Subject: Re: [dpdk-dev] [PATCH v8 2/3] devtools: prevent use of rte
> > > > atomic APIs in future patches
> > > >
> > > > 16/07/2020 12:48, David Marchand:
> > > > > On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com> wrote:
> > > > > > check_forbidden_additions() { # <patch>
> > > > > > res=0
> > > > > > + c11_atomics_dir="lib/librte_distributor lib/librte_hash
> > lib/librte_kni
> > > > > > + lib/librte_lpm lib/librte_rcu lib/librte_ring
> > > > > > + lib/librte_stack lib/librte_vhost
> > > > > > + drivers/event/octeontx drivers/event/octeontx2
> > > > > > + drivers/event/opdl drivers/net/bnx2x
> drivers/net/hinic
> > > > > > + drivers/net/hns3 drivers/net/memif
> > drivers/net/thunderx
> > > > > > + drivers/net/virtio examples/l2fwd-event"
> > > > >
> > > > > I prefer a form like:
> > > > >
> > > > > + c11_atomics_dir=""
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
> > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
> > > > > + c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
> > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
> > > > >
> > > > > Easier to read and update.
> > > >
> > > > Why do we need this list at all?
> > > > Are we allowed to add new code with old atomics in other
> > > > directories?
> > > > How bad it is to have a warning on non-converted libs?
> > >
> > > From my perspective, the pros of this list are :
> > > 1. Avoid introducing false warnings in non-converted modules. Otherwise,
> > the maintainers have to wonder if that module is converted or not.
> >
> > Don't we want to convert all libs?
> The goal is to convert all the libs.
>
> > If we are adding one more rte_atomic in a lib, we should ask the question
> > why not converting to C11, no?
> Agree, I am fine with this approach. That will kind of distribute the conversion
> work as well.
Will do in V9.
Thanks,
Phil
>
> >
> > > 2. Keep non-converted modules compatible. C11 atomic builtins cannot
> be
> > used directly for rte_atomicXX_t variables.
> > >
> > > The cons are :
> > > 1. The list needs updating every time we convert a module.
> This list will go away once all the modules are converted.
>
> > > 2. The script is not elegant as before.
> >
> >
>
<snip>
> >
> > > >
> > > > > Subject: Re: [dpdk-dev] [PATCH v8 2/3] devtools: prevent use of
> > > > > rte atomic APIs in future patches
> > > > >
> > > > > 16/07/2020 12:48, David Marchand:
> > > > > > On Thu, Jul 16, 2020 at 6:58 AM Phil Yang <phil.yang@arm.com>
> wrote:
> > > > > > > check_forbidden_additions() { # <patch>
> > > > > > > res=0
> > > > > > > + c11_atomics_dir="lib/librte_distributor
> > > > > > > + lib/librte_hash
> > > lib/librte_kni
> > > > > > > + lib/librte_lpm lib/librte_rcu lib/librte_ring
> > > > > > > + lib/librte_stack lib/librte_vhost
> > > > > > > + drivers/event/octeontx drivers/event/octeontx2
> > > > > > > + drivers/event/opdl drivers/net/bnx2x
> drivers/net/hinic
> > > > > > > + drivers/net/hns3 drivers/net/memif
> > > drivers/net/thunderx
> > > > > > > + drivers/net/virtio examples/l2fwd-event"
> > > > > >
> > > > > > I prefer a form like:
> > > > > >
> > > > > > + c11_atomics_dir=""
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx"
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/octeontx2"
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/event/opdl"
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/bnx2x"
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hinic"
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/hns3"
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/memif"
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/thunderx"
> > > > > > + c11_atomics_dir="$c11_atomics_dir drivers/net/virtio"
> > > > > > + c11_atomics_dir="$c11_atomics_dir examples/l2fwd-event"
> > > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_distributor"
> > > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_hash"
> > > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_kni"
> > > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_lpm"
> > > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_rcu"
> > > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_ring"
> > > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_stack"
> > > > > > + c11_atomics_dir="$c11_atomics_dir lib/librte_vhost"
> > > > > >
> > > > > > Easier to read and update.
> > > > >
> > > > > Why do we need this list at all?
> > > > > Are we allowed to add new code with old atomics in other
> > > > > directories?
> > > > > How bad it is to have a warning on non-converted libs?
> > > >
> > > > From my perspective, the pros of this list are :
> > > > 1. Avoid introducing false warnings in non-converted modules.
> > > > Otherwise,
> > > the maintainers have to wonder if that module is converted or not.
> > >
> > > Don't we want to convert all libs?
> > The goal is to convert all the libs.
> >
> > > If we are adding one more rte_atomic in a lib, we should ask the
> > > question why not converting to C11, no?
> > Agree, I am fine with this approach. That will kind of distribute the
> conversion work as well.
> >
> > >
> > > > 2. Keep non-converted modules compatible. C11 atomic builtins
> > > > cannot be
> > > used directly for rte_atomicXX_t variables.
> > > >
> > > > The cons are :
> > > > 1. The list needs updating every time we convert a module.
> > This list will go away once all the modules are converted.
> >
> > > > 2. The script is not elegant as before.
> > >
> > >
> >
>
> Can't the conversion just be automated with something coccinelle?
I do not know what 'coccinelle' means, not much help on the internet as well.
I really have not thought about automation. But, working on these conversions, I have realized that understanding of the synchronizations used is required (basically, understanding the entire module) to ensure the correct barriers are used. We could use the most strongest barrier and functionality would be fine, but the performance would take a hit.
On Fri, 17 Jul 2020 22:48:04 +0000
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> > > > Don't we want to convert all libs?
> > > The goal is to convert all the libs.
> > >
> > > > If we are adding one more rte_atomic in a lib, we should ask the
> > > > question why not converting to C11, no?
> > > Agree, I am fine with this approach. That will kind of distribute the
> > conversion work as well.
> > >
> > > >
> > > > > 2. Keep non-converted modules compatible. C11 atomic builtins
> > > > > cannot be
> > > > used directly for rte_atomicXX_t variables.
> > > > >
> > > > > The cons are :
> > > > > 1. The list needs updating every time we convert a module.
> > > This list will go away once all the modules are converted.
> > >
> > > > > 2. The script is not elegant as before.
> > > >
> > > >
> > >
> >
> > Can't the conversion just be automated with something coccinelle?
> I do not know what 'coccinelle' means, not much help on the internet as well.
>
> I really have not thought about automation. But, working on these conversions, I have realized that understanding of the synchronizations used is required (basically, understanding the entire module) to ensure the correct barriers are used. We could use the most strongest barrier and functionality would be fine, but the performance would take a hit.
coccinelle is a semantic patch tool, it allows for changes that are know how to deal
with arguments to functions etc. It is widely used in Linux kernel to do tree wide
changes (for example adding a new argument to an internal function).
https://www.kernel.org/doc/html/latest/dev-tools/coccinelle.html
@@ -51,6 +51,13 @@ print_usage () {
check_forbidden_additions() { # <patch>
res=0
+ c11_atomics_dir="lib/librte_distributor lib/librte_hash lib/librte_kni
+ lib/librte_lpm lib/librte_rcu lib/librte_ring
+ lib/librte_stack lib/librte_vhost
+ drivers/event/octeontx drivers/event/octeontx2
+ drivers/event/opdl drivers/net/bnx2x drivers/net/hinic
+ drivers/net/hns3 drivers/net/memif drivers/net/thunderx
+ drivers/net/virtio examples/l2fwd-event"
# refrain from new additions of rte_panic() and rte_exit()
# multiple folders and expressions are separated by spaces
@@ -74,6 +81,39 @@ check_forbidden_additions() { # <patch>
-v EXPRESSIONS='for[[:space:]]*\\((char|u?int|unsigned|s?size_t)' \
-v RET_ON_FAIL=1 \
-v MESSAGE='Declaring a variable inside for()' \
+
+ # refrain from new additions of 16/32/64 bits rte_atomic_xxx()
+ # multiple folders and expressions are separated by spaces
+ awk -v FOLDERS="$c11_atomics_dir" \
+ -v EXPRESSIONS="rte_atomic[0-9][0-9]_.*\\\(" \
+ -v RET_ON_FAIL=1 \
+ -v MESSAGE='Use of rte_atomicNN_xxx APIs not allowed, use __atomic_xxx built-ins' \
+ -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+ "$1" || res=1
+
+ # refrain from new additions of rte_smp_XXmb()
+ # multiple folders and expressions are separated by spaces
+ awk -v FOLDERS="$c11_atomics_dir" \
+ -v EXPRESSIONS="rte_smp_(r|w)?mb\\\(" \
+ -v RET_ON_FAIL=1 \
+ -v MESSAGE='Use of rte_smp_[r/w]mb not allowed, use __atomic_xxx built-ins' \
+ -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+ "$1" || res=1
+
+ # refrain from using compiler __sync built-ins
+ awk -v FOLDERS="lib drivers app examples" \
+ -v EXPRESSIONS="__sync_.*\\\(" \
+ -v RET_ON_FAIL=1 \
+ -v MESSAGE='Use of __sync_xxx built-ins not allowed, use __atomic_xxx built-ins' \
+ -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
+ "$1" || res=1
+
+ # 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='The __atomic_thread_fence is not allowed, use rte_atomic_thread_fence' \
-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
"$1" || res=1