[v1] test/ticketlock: use C11 atomic builtins for lcores sync
Checks
Commit Message
Convert rte_atomic usages to C11 atomic builtins for lcores sync
in ticketlock testcases.
Signed-off-by: Joyce Kong <joyce.kong@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
app/test/test_ticketlock.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
Comments
On Wed, Apr 21, 2021 at 02:17:33AM -0500, Joyce Kong wrote:
> Convert rte_atomic usages to C11 atomic builtins for lcores sync
> in ticketlock testcases.
gcc atomic builtins aren't 'C11'
>
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
the change makes this code less portable to non-gcc emulating compilers.
please don't.
<snip>
> Subject: Re: [dpdk-dev] [PATCH v1] test/ticketlock: use C11 atomic builtins
> for lcores sync
>
> On Wed, Apr 21, 2021 at 02:17:33AM -0500, Joyce Kong wrote:
> > Convert rte_atomic usages to C11 atomic builtins for lcores sync in
> > ticketlock testcases.
>
> gcc atomic builtins aren't 'C11'
Sorry, I did not understand this, can you elaborate? I am referring to [1].
[1] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
>
> >
> > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
>
> the change makes this code less portable to non-gcc emulating compilers.
>
> please don't.
Not sure if these compilers are supported in DPDK. DPDK officially supports gcc, clang (not sure on icc).
On Thu, Apr 29, 2021 at 07:17:06PM +0000, Honnappa Nagarahalli wrote:
> <snip>
>
> > Subject: Re: [dpdk-dev] [PATCH v1] test/ticketlock: use C11 atomic builtins
> > for lcores sync
> >
> > On Wed, Apr 21, 2021 at 02:17:33AM -0500, Joyce Kong wrote:
> > > Convert rte_atomic usages to C11 atomic builtins for lcores sync in
> > > ticketlock testcases.
> >
> > gcc atomic builtins aren't 'C11'
> Sorry, I did not understand this, can you elaborate? I am referring to [1].
your subject line indicates the use of C11 which is a standard [1].
the patch itself uses gcc atomics builtins which are not part of C11
standard so the subject line is incorrect and misleading.
[1] http://www.open-std.org/jtc1/sc22/wg14/www/standards.html#9899
>
> Not sure if these compilers are supported in DPDK. DPDK officially supports gcc, clang (not sure on icc).
dpdk may incorporate support for other compilers in the future so unless
there is substantive justification for moving to non-standard/non-portable
code i'm asking that this change not be made as it will complicate those future
efforts.
<snip>
> >
> > > Subject: Re: [dpdk-dev] [PATCH v1] test/ticketlock: use C11 atomic
> > > builtins for lcores sync
> > >
> > > On Wed, Apr 21, 2021 at 02:17:33AM -0500, Joyce Kong wrote:
> > > > Convert rte_atomic usages to C11 atomic builtins for lcores sync
> > > > in ticketlock testcases.
> > >
> > > gcc atomic builtins aren't 'C11'
> > Sorry, I did not understand this, can you elaborate? I am referring to [1].
>
> your subject line indicates the use of C11 which is a standard [1].
>
> the patch itself uses gcc atomics builtins which are not part of C11 standard so
> the subject line is incorrect and misleading.
Ok, understood. How about the following?
"use gcc's C11 atomic built-ins for lcore synchronization"
>
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/standards.html#9899
>
> >
> > Not sure if these compilers are supported in DPDK. DPDK officially supports
> gcc, clang (not sure on icc).
>
> dpdk may incorporate support for other compilers in the future so unless there is
> substantive justification for moving to non-standard/non-portable code i'm
> asking that this change not be made as it will complicate those future efforts.
There is some history [1] behind why we are doing this. I guess new compiler support needs to be discussed in the future.
[1] https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
On Thu, 29 Apr 2021 21:10:04 +0000
Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote:
> <snip>
>
> > >
> > > > Subject: Re: [dpdk-dev] [PATCH v1] test/ticketlock: use C11 atomic
> > > > builtins for lcores sync
> > > >
> > > > On Wed, Apr 21, 2021 at 02:17:33AM -0500, Joyce Kong wrote:
> > > > > Convert rte_atomic usages to C11 atomic builtins for lcores sync
> > > > > in ticketlock testcases.
> > > >
> > > > gcc atomic builtins aren't 'C11'
> > > Sorry, I did not understand this, can you elaborate? I am referring to [1].
> >
> > your subject line indicates the use of C11 which is a standard [1].
> >
> > the patch itself uses gcc atomics builtins which are not part of C11 standard so
> > the subject line is incorrect and misleading.
> Ok, understood. How about the following?
> "use gcc's C11 atomic built-ins for lcore synchronization"
>
> >
> > [1] http://www.open-std.org/jtc1/sc22/wg14/www/standards.html#9899
> >
> > >
> > > Not sure if these compilers are supported in DPDK. DPDK officially supports
> > gcc, clang (not sure on icc).
> >
> > dpdk may incorporate support for other compilers in the future so unless there is
> > substantive justification for moving to non-standard/non-portable code i'm
> > asking that this change not be made as it will complicate those future efforts.
> There is some history [1] behind why we are doing this. I guess new compiler support needs to be discussed in the future.
>
> [1] https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
Footnote: C++ 11 standard memory model (see https://en.cppreference.com/w/cpp/atomic/memory_order)
which is made available as builtin extensions in clang and gcc.
On Thu, Apr 29, 2021 at 09:10:04PM +0000, Honnappa Nagarahalli wrote:
> <snip>
>
> >
> > your subject line indicates the use of C11 which is a standard [1].
> >
> > the patch itself uses gcc atomics builtins which are not part of C11 standard so
> > the subject line is incorrect and misleading.
> Ok, understood. How about the following?
> "use gcc's C11 atomic built-ins for lcore synchronization"
drop 'C11' from it and it describes the actual change
>
> >
> > [1] http://www.open-std.org/jtc1/sc22/wg14/www/standards.html#9899
> >
> > >
> > > Not sure if these compilers are supported in DPDK. DPDK officially supports
> > gcc, clang (not sure on icc).
> >
> > dpdk may incorporate support for other compilers in the future so unless there is
> > substantive justification for moving to non-standard/non-portable code i'm
> > asking that this change not be made as it will complicate those future efforts.
> There is some history [1] behind why we are doing this. I guess new compiler support needs to be discussed in the future.
>
> [1] https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
thanks for the reference. it seems this documents explicitly states the
choice to not use C11 stdatomic.h and the basis of that choice appears
to be to support old versions of gcc.
it doesn't seem particularly forward looking to reduce future compiler
portability to support old versions of gcc thereby excluding standards
compliant compilers.
i would like to hear from the tech board that it is the best
trade-off for the project to reduce compiler portability for older
versions of gcc instead of adopting standard C11 atomics which locks out
the use of other compilers.
if this change does go forward could i at least ask that the builtins
used are abstracted behind either macros or inline functions so that if
alternate implementations appear for the builtins we don't have to
perform shotgun surgery on the broader codebase when it arrives?
thanks!
<snip>
> > >
> > > your subject line indicates the use of C11 which is a standard [1].
> > >
> > > the patch itself uses gcc atomics builtins which are not part of C11
> > > standard so the subject line is incorrect and misleading.
> > Ok, understood. How about the following?
> > "use gcc's C11 atomic built-ins for lcore synchronization"
>
> drop 'C11' from it and it describes the actual change
>
> >
> > >
> > > [1] http://www.open-
> std.org/jtc1/sc22/wg14/www/standards.html#9899
> > >
> > > >
> > > > Not sure if these compilers are supported in DPDK. DPDK officially
> > > > supports
> > > gcc, clang (not sure on icc).
> > >
> > > dpdk may incorporate support for other compilers in the future so
> > > unless there is substantive justification for moving to
> > > non-standard/non-portable code i'm asking that this change not be made
> as it will complicate those future efforts.
> > There is some history [1] behind why we are doing this. I guess new
> compiler support needs to be discussed in the future.
> >
> > [1]
> > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-
> model/
>
> thanks for the reference. it seems this documents explicitly states the choice
> to not use C11 stdatomic.h and the basis of that choice appears to be to
> support old versions of gcc.
>
> it doesn't seem particularly forward looking to reduce future compiler
> portability to support old versions of gcc thereby excluding standards
> compliant compilers.
>
> i would like to hear from the tech board that it is the best trade-off for the
> project to reduce compiler portability for older versions of gcc instead of
> adopting standard C11 atomics which locks out the use of other compilers.
>
> if this change does go forward could i at least ask that the builtins used are
> abstracted behind either macros or inline functions so that if alternate
> implementations appear for the builtins we don't have to perform shotgun
> surgery on the broader codebase when it arrives?
There is already code using the built-ins in the repo. I do not see why this is any different.
How difficult it is for the compiler to support these built-ins?
If DPDK supports another compiler in the future that do not have these built-ins, the shotgun approach should be straight forward as there is a 1:1 mapping between the built-ins and the C11 atomic APIs from stdatomic.h.
>
> thanks!
On Wed, May 05, 2021 at 12:37:50AM +0000, Honnappa Nagarahalli wrote:
>
> <snip>
>
> > > >
> > > > your subject line indicates the use of C11 which is a standard [1].
> > > >
> > > > the patch itself uses gcc atomics builtins which are not part of C11
> > > > standard so the subject line is incorrect and misleading.
> > > Ok, understood. How about the following?
> > > "use gcc's C11 atomic built-ins for lcore synchronization"
> >
> > drop 'C11' from it and it describes the actual change
> >
> > >
> > > >
> > > > [1] http://www.open-
> > std.org/jtc1/sc22/wg14/www/standards.html#9899
> > > >
> > > > >
> > > > > Not sure if these compilers are supported in DPDK. DPDK officially
> > > > > supports
> > > > gcc, clang (not sure on icc).
> > > >
> > > > dpdk may incorporate support for other compilers in the future so
> > > > unless there is substantive justification for moving to
> > > > non-standard/non-portable code i'm asking that this change not be made
> > as it will complicate those future efforts.
> > > There is some history [1] behind why we are doing this. I guess new
> > compiler support needs to be discussed in the future.
> > >
> > > [1]
> > > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-
> > model/
> >
> > thanks for the reference. it seems this documents explicitly states the choice
> > to not use C11 stdatomic.h and the basis of that choice appears to be to
> > support old versions of gcc.
> >
> > it doesn't seem particularly forward looking to reduce future compiler
> > portability to support old versions of gcc thereby excluding standards
> > compliant compilers.
> >
> > i would like to hear from the tech board that it is the best trade-off for the
> > project to reduce compiler portability for older versions of gcc instead of
> > adopting standard C11 atomics which locks out the use of other compilers.
> >
> > if this change does go forward could i at least ask that the builtins used are
> > abstracted behind either macros or inline functions so that if alternate
> > implementations appear for the builtins we don't have to perform shotgun
> > surgery on the broader codebase when it arrives?
> There is already code using the built-ins in the repo. I do not see why this is any different.
i agree there are several dozen uses of gcc atomic builtins in use in
lib/* already. so it's easy to fall into the trap of only a few more
won't hurt but it's a slippery slope if everyone continues to use that
as justification for not abstracting them away now.
would you not consider abstracting behind a macro though? perhaps
submitting a patch that cleans up the existing use so the amount of
change later?
> How difficult it is for the compiler to support these built-ins?
there are a lot of non-technical factors that would have to be addressed
to answer that question so i don't think it's a useful discussion. the
functionality is supported by C11 compilers (albiet optionally) so you have
to somehow make a case for adding duplicated non-standard functionality which
is difficult politically and financially depending on the vendor.
> If DPDK supports another compiler in the future that do not have these built-ins, the shotgun approach should be straight forward as there is a 1:1 mapping between the built-ins and the C11 atomic APIs from stdatomic.h.
not if you abstract it now, otherwise it just grows.
others are free to ack this change as is. so i'm not blocking it by any
means and i'd be happy to ack a change that introduced the abstraction
instead of direct use of compiler specific builtins.
thanks
@@ -9,7 +9,6 @@
#include <sys/queue.h>
#include <unistd.h>
-#include <rte_atomic.h>
#include <rte_common.h>
#include <rte_cycles.h>
#include <rte_eal.h>
@@ -49,7 +48,7 @@ static rte_ticketlock_t tl_tab[RTE_MAX_LCORE];
static rte_ticketlock_recursive_t tlr;
static unsigned int count;
-static rte_atomic32_t synchro;
+static uint32_t synchro;
static int
test_ticketlock_per_core(__rte_unused void *arg)
@@ -112,7 +111,7 @@ load_loop_fn(void *func_param)
/* wait synchro for workers */
if (lcore != rte_get_main_lcore())
- while (rte_atomic32_read(&synchro) == 0)
+ while (__atomic_load_n(&synchro, __ATOMIC_RELAXED) == 0)
;
begin = rte_rdtsc_precise();
@@ -155,11 +154,11 @@ test_ticketlock_perf(void)
printf("\nTest with lock on %u cores...\n", rte_lcore_count());
/* Clear synchro and start workers */
- rte_atomic32_set(&synchro, 0);
+ __atomic_store_n(&synchro, 0, __ATOMIC_RELAXED);
rte_eal_mp_remote_launch(load_loop_fn, &lock, SKIP_MAIN);
/* start synchro and launch test on main */
- rte_atomic32_set(&synchro, 1);
+ __atomic_store_n(&synchro, 1, __ATOMIC_RELAXED);
load_loop_fn(&lock);
rte_eal_mp_wait_lcore();