[00/18] locking/atomic: atomic64 type cleanup

Message ID 20190522132250.26499-1-mark.rutland@arm.com
Headers show
Series
  • locking/atomic: atomic64 type cleanup
Related show

Message

Mark Rutland May 22, 2019, 1:22 p.m.
Currently architectures return inconsistent types for atomic64 ops. Some return
long (e..g. powerpc), some return long long (e.g. arc), and some return s64
(e.g. x86).

This is a bit messy, and causes unnecessary pain (e.g. as values must be cast
before they can be printed [1]).

This series reworks all the atomic64 implementations to use s64 as the base
type for atomic64_t (as discussed [2]), and to ensure that this type is
consistently used for parameters and return values in the API, avoiding further
problems in this area.

This series (based on v5.1-rc1) can also be found in my atomics/type-cleanup
branch [3] on kernel.org.

Thanks,
Mark.

[1] https://lkml.kernel.org/r/20190310183051.87303-1-cai@lca.pw
[2] https://lkml.kernel.org/r/20190313091844.GA24390@hirez.programming.kicks-ass.net
[3] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git atomics/type-cleanup

Mark Rutland (18):
  locking/atomic: crypto: nx: prepare for atomic64_read() conversion
  locking/atomic: s390/pci: prepare for atomic64_read() conversion
  locking/atomic: generic: use s64 for atomic64
  locking/atomic: alpha: use s64 for atomic64
  locking/atomic: arc: use s64 for atomic64
  locking/atomic: arm: use s64 for atomic64
  locking/atomic: arm64: use s64 for atomic64
  locking/atomic: ia64: use s64 for atomic64
  locking/atomic: mips: use s64 for atomic64
  locking/atomic: powerpc: use s64 for atomic64
  locking/atomic: riscv: fix atomic64_sub_if_positive() offset argument
  locking/atomic: riscv: use s64 for atomic64
  locking/atomic: s390: use s64 for atomic64
  locking/atomic: sparc: use s64 for atomic64
  locking/atomic: x86: use s64 for atomic64
  locking/atomic: use s64 for atomic64_t on 64-bit
  locking/atomic: crypto: nx: remove redundant casts
  locking/atomic: s390/pci: remove redundant casts

 arch/alpha/include/asm/atomic.h       | 20 +++++------
 arch/arc/include/asm/atomic.h         | 41 +++++++++++-----------
 arch/arm/include/asm/atomic.h         | 50 +++++++++++++-------------
 arch/arm64/include/asm/atomic_ll_sc.h | 20 +++++------
 arch/arm64/include/asm/atomic_lse.h   | 34 +++++++++---------
 arch/ia64/include/asm/atomic.h        | 20 +++++------
 arch/mips/include/asm/atomic.h        | 22 ++++++------
 arch/powerpc/include/asm/atomic.h     | 44 +++++++++++------------
 arch/riscv/include/asm/atomic.h       | 44 ++++++++++++-----------
 arch/s390/include/asm/atomic.h        | 38 ++++++++++----------
 arch/s390/pci/pci_debug.c             |  2 +-
 arch/sparc/include/asm/atomic_64.h    |  8 ++---
 arch/x86/include/asm/atomic64_32.h    | 66 +++++++++++++++++------------------
 arch/x86/include/asm/atomic64_64.h    | 38 ++++++++++----------
 drivers/crypto/nx/nx-842-pseries.c    |  6 ++--
 include/asm-generic/atomic64.h        | 20 +++++------
 include/linux/types.h                 |  2 +-
 lib/atomic64.c                        | 32 ++++++++---------
 18 files changed, 252 insertions(+), 255 deletions(-)

-- 
2.11.0

Comments

Andrea Parri May 23, 2019, 8:30 a.m. | #1
Hi Mark,

On Wed, May 22, 2019 at 02:22:32PM +0100, Mark Rutland wrote:
> Currently architectures return inconsistent types for atomic64 ops. Some return

> long (e..g. powerpc), some return long long (e.g. arc), and some return s64

> (e.g. x86).


(only partially related, but probably worth asking:)

While reading the series, I realized that the following expression:

	atomic64_t v;
        ...
	typeof(v.counter) my_val = atomic64_set(&v, VAL);

is a valid expression on some architectures (in part., on architectures
which #define atomic64_set() to WRITE_ONCE()) but is invalid on others.
(This is due to the fact that WRITE_ONCE() can be used as an rvalue in
the above assignment; TBH, I ignore the reasons for having such rvalue?)

IIUC, similar considerations hold for atomic_set().

The question is whether this is a known/"expected" inconsistency in the
implementation of atomic64_set() or if this would also need to be fixed
/addressed (say in a different patchset)?

Thanks,
  Andrea
Mark Rutland May 23, 2019, 10:19 a.m. | #2
On Thu, May 23, 2019 at 10:30:13AM +0200, Andrea Parri wrote:
> Hi Mark,


Hi Andrea,

> On Wed, May 22, 2019 at 02:22:32PM +0100, Mark Rutland wrote:

> > Currently architectures return inconsistent types for atomic64 ops. Some return

> > long (e..g. powerpc), some return long long (e.g. arc), and some return s64

> > (e.g. x86).

> 

> (only partially related, but probably worth asking:)

> 

> While reading the series, I realized that the following expression:

> 

> 	atomic64_t v;

>         ...

> 	typeof(v.counter) my_val = atomic64_set(&v, VAL);

> 

> is a valid expression on some architectures (in part., on architectures

> which #define atomic64_set() to WRITE_ONCE()) but is invalid on others.

> (This is due to the fact that WRITE_ONCE() can be used as an rvalue in

> the above assignment; TBH, I ignore the reasons for having such rvalue?)

> 

> IIUC, similar considerations hold for atomic_set().

> 

> The question is whether this is a known/"expected" inconsistency in the

> implementation of atomic64_set() or if this would also need to be fixed

> /addressed (say in a different patchset)?


In either case, I don't think the intent is that they should be used that way,
and from a quick scan, I can only fine a single relevant instance today:

[mark@lakrids:~/src/linux]% git grep '\(return\|=\)\s\+atomic\(64\)\?_set'
include/linux/vmw_vmci_defs.h:  return atomic_set((atomic_t *)var, (u32)new_val);
include/linux/vmw_vmci_defs.h:  return atomic64_set(var, new_val);


[mark@lakrids:~/src/linux]% git grep '=\s+atomic_set' | wc -l
0
[mark@lakrids:~/src/linux]% git grep '=\s+atomic64_set' | wc -l
0

Any architectures implementing arch_atomic_* will have both of these functions
returning void. Currently that's x86 and arm64, but (time permitting) I intend
to migrate other architectures, so I guess we'll have to fix the above up as
required.

I think it's best to avoid the construct above.

Thanks,
Mark.
Mark Rutland May 23, 2019, 10:28 a.m. | #3
On Wed, May 22, 2019 at 11:18:59PM +0200, Arnd Bergmann wrote:
> On Wed, May 22, 2019 at 3:23 PM Mark Rutland <mark.rutland@arm.com> wrote:

> >

> > Currently architectures return inconsistent types for atomic64 ops. Some return

> > long (e..g. powerpc), some return long long (e.g. arc), and some return s64

> > (e.g. x86).

> >

> > This is a bit messy, and causes unnecessary pain (e.g. as values must be cast

> > before they can be printed [1]).

> >

> > This series reworks all the atomic64 implementations to use s64 as the base

> > type for atomic64_t (as discussed [2]), and to ensure that this type is

> > consistently used for parameters and return values in the API, avoiding further

> > problems in this area.

> >

> > This series (based on v5.1-rc1) can also be found in my atomics/type-cleanup

> > branch [3] on kernel.org.

> 

> Nice cleanup!

> 

> I've provided an explicit Ack for the asm-generic patch if someone wants

> to pick up the entire series, but I can also put it all into my asm-generic

> tree if you want, after more people have had a chance to take a look.


Thanks!

I had assumed that this would go through the tip tree, as previous
atomic rework had, but I have no preference as to how this gets merged.

I'm not sure what the policy is, so I'll leave it to Peter and Will to
say.

Mark.
Andrea Parri May 23, 2019, 11:20 a.m. | #4
> > While reading the series, I realized that the following expression:

> > 

> > 	atomic64_t v;

> >         ...

> > 	typeof(v.counter) my_val = atomic64_set(&v, VAL);

> > 

> > is a valid expression on some architectures (in part., on architectures

> > which #define atomic64_set() to WRITE_ONCE()) but is invalid on others.

> > (This is due to the fact that WRITE_ONCE() can be used as an rvalue in

> > the above assignment; TBH, I ignore the reasons for having such rvalue?)

> > 

> > IIUC, similar considerations hold for atomic_set().

> > 

> > The question is whether this is a known/"expected" inconsistency in the

> > implementation of atomic64_set() or if this would also need to be fixed

> > /addressed (say in a different patchset)?

> 

> In either case, I don't think the intent is that they should be used that way,

> and from a quick scan, I can only fine a single relevant instance today:

> 

> [mark@lakrids:~/src/linux]% git grep '\(return\|=\)\s\+atomic\(64\)\?_set'

> include/linux/vmw_vmci_defs.h:  return atomic_set((atomic_t *)var, (u32)new_val);

> include/linux/vmw_vmci_defs.h:  return atomic64_set(var, new_val);

> 

> 

> [mark@lakrids:~/src/linux]% git grep '=\s+atomic_set' | wc -l

> 0

> [mark@lakrids:~/src/linux]% git grep '=\s+atomic64_set' | wc -l

> 0

> 

> Any architectures implementing arch_atomic_* will have both of these functions

> returning void. Currently that's x86 and arm64, but (time permitting) I intend

> to migrate other architectures, so I guess we'll have to fix the above up as

> required.

> 

> I think it's best to avoid the construct above.


Thank you for the clarification, Mark.  I agree with you that it'd be
better to avoid such constructs.  (FWIW, it is not currently possible
to use them in litmus tests for the LKMM...)

Thanks,
  Andrea
Vineet Gupta May 23, 2019, 11:10 p.m. | #5
On 5/22/19 6:24 AM, Mark Rutland wrote:
> As a step towards making the atomic64 API use consistent types treewide,
> let's have the arc atomic64 implementation use s64 as the underlying
> type for atomic64_t, rather than u64, matching the generated headers.
>
> Otherwise, there should be no functional change as a result of this
> patch.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Will Deacon <will.deacon@arm.com>

Thx for the cleanup Mark.

Acked-By: Vineet Gupta <vgupta@synopsys.com>   # for ARC bits

-Vineet
Peter Zijlstra May 24, 2019, 10:37 a.m. | #6
On Thu, May 23, 2019 at 11:19:26AM +0100, Mark Rutland wrote:

> [mark@lakrids:~/src/linux]% git grep '\(return\|=\)\s\+atomic\(64\)\?_set'

> include/linux/vmw_vmci_defs.h:  return atomic_set((atomic_t *)var, (u32)new_val);

> include/linux/vmw_vmci_defs.h:  return atomic64_set(var, new_val);

> 


Oh boy, what a load of crap you just did find.

How about something like the below? I've not read how that buffer is
used, but the below preserves all broken without using atomic*_t.

---
diff --git a/include/linux/vmw_vmci_defs.h b/include/linux/vmw_vmci_defs.h
index 0c06178e4985..8ee472118f54 100644
--- a/include/linux/vmw_vmci_defs.h
+++ b/include/linux/vmw_vmci_defs.h
@@ -438,8 +438,8 @@ enum {
 struct vmci_queue_header {
 	/* All fields are 64bit and aligned. */
 	struct vmci_handle handle;	/* Identifier. */
-	atomic64_t producer_tail;	/* Offset in this queue. */
-	atomic64_t consumer_head;	/* Offset in peer queue. */
+	u64 producer_tail;	/* Offset in this queue. */
+	u64 consumer_head;	/* Offset in peer queue. */
 };
 
 /*
@@ -740,13 +740,9 @@ static inline void *vmci_event_data_payload(struct vmci_event_data *ev_data)
  * prefix will be used, so correctness isn't an issue, but using a
  * 64bit operation still adds unnecessary overhead.
  */
-static inline u64 vmci_q_read_pointer(atomic64_t *var)
+static inline u64 vmci_q_read_pointer(u64 *var)
 {
-#if defined(CONFIG_X86_32)
-	return atomic_read((atomic_t *)var);
-#else
-	return atomic64_read(var);
-#endif
+	return READ_ONCE(*(unsigned long *)var);
 }
 
 /*
@@ -755,23 +751,17 @@ static inline u64 vmci_q_read_pointer(atomic64_t *var)
  * never exceeds a 32bit value in this case. On 32bit SMP, using a
  * locked cmpxchg8b adds unnecessary overhead.
  */
-static inline void vmci_q_set_pointer(atomic64_t *var,
-				      u64 new_val)
+static inline void vmci_q_set_pointer(u64 *var, u64 new_val)
 {
-#if defined(CONFIG_X86_32)
-	return atomic_set((atomic_t *)var, (u32)new_val);
-#else
-	return atomic64_set(var, new_val);
-#endif
+	/* XXX buggered on big-endian */
+	WRITE_ONCE(*(unsigned long *)var, (unsigned long)new_val);
 }
 
 /*
  * Helper to add a given offset to a head or tail pointer. Wraps the
  * value of the pointer around the max size of the queue.
  */
-static inline void vmci_qp_add_pointer(atomic64_t *var,
-				       size_t add,
-				       u64 size)
+static inline void vmci_qp_add_pointer(u64 *var, size_t add, u64 size)
 {
 	u64 new_val = vmci_q_read_pointer(var);
 
@@ -848,8 +838,8 @@ static inline void vmci_q_header_init(struct vmci_queue_header *q_header,
 				      const struct vmci_handle handle)
 {
 	q_header->handle = handle;
-	atomic64_set(&q_header->producer_tail, 0);
-	atomic64_set(&q_header->consumer_head, 0);
+	q_header->producer_tail = 0;
+	q_header->consumer_head = 0;
 }
 
 /*
Peter Zijlstra May 24, 2019, 11:18 a.m. | #7
On Fri, May 24, 2019 at 12:37:31PM +0200, Peter Zijlstra wrote:
> On Thu, May 23, 2019 at 11:19:26AM +0100, Mark Rutland wrote:

> 

> > [mark@lakrids:~/src/linux]% git grep '\(return\|=\)\s\+atomic\(64\)\?_set'

> > include/linux/vmw_vmci_defs.h:  return atomic_set((atomic_t *)var, (u32)new_val);

> > include/linux/vmw_vmci_defs.h:  return atomic64_set(var, new_val);

> > 

> 

> Oh boy, what a load of crap you just did find.

> 

> How about something like the below? I've not read how that buffer is

> used, but the below preserves all broken without using atomic*_t.


Clarified by something along these lines?

---
 Documentation/atomic_t.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt
index dca3fb0554db..125c95ddbbc0 100644
--- a/Documentation/atomic_t.txt
+++ b/Documentation/atomic_t.txt
@@ -83,6 +83,9 @@ The non-RMW ops are (typically) regular LOADs and STOREs and are canonically
 implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and
 smp_store_release() respectively.
 
+Therefore, if you find yourself only using the Non-RMW operations of atomic_t,
+you do not in fact need atomic_t at all and are doing it wrong.
+
 The one detail to this is that atomic_set{}() should be observable to the RMW
 ops. That is:
Greg Kroah-Hartman May 24, 2019, 11:38 a.m. | #8
On Fri, May 24, 2019 at 01:18:07PM +0200, Peter Zijlstra wrote:
> On Fri, May 24, 2019 at 12:37:31PM +0200, Peter Zijlstra wrote:

> > On Thu, May 23, 2019 at 11:19:26AM +0100, Mark Rutland wrote:

> > 

> > > [mark@lakrids:~/src/linux]% git grep '\(return\|=\)\s\+atomic\(64\)\?_set'

> > > include/linux/vmw_vmci_defs.h:  return atomic_set((atomic_t *)var, (u32)new_val);

> > > include/linux/vmw_vmci_defs.h:  return atomic64_set(var, new_val);

> > > 

> > 

> > Oh boy, what a load of crap you just did find.

> > 

> > How about something like the below? I've not read how that buffer is

> > used, but the below preserves all broken without using atomic*_t.

> 

> Clarified by something along these lines?

> 

> ---

>  Documentation/atomic_t.txt | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt

> index dca3fb0554db..125c95ddbbc0 100644

> --- a/Documentation/atomic_t.txt

> +++ b/Documentation/atomic_t.txt

> @@ -83,6 +83,9 @@ The non-RMW ops are (typically) regular LOADs and STOREs and are canonically

>  implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and

>  smp_store_release() respectively.

>  

> +Therefore, if you find yourself only using the Non-RMW operations of atomic_t,

> +you do not in fact need atomic_t at all and are doing it wrong.

> +

>  The one detail to this is that atomic_set{}() should be observable to the RMW

>  ops. That is:

>  


I like it!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Will Deacon May 24, 2019, 11:42 a.m. | #9
On Fri, May 24, 2019 at 01:18:07PM +0200, Peter Zijlstra wrote:
> On Fri, May 24, 2019 at 12:37:31PM +0200, Peter Zijlstra wrote:

> > On Thu, May 23, 2019 at 11:19:26AM +0100, Mark Rutland wrote:

> > 

> > > [mark@lakrids:~/src/linux]% git grep '\(return\|=\)\s\+atomic\(64\)\?_set'

> > > include/linux/vmw_vmci_defs.h:  return atomic_set((atomic_t *)var, (u32)new_val);

> > > include/linux/vmw_vmci_defs.h:  return atomic64_set(var, new_val);

> > > 

> > 

> > Oh boy, what a load of crap you just did find.

> > 

> > How about something like the below? I've not read how that buffer is

> > used, but the below preserves all broken without using atomic*_t.

> 

> Clarified by something along these lines?

> 

> ---

>  Documentation/atomic_t.txt | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt

> index dca3fb0554db..125c95ddbbc0 100644

> --- a/Documentation/atomic_t.txt

> +++ b/Documentation/atomic_t.txt

> @@ -83,6 +83,9 @@ The non-RMW ops are (typically) regular LOADs and STOREs and are canonically

>  implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and

>  smp_store_release() respectively.

>  


Not sure you need a new paragraph here.

> +Therefore, if you find yourself only using the Non-RMW operations of atomic_t,

> +you do not in fact need atomic_t at all and are doing it wrong.

> +


That makes sense to me, although I now find that the sentence below is a bit
confusing because it sounds like it's a caveat relating to only using
Non-RMW ops.

>  The one detail to this is that atomic_set{}() should be observable to the RMW

>  ops. That is:


How about changing this to be:

  "A subtle detail of atomic_set{}() is that it should be observable..."

With that:

Acked-by: Will Deacon <will.deacon@arm.com>


Will
Peter Zijlstra May 24, 2019, 11:52 a.m. | #10
On Fri, May 24, 2019 at 12:42:20PM +0100, Will Deacon wrote:

> > diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt

> > index dca3fb0554db..125c95ddbbc0 100644

> > --- a/Documentation/atomic_t.txt

> > +++ b/Documentation/atomic_t.txt

> > @@ -83,6 +83,9 @@ The non-RMW ops are (typically) regular LOADs and STOREs and are canonically

> >  implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and

> >  smp_store_release() respectively.

> >  

> 

> Not sure you need a new paragraph here.

> 

> > +Therefore, if you find yourself only using the Non-RMW operations of atomic_t,

> > +you do not in fact need atomic_t at all and are doing it wrong.

> > +

> 

> That makes sense to me, although I now find that the sentence below is a bit

> confusing because it sounds like it's a caveat relating to only using

> Non-RMW ops.

> 

> >  The one detail to this is that atomic_set{}() should be observable to the RMW

> >  ops. That is:

> 

> How about changing this to be:

> 

>   "A subtle detail of atomic_set{}() is that it should be observable..."


Done, find below.

---
Subject: Documentation/atomic_t.txt: Clarify pure non-rmw usage

Clarify that pure non-RMW usage of atomic_t is pointless, there is
nothing 'magical' about atomic_set() / atomic_read().

This is something that seems to confuse people, because I happen upon it
semi-regularly.

Acked-by: Will Deacon <will.deacon@arm.com>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

---
 Documentation/atomic_t.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt
index dca3fb0554db..89eae7f6b360 100644
--- a/Documentation/atomic_t.txt
+++ b/Documentation/atomic_t.txt
@@ -81,9 +81,11 @@ SEMANTICS
 
 The non-RMW ops are (typically) regular LOADs and STOREs and are canonically
 implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and
-smp_store_release() respectively.
+smp_store_release() respectively. Therefore, if you find yourself only using
+the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all
+and are doing it wrong.
 
-The one detail to this is that atomic_set{}() should be observable to the RMW
+A subtle detail of atomic_set{}() is that it should be observable to the RMW
 ops. That is:
 
   C atomic-set
Peter Zijlstra May 28, 2019, 10:47 a.m. | #11
On Sat, May 25, 2019 at 12:43:40AM +0200, Andrea Parri wrote:
> > ---

> > Subject: Documentation/atomic_t.txt: Clarify pure non-rmw usage

> > 

> > Clarify that pure non-RMW usage of atomic_t is pointless, there is

> > nothing 'magical' about atomic_set() / atomic_read().

> > 

> > This is something that seems to confuse people, because I happen upon it

> > semi-regularly.

> > 

> > Acked-by: Will Deacon <will.deacon@arm.com>

> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> > ---

> >  Documentation/atomic_t.txt | 6 ++++--

> >  1 file changed, 4 insertions(+), 2 deletions(-)

> > 

> > diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt

> > index dca3fb0554db..89eae7f6b360 100644

> > --- a/Documentation/atomic_t.txt

> > +++ b/Documentation/atomic_t.txt

> > @@ -81,9 +81,11 @@ SEMANTICS

> >  

> >  The non-RMW ops are (typically) regular LOADs and STOREs and are canonically

> >  implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and

> > -smp_store_release() respectively.

> > +smp_store_release() respectively. Therefore, if you find yourself only using

> > +the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all

> > +and are doing it wrong.

> 

> The counterargument (not so theoretic, just look around in the kernel!) is:

> we all 'forget' to use READ_ONCE() and WRITE_ONCE(), it should be difficult

> or more difficult to forget to use atomic_read() and atomic_set()...   IAC,

> I wouldn't call any of them 'wrong'.


I'm thinking you mean that the type system isn't helping us with
READ/WRITE_ONCE() like it does with atomic_t ? And while I agree that
there is room for improvement there, that doesn't mean we should start
using atomic*_t all over the place for that.

Part of the problem with READ/WRITE_ONCE() is that it serves a dual
purpose; we've tried to untangle that at some point, but Linus wasn't
having it.
Andrea Parri May 28, 2019, 11:15 a.m. | #12
On Tue, May 28, 2019 at 12:47:19PM +0200, Peter Zijlstra wrote:
> On Sat, May 25, 2019 at 12:43:40AM +0200, Andrea Parri wrote:

> > > ---

> > > Subject: Documentation/atomic_t.txt: Clarify pure non-rmw usage

> > > 

> > > Clarify that pure non-RMW usage of atomic_t is pointless, there is

> > > nothing 'magical' about atomic_set() / atomic_read().

> > > 

> > > This is something that seems to confuse people, because I happen upon it

> > > semi-regularly.

> > > 

> > > Acked-by: Will Deacon <will.deacon@arm.com>

> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

> > > ---

> > >  Documentation/atomic_t.txt | 6 ++++--

> > >  1 file changed, 4 insertions(+), 2 deletions(-)

> > > 

> > > diff --git a/Documentation/atomic_t.txt b/Documentation/atomic_t.txt

> > > index dca3fb0554db..89eae7f6b360 100644

> > > --- a/Documentation/atomic_t.txt

> > > +++ b/Documentation/atomic_t.txt

> > > @@ -81,9 +81,11 @@ SEMANTICS

> > >  

> > >  The non-RMW ops are (typically) regular LOADs and STOREs and are canonically

> > >  implemented using READ_ONCE(), WRITE_ONCE(), smp_load_acquire() and

> > > -smp_store_release() respectively.

> > > +smp_store_release() respectively. Therefore, if you find yourself only using

> > > +the Non-RMW operations of atomic_t, you do not in fact need atomic_t at all

> > > +and are doing it wrong.

> > 

> > The counterargument (not so theoretic, just look around in the kernel!) is:

> > we all 'forget' to use READ_ONCE() and WRITE_ONCE(), it should be difficult

> > or more difficult to forget to use atomic_read() and atomic_set()...   IAC,

> > I wouldn't call any of them 'wrong'.

> 

> I'm thinking you mean that the type system isn't helping us with

> READ/WRITE_ONCE() like it does with atomic_t ?


Yep.


> And while I agree that

> there is room for improvement there, that doesn't mean we should start

> using atomic*_t all over the place for that.


Agreed.  But this still doesn't explain that "and are doing it wrong",
AFAICT; maybe just remove that part?

  Andrea


> 

> Part of the problem with READ/WRITE_ONCE() is that it serves a dual

> purpose; we've tried to untangle that at some point, but Linus wasn't

> having it.