> -----Original Message-----
> From: David Marchand <david.march...@redhat.com>
> Sent: Thursday, July 16, 2020 6:49 PM
> To: Phil Yang <phil.y...@arm.com>
> Cc: tho...@monjalon.net; Mcnamara, John <john.mcnam...@intel.com>;
> Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>; David Christensen
> <d...@linux.vnet.ibm.com>; dev <dev@dpdk.org>; jer...@marvell.com;
> Ananyev, Konstantin <konstantin.anan...@intel.com>; Ola Liljedahl
> <ola.liljed...@arm.com>; Bruce Richardson <bruce.richard...@intel.com>;
> Ruifeng Wang <ruifeng.w...@arm.com>; nd <n...@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.y...@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.y...@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.w...@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
