[v3,2/5] Documentation/locking/atomic: Fix atomic-set litmus test
diff mbox series

Message ID 20200227004049.6853-3-boqun.feng@gmail.com
State In Next
Commit 17dcda68c0bfba2e925cff97a94dceb73d914294
Headers show
Series
  • Documentation/locking/atomic: Add litmus tests for atomic APIs
Related show

Commit Message

Boqun Feng Feb. 27, 2020, 12:40 a.m. UTC
Currently the litmus test "atomic-set" in atomic_t.txt has a few things
to be improved:

1)	The CPU/Processor numbers "P1,P2" are not only inconsistent with
	the rest of the document, which uses "CPU0" and "CPU1", but also
	unacceptable by the herd tool, which requires processors start
	at "P0".

2)	The initialization block uses a "atomic_set()", which is OK, but
	it's better to use ATOMIC_INIT() to make clear this is an
	initialization.

3)	The return value of atomic_add_unless() is discarded
	inexplicitly, which is OK for C language, but it will be helpful
	to the herd tool if we use a void cast to make the discard
	explicit.

Therefore fix these and this is the preparation for adding the litmus
test into memory-model litmus-tests directory so that people can
understand better about our requirements of atomic APIs and klitmus tool
can be used to generate tests.

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 Documentation/atomic_t.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Alan Stern Feb. 27, 2020, 4:34 p.m. UTC | #1
On Thu, 27 Feb 2020, Boqun Feng wrote:

> Currently the litmus test "atomic-set" in atomic_t.txt has a few things
> to be improved:
> 
> 1)	The CPU/Processor numbers "P1,P2" are not only inconsistent with
> 	the rest of the document, which uses "CPU0" and "CPU1", but also
> 	unacceptable by the herd tool, which requires processors start
> 	at "P0".
> 
> 2)	The initialization block uses a "atomic_set()", which is OK, but
> 	it's better to use ATOMIC_INIT() to make clear this is an
> 	initialization.
> 
> 3)	The return value of atomic_add_unless() is discarded
> 	inexplicitly, which is OK for C language, but it will be helpful
> 	to the herd tool if we use a void cast to make the discard
> 	explicit.
> 
> Therefore fix these and this is the preparation for adding the litmus
> test into memory-model litmus-tests directory so that people can
> understand better about our requirements of atomic APIs and klitmus tool
> can be used to generate tests.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Patch 5/5 in this series does basically the same thing for 
Atomic-RMW+mb__after_atomic-is-stronger-than-acquire.  How come you 
used one patch for that, but this is split into two patches (2/5 and 
4/5)?

Alan

> ---
>  Documentation/atomic_t.txt | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt
> index 0ab747e0d5ac..ceb85ada378e 100644
> --- a/Documentation/atomic_t.txt
> +++ b/Documentation/atomic_t.txt
> @@ -91,15 +91,15 @@ ops. That is:
>    C atomic-set
>  
>    {
> -    atomic_set(v, 1);
> +    atomic_t v = ATOMIC_INIT(1);
>    }
>  
> -  P1(atomic_t *v)
> +  P0(atomic_t *v)
>    {
> -    atomic_add_unless(v, 1, 0);
> +    (void)atomic_add_unless(v, 1, 0);
>    }
>  
> -  P2(atomic_t *v)
> +  P1(atomic_t *v)
>    {
>      atomic_set(v, 0);
>    }
>
Boqun Feng Feb. 28, 2020, 6:30 a.m. UTC | #2
On Thu, Feb 27, 2020 at 11:34:55AM -0500, Alan Stern wrote:
> On Thu, 27 Feb 2020, Boqun Feng wrote:
> 
> > Currently the litmus test "atomic-set" in atomic_t.txt has a few things
> > to be improved:
> > 
> > 1)	The CPU/Processor numbers "P1,P2" are not only inconsistent with
> > 	the rest of the document, which uses "CPU0" and "CPU1", but also
> > 	unacceptable by the herd tool, which requires processors start
> > 	at "P0".
> > 
> > 2)	The initialization block uses a "atomic_set()", which is OK, but
> > 	it's better to use ATOMIC_INIT() to make clear this is an
> > 	initialization.
> > 
> > 3)	The return value of atomic_add_unless() is discarded
> > 	inexplicitly, which is OK for C language, but it will be helpful
> > 	to the herd tool if we use a void cast to make the discard
> > 	explicit.
> > 
> > Therefore fix these and this is the preparation for adding the litmus
> > test into memory-model litmus-tests directory so that people can
> > understand better about our requirements of atomic APIs and klitmus tool
> > can be used to generate tests.
> > 
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> 
> Patch 5/5 in this series does basically the same thing for 
> Atomic-RMW+mb__after_atomic-is-stronger-than-acquire.  How come you 
> used one patch for that, but this is split into two patches (2/5 and 
> 4/5)?
> 

When I was working one the first version, I wasn't so sure that we would
reach the agreement of where to put the litmus tests, and the litmus
test in the atomic_t.txt obviously needs a fix, so I separated the fix
and the adding of a litmus test to make my rebase easier ;-). But you're
right, the separation is not needed now. 

I will merge those two patches into one in the next version, also with
the name adjustment you and Andrea have pointed out. Thanks!

Regards,
Boqun

> Alan
> 
> > ---
> >  Documentation/atomic_t.txt | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt
> > index 0ab747e0d5ac..ceb85ada378e 100644
> > --- a/Documentation/atomic_t.txt
> > +++ b/Documentation/atomic_t.txt
> > @@ -91,15 +91,15 @@ ops. That is:
> >    C atomic-set
> >  
> >    {
> > -    atomic_set(v, 1);
> > +    atomic_t v = ATOMIC_INIT(1);
> >    }
> >  
> > -  P1(atomic_t *v)
> > +  P0(atomic_t *v)
> >    {
> > -    atomic_add_unless(v, 1, 0);
> > +    (void)atomic_add_unless(v, 1, 0);
> >    }
> >  
> > -  P2(atomic_t *v)
> > +  P1(atomic_t *v)
> >    {
> >      atomic_set(v, 0);
> >    }
> > 
> 
>

Patch
diff mbox series

diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt
index 0ab747e0d5ac..ceb85ada378e 100644
--- a/Documentation/atomic_t.txt
+++ b/Documentation/atomic_t.txt
@@ -91,15 +91,15 @@  ops. That is:
   C atomic-set
 
   {
-    atomic_set(v, 1);
+    atomic_t v = ATOMIC_INIT(1);
   }
 
-  P1(atomic_t *v)
+  P0(atomic_t *v)
   {
-    atomic_add_unless(v, 1, 0);
+    (void)atomic_add_unless(v, 1, 0);
   }
 
-  P2(atomic_t *v)
+  P1(atomic_t *v)
   {
     atomic_set(v, 0);
   }