| Message ID | 20190522132250.26499-1-mark.rutland@arm.com |
|---|---|
| Headers | show |
| Series | locking/atomic: atomic64 type cleanup | expand |
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
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.
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.
> > 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
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
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; } /*
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:
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>
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
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
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.
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.