[v2,0/8] use GCC's C11 atomic builtins for test

  Since C11 memory model is adopted in DPDK now[1], use GCC's 
atomic builtins in test cases.

[1] https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/

v2:
 Use rte_wait_until_equal() instead of original sync loops.
 <David Marchand>

v1:
 The initial version.

Joyce Kong (8):
  test/ticketlock: use GCC atomic builtins for lcores sync
  test/spinlock: use GCC atomic builtins for lcores sync
  test/rwlock: use GCC atomic builtins for lcores sync
  test/mcslock: use GCC atomic builtins for lcores sync
  test/mempool: remove unused variable for lcores sync
  test/mempool_perf: use GCC atomic builtins for lcores sync
  test/service_cores: use GCC atomic builtins for lock sync
  test/rcu: use GCC atomic builtins for data sync

 app/test/test_mcslock.c       | 14 +++--
 app/test/test_mempool.c       |  5 --
 app/test/test_mempool_perf.c  | 11 ++--
 app/test/test_rcu_qsbr_perf.c | 98 +++++++++++++++++------------------
 app/test/test_rwlock.c        | 10 ++--
 app/test/test_service_cores.c | 36 +++++++------
 app/test/test_spinlock.c      |  9 ++--
 app/test/test_ticketlock.c    | 10 ++--
 8 files changed, 91 insertions(+), 102 deletions(-)
  

On Tue, Jun 15, 2021 at 09:54:51PM -0500, Joyce Kong wrote:
> Since C11 memory model is adopted in DPDK now[1], use GCC's 
> atomic builtins in test cases.

as previously discussed these atomics are not "C11" they are
direct use of gcc builtins. please don't incorporate C11 into the title
of the patches or commit messages since it isn't.

please do not integrate a patch that directly uses gcc builtins and
extensions please maintain abstractions under the rte_ namespace.
specifically this patch substantially increases coupling to a single
compiler implementation reducing portability.

as previously requested, please establish at a minimum macros in the
rte_ namespace for this.

thanks.

> 
> [1] https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-model/
> 
> v2:
>  Use rte_wait_until_equal() instead of original sync loops.
>  <David Marchand>
> 
> v1:
>  The initial version.
> 
> Joyce Kong (8):
>   test/ticketlock: use GCC atomic builtins for lcores sync
>   test/spinlock: use GCC atomic builtins for lcores sync
>   test/rwlock: use GCC atomic builtins for lcores sync
>   test/mcslock: use GCC atomic builtins for lcores sync
>   test/mempool: remove unused variable for lcores sync
>   test/mempool_perf: use GCC atomic builtins for lcores sync
>   test/service_cores: use GCC atomic builtins for lock sync
>   test/rcu: use GCC atomic builtins for data sync
> 
>  app/test/test_mcslock.c       | 14 +++--
>  app/test/test_mempool.c       |  5 --
>  app/test/test_mempool_perf.c  | 11 ++--
>  app/test/test_rcu_qsbr_perf.c | 98 +++++++++++++++++------------------
>  app/test/test_rwlock.c        | 10 ++--
>  app/test/test_service_cores.c | 36 +++++++------
>  app/test/test_spinlock.c      |  9 ++--
>  app/test/test_ticketlock.c    | 10 ++--
>  8 files changed, 91 insertions(+), 102 deletions(-)
> 
> -- 
> 2.17.1
  

<snip>
> 
> On Tue, Jun 15, 2021 at 09:54:51PM -0500, Joyce Kong wrote:
> > Since C11 memory model is adopted in DPDK now[1], use GCC's atomic
> > builtins in test cases.
> 
> as previously discussed these atomics are not "C11" they are direct use of gcc
> builtins. please don't incorporate C11 into the title of the patches or commit
> messages since it isn't.
GCC supports 2 types of built-in atomics, __atomic_xxx[1] and __sync_xxx [2]. We need a way to distinguish between them. We are using "C11" as [1] says they match C++11 memory model.

[1] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
[2] https://gcc.gnu.org/onlinedocs/gcc-4.1.1/gcc/Atomic-Builtins.html

> 
> please do not integrate a patch that directly uses gcc builtins and extensions
> please maintain abstractions under the rte_ namespace.
This is just another wrapper which is not required according to the current requirements we are working with.

> specifically this patch substantially increases coupling to a single compiler
> implementation reducing portability.
> 
> as previously requested, please establish at a minimum macros in the rte_
> namespace for this.
This needs to be discussed at the Techboard. I have CCed the Techboard. The Techboard meets once in 2 weeks. The details are at [3]. Next meeting is on 6/30 at 10am CST. Can you please attend and make your case?

Alternately, you can send an email to techboard@dpdk.org explaining your case and we will debate and make a decision.

[3] https://core.dpdk.org/techboard/

> 
> thanks.
> 
> >
> > [1]
> > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-
> model/
> >
> > v2:
> >  Use rte_wait_until_equal() instead of original sync loops.
> >  <David Marchand>
> >
> > v1:
> >  The initial version.
> >
> > Joyce Kong (8):
> >   test/ticketlock: use GCC atomic builtins for lcores sync
> >   test/spinlock: use GCC atomic builtins for lcores sync
> >   test/rwlock: use GCC atomic builtins for lcores sync
> >   test/mcslock: use GCC atomic builtins for lcores sync
> >   test/mempool: remove unused variable for lcores sync
> >   test/mempool_perf: use GCC atomic builtins for lcores sync
> >   test/service_cores: use GCC atomic builtins for lock sync
> >   test/rcu: use GCC atomic builtins for data sync
> >
> >  app/test/test_mcslock.c       | 14 +++--
> >  app/test/test_mempool.c       |  5 --
> >  app/test/test_mempool_perf.c  | 11 ++--
> > app/test/test_rcu_qsbr_perf.c | 98 +++++++++++++++++------------------
> >  app/test/test_rwlock.c        | 10 ++--
> >  app/test/test_service_cores.c | 36 +++++++------
> >  app/test/test_spinlock.c      |  9 ++--
> >  app/test/test_ticketlock.c    | 10 ++--
> >  8 files changed, 91 insertions(+), 102 deletions(-)
> >
> > --
> > 2.17.1
  

18/06/2021 01:26, Honnappa Nagarahalli:
> > On Tue, Jun 15, 2021 at 09:54:51PM -0500, Joyce Kong wrote:
> > > Since C11 memory model is adopted in DPDK now[1], use GCC's atomic
> > > builtins in test cases.
> > 
> > as previously discussed these atomics are not "C11" they are direct use of gcc
> > builtins. please don't incorporate C11 into the title of the patches or commit
> > messages since it isn't.
> 
> GCC supports 2 types of built-in atomics,
> __atomic_xxx[1] and __sync_xxx [2].
> We need a way to distinguish between them.
> We are using "C11" as [1] says they match C++11 memory model.

I agree it would be more correct to mention "compiler builtin"
as it is not strictly the C11 API.

> [1] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> [2] https://gcc.gnu.org/onlinedocs/gcc-4.1.1/gcc/Atomic-Builtins.html
> 
> > 
> > please do not integrate a patch that directly uses gcc builtins and extensions
> > please maintain abstractions under the rte_ namespace.
> 
> This is just another wrapper which is not required
> according to the current requirements we are working with.

Yes such wrapper is not required *today*.
We have 2 options:
	1/ introduce a wrapper now to anticipate any future issue
	2/ introduce a wrapper later when required

Given we already use these builtins, we should not block this patchset.
If it is decided to change the policy, then we'll replace the calls
to the compiler builtins in all the codebase.

> > specifically this patch substantially increases coupling to a single compiler
> > implementation reducing portability.
> > 
> > as previously requested, please establish at a minimum macros in the rte_
> > namespace for this.
> 
> This needs to be discussed at the Techboard. I have CCed the Techboard.
> The Techboard meets once in 2 weeks. The details are at [3].
> Next meeting is on 6/30 at 10am CST. Can you please attend and make your case?

I agree to discuss options 1 or 2 in a techboard meeting.

> Alternately, you can send an email to techboard@dpdk.org
> explaining your case and we will debate and make a decision.
> 
> [3] https://core.dpdk.org/techboard/
> 
> > 
> > thanks.
> > 
> > > [1]
> > > https://www.dpdk.org/blog/2021/03/26/dpdk-adopts-the-c11-memory-
> > model/
  

On Wed, Jun 23, 2021 at 12:24:55PM +0200, Thomas Monjalon wrote:
> 18/06/2021 01:26, Honnappa Nagarahalli:
> 
> Yes such wrapper is not required *today*.
> We have 2 options:
> 	1/ introduce a wrapper now to anticipate any future issue
> 	2/ introduce a wrapper later when required
> 
> Given we already use these builtins, we should not block this patchset.
> If it is decided to change the policy, then we'll replace the calls
> to the compiler builtins in all the codebase.

agreed. on the condition that the community accepts that a broad
tree-wide change (or set of changes) will have to be accepted later
to introduce the abstraction.

> > This needs to be discussed at the Techboard. I have CCed the Techboard.
> > The Techboard meets once in 2 weeks. The details are at [3].
> > Next meeting is on 6/30 at 10am CST. Can you please attend and make your case?
> 
> I agree to discuss options 1 or 2 in a techboard meeting.

option 2 (when required) is acceptable. in general it would be good to
communicate that this isn't the only abstraction that will be
introduced to improve portability. there are areas other than atomics
that need to be addressed in the platform/toolchain matrix.

thanks
  

<snip>
> 
> 18/06/2021 01:26, Honnappa Nagarahalli:
> > > On Tue, Jun 15, 2021 at 09:54:51PM -0500, Joyce Kong wrote:
> > > > Since C11 memory model is adopted in DPDK now[1], use GCC's atomic
> > > > builtins in test cases.
> > >
> > > as previously discussed these atomics are not "C11" they are direct
> > > use of gcc builtins. please don't incorporate C11 into the title of
> > > the patches or commit messages since it isn't.
> >
> > GCC supports 2 types of built-in atomics, __atomic_xxx[1] and
> > __sync_xxx [2].
> > We need a way to distinguish between them.
> > We are using "C11" as [1] says they match C++11 memory model.
> 
> I agree it would be more correct to mention "compiler builtin"
> as it is not strictly the C11 API.
The log already mentions "GCC's C11 atomic builtins". I think that is correct enough and represents the change correctly.

> 
> > [1] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
> > [2] https://gcc.gnu.org/onlinedocs/gcc-4.1.1/gcc/Atomic-Builtins.html
> >
> > >
> > > please do not integrate a patch that directly uses gcc builtins and
> > > extensions please maintain abstractions under the rte_ namespace.
> >
<snip>
  

On Tue, Jun 29, 2021 at 05:04:55PM +0000, Honnappa Nagarahalli wrote:
> <snip>
> > 
> > 18/06/2021 01:26, Honnappa Nagarahalli:
> > > > On Tue, Jun 15, 2021 at 09:54:51PM -0500, Joyce Kong wrote:
> > > > > Since C11 memory model is adopted in DPDK now[1], use GCC's atomic
> > > > > builtins in test cases.
> > > >
> > > > as previously discussed these atomics are not "C11" they are direct
> > > > use of gcc builtins. please don't incorporate C11 into the title of
> > > > the patches or commit messages since it isn't.
> > >
> > > GCC supports 2 types of built-in atomics, __atomic_xxx[1] and
> > > __sync_xxx [2].
> > > We need a way to distinguish between them.
> > > We are using "C11" as [1] says they match C++11 memory model.
> > 
> > I agree it would be more correct to mention "compiler builtin"
> > as it is not strictly the C11 API.
> The log already mentions "GCC's C11 atomic builtins". I think that is correct enough and represents the change correctly.

it's misleading and does not attract the correct reviewers particularly
due to prominence in the commit/mail subject.

please change it to "Use GCC atomic builtins" which describes clearly
the actual change without ambiguity.  using "C11" implies the patch is
adding code that uses C11 stdatomic.h and it doesn't.
  

<snip>

> > >
> > > 18/06/2021 01:26, Honnappa Nagarahalli:
> > > > > On Tue, Jun 15, 2021 at 09:54:51PM -0500, Joyce Kong wrote:
> > > > > > Since C11 memory model is adopted in DPDK now[1], use GCC's
> > > > > > atomic builtins in test cases.
> > > > >
> > > > > as previously discussed these atomics are not "C11" they are
> > > > > direct use of gcc builtins. please don't incorporate C11 into
> > > > > the title of the patches or commit messages since it isn't.
> > > >
> > > > GCC supports 2 types of built-in atomics, __atomic_xxx[1] and
> > > > __sync_xxx [2].
> > > > We need a way to distinguish between them.
> > > > We are using "C11" as [1] says they match C++11 memory model.
> > >
> > > I agree it would be more correct to mention "compiler builtin"
> > > as it is not strictly the C11 API.
> > The log already mentions "GCC's C11 atomic builtins". I think that is correct
> enough and represents the change correctly.
> 
> it's misleading and does not attract the correct reviewers particularly due to
> prominence in the commit/mail subject.
> 
> please change it to "Use GCC atomic builtins" which describes clearly the
> actual change without ambiguity.  using "C11" implies the patch is adding
> code that uses C11 stdatomic.h and it doesn't.
As I mentioned earlier in this thread, GCC supports 2 types of atomics. "Use GCC atomic builtins" does not help distinguish between them. In "GCC's C11 atomic builtins" - "C11" indicates which atomics we are using, "atomic builtins" indicates that we are NOT using APIs from stdatomic.h
  

On Wed, Jun 30, 2021 at 07:06:31PM +0000, Honnappa Nagarahalli wrote:
> <snip>
> 
> As I mentioned earlier in this thread, GCC supports 2 types of atomics. "Use GCC atomic builtins" does not help distinguish between them. In "GCC's C11 atomic builtins" - "C11" indicates which atomics we are using, "atomic builtins" indicates that we are NOT using APIs from stdatomic.h

if you need a term to distinguish the two sets of atomics in gcc you can
qualify it with "Memory Model Aware" which is straight from the gcc
manual.
  

<snip>

> >
> > As I mentioned earlier in this thread, GCC supports 2 types of
> > atomics. "Use GCC atomic builtins" does not help distinguish between
> > them. In "GCC's C11 atomic builtins" - "C11" indicates which atomics
> > we are using, "atomic builtins" indicates that we are NOT using APIs
> > from stdatomic.h
> 
> if you need a term to distinguish the two sets of atomics in gcc you can qualify
> it with "Memory Model Aware" which is straight from the gcc manual.
"Memory model aware" sounds too generic. The same page [1] also makes it clear that the built-in functions match the requirements for the C11 memory model.

There are also several patches merged in the past which do not use the term "memory model aware". I would prefer to be consistent.

[1] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
  

On Wed, Jun 30, 2021 at 08:25:44PM +0000, Honnappa Nagarahalli wrote:
> <snip>
> 
> > >
> > > As I mentioned earlier in this thread, GCC supports 2 types of
> > > atomics. "Use GCC atomic builtins" does not help distinguish between
> > > them. In "GCC's C11 atomic builtins" - "C11" indicates which atomics
> > > we are using, "atomic builtins" indicates that we are NOT using APIs
> > > from stdatomic.h
> > 
> > if you need a term to distinguish the two sets of atomics in gcc you can qualify
> > it with "Memory Model Aware" which is straight from the gcc manual.
> "Memory model aware" sounds too generic. The same page [1] also makes it clear that the built-in functions match the requirements for the C11 memory model.

allow me to put your interpretation of the manual that you linked side
by side with what the manual text actually says verbatim.

your text from above
  "built-in functions match the requirements for the C11 memory model."

the actual text from your link
  "built-in functions approximately match the requirements for the C++11 memory model."

* you've chosen to drop approximately from the wording to try and make
  your argument.

* you've also chosen to substitute C11 in place of C++11. again
  presumably for the same reason.

in fact the entire page does not mention C11 even once, it also goes on
to highlight a specific deviation from C++11 with this excerpt "because
of a deficiency in C++11's semantics for memory_order_consume"

> There are also several patches merged in the past which do not use the term "memory model aware". I would prefer to be consistent.

i prefer the history represent the change. that previous submitters and
reviewers lacked precision is not my concern nor is consistency a reason
to continue documenting history incorrectly.

i'm waiting to ack the change, it's up to you. you've already spent more
time arguing than it would have taken to submit a v2 correcting the
problem.

> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
  

<snip>

> >
> > > >
> > > > As I mentioned earlier in this thread, GCC supports 2 types of
> > > > atomics. "Use GCC atomic builtins" does not help distinguish
> > > > between them. In "GCC's C11 atomic builtins" - "C11" indicates
> > > > which atomics we are using, "atomic builtins" indicates that we
> > > > are NOT using APIs from stdatomic.h
> > >
> > > if you need a term to distinguish the two sets of atomics in gcc you
> > > can qualify it with "Memory Model Aware" which is straight from the gcc
> manual.
> > "Memory model aware" sounds too generic. The same page [1] also makes
> it clear that the built-in functions match the requirements for the C11
> memory model.
> 
> allow me to put your interpretation of the manual that you linked side by side
> with what the manual text actually says verbatim.
> 
> your text from above
>   "built-in functions match the requirements for the C11 memory model."
> 
> the actual text from your link
>   "built-in functions approximately match the requirements for the C++11
> memory model."
> 
> * you've chosen to drop approximately from the wording to try and make
>   your argument.
I am not sure how this makes a difference to our arguments. For ex: there are no other built in functions that "exactly" match the C++11 memory model supported by GCC.

> 
> * you've also chosen to substitute C11 in place of C++11. again
>   presumably for the same reason.
> 
> in fact the entire page does not mention C11 even once, it also goes on to
> highlight a specific deviation from C++11 with this excerpt "because of a
> deficiency in C++11's semantics for memory_order_consume"
I do not have a problem to call it C++11. IMO, calling it "GCC's C++11 ..." will address this deviation and the approximation.

> 
> > There are also several patches merged in the past which do not use the term
> "memory model aware". I would prefer to be consistent.
> 
> i prefer the history represent the change. that previous submitters and
> reviewers lacked precision is not my concern nor is consistency a reason to
> continue documenting history incorrectly.
Ok. As I mentioned, it is just my preference.

> 
> i'm waiting to ack the change, it's up to you. you've already spent more time
> arguing than it would have taken to submit a v2 correcting the problem.
I am not arguing for the sake of arguing. You are trying to correct few mistakes here (I truly appreciate that) and I am trying to explain my POV and making corrections as needed. I am sure we will conclude soon.

> 
> >
> > [1] https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
  

Hi David,

Since we have some discussion about the atomic operations now, I changed the commit message from "C11 atomics"(which has been widely used in previous commit) to "GCC atomic built-ins".
What's your opinion about whether keeping the previous message for the consistency or using the new description?

Joyce

> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Thursday, July 1, 2021 6:41 AM
> To: Tyler Retzlaff <roretzla@linux.microsoft.com>
> Cc: thomas@monjalon.net; Joyce Kong <Joyce.Kong@arm.com>;
> dev@dpdk.org; david.marchand@redhat.com;
> stephen@networkplumber.org; olivier.matz@6wind.com;
> andrew.rybchenko@oktetlabs.ru; harry.van.haaren@intel.com; Ruifeng
> Wang <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; techboard@dpdk.org;
> nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v2 0/8] use GCC's C11 atomic builtins for test
> 
> <snip>
> 
> > >
> > > > >
> > > > > As I mentioned earlier in this thread, GCC supports 2 types of
> > > > > atomics. "Use GCC atomic builtins" does not help distinguish
> > > > > between them. In "GCC's C11 atomic builtins" - "C11" indicates
> > > > > which atomics we are using, "atomic builtins" indicates that we
> > > > > are NOT using APIs from stdatomic.h
> > > >
> > > > if you need a term to distinguish the two sets of atomics in gcc
> > > > you can qualify it with "Memory Model Aware" which is straight
> > > > from the gcc
> > manual.
> > > "Memory model aware" sounds too generic. The same page [1] also
> > > makes
> > it clear that the built-in functions match the requirements for the
> > C11 memory model.
> >
> > allow me to put your interpretation of the manual that you linked side
> > by side with what the manual text actually says verbatim.
> >
> > your text from above
> >   "built-in functions match the requirements for the C11 memory model."
> >
> > the actual text from your link
> >   "built-in functions approximately match the requirements for the
> > C++11 memory model."
> >
> > * you've chosen to drop approximately from the wording to try and make
> >   your argument.
> I am not sure how this makes a difference to our arguments. For ex: there
> are no other built in functions that "exactly" match the C++11 memory model
> supported by GCC.
> 
> >
> > * you've also chosen to substitute C11 in place of C++11. again
> >   presumably for the same reason.
> >
> > in fact the entire page does not mention C11 even once, it also goes
> > on to highlight a specific deviation from C++11 with this excerpt
> > "because of a deficiency in C++11's semantics for
> memory_order_consume"
> I do not have a problem to call it C++11. IMO, calling it "GCC's C++11 ..." will
> address this deviation and the approximation.
> 
> >
> > > There are also several patches merged in the past which do not use
> > > the term
> > "memory model aware". I would prefer to be consistent.
> >
> > i prefer the history represent the change. that previous submitters
> > and reviewers lacked precision is not my concern nor is consistency a
> > reason to continue documenting history incorrectly.
> Ok. As I mentioned, it is just my preference.
> 
> >
> > i'm waiting to ack the change, it's up to you. you've already spent
> > more time arguing than it would have taken to submit a v2 correcting the
> problem.
> I am not arguing for the sake of arguing. You are trying to correct few
> mistakes here (I truly appreciate that) and I am trying to explain my POV and
> making corrections as needed. I am sure we will conclude soon.
> 
> >
> > >
> > > [1]
> > > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html
  

13/07/2021 09:28, Joyce Kong:
> Hi David,
> 
> Since we have some discussion about the atomic operations now, I changed the commit message from "C11 atomics"(which has been widely used in previous commit) to "GCC atomic built-ins".
> What's your opinion about whether keeping the previous message for the consistency or using the new description?

Given clang adopted the same syntax as GCC,
I prefer "compiler atomic builtins".