There were no memory barriers on cmpxchg64() _at_all_. Fix this.
Cc: Paul Burton <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/mips/include/asm/cmpxchg.h | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
--- a/arch/mips/include/asm/cmpxchg.h
+++ b/arch/mips/include/asm/cmpxchg.h
@@ -290,9 +290,11 @@ static inline unsigned long __cmpxchg64(
* will cause a build error unless cpu_has_64bits is a \
* compile-time constant 1. \
*/ \
- if (cpu_has_64bits && kernel_uses_llsc) \
+ if (cpu_has_64bits && kernel_uses_llsc) { \
+ smp_mb__before_llsc(); \
__res = __cmpxchg64((ptr), __old, __new); \
- else \
+ smp_llsc_mb(); \
+ } else \
__res = __cmpxchg64_unsupported(); \
\
__res; \
On Wed, Apr 24, 2019 at 09:00:25PM +0000, Paul Burton wrote:
> Hi Peter,
>
> On Wed, Apr 24, 2019 at 02:36:57PM +0200, Peter Zijlstra wrote:
> > There were no memory barriers on cmpxchg64() _at_all_. Fix this.
>
> This does looks problematic, but it's worth noting that this code path
> is only applicable to 32b kernels running on 64b CPUs which is pretty
> rare. The commit message as-is suggests to me that all configurations
> are broken, which isn't the case (at least, not in this respect :) ).
OK, I hadn't gone through the ifdef selection process. I just
encountered this cmpxchg implementation and noted a significant lack of
barriers.
> >
> > Cc: Paul Burton <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > ---
> > arch/mips/include/asm/cmpxchg.h | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > --- a/arch/mips/include/asm/cmpxchg.h
> > +++ b/arch/mips/include/asm/cmpxchg.h
> > @@ -290,9 +290,11 @@ static inline unsigned long __cmpxchg64(
> > * will cause a build error unless cpu_has_64bits is a \
> > * compile-time constant 1. \
> > */ \
> > - if (cpu_has_64bits && kernel_uses_llsc) \
> > + if (cpu_has_64bits && kernel_uses_llsc) { \
> > + smp_mb__before_llsc(); \
> > __res = __cmpxchg64((ptr), __old, __new); \
> > - else \
> > + smp_llsc_mb(); \
> > + } else \
> > __res = __cmpxchg64_unsupported(); \
>
> It would be good to also add braces around the else block, and I believe
> checkpatch should be complaining about that ("braces {} should be used
> on all arms of this statement").
You're right, I'll fix up.
Hi Peter,
On Wed, Apr 24, 2019 at 02:36:57PM +0200, Peter Zijlstra wrote:
> There were no memory barriers on cmpxchg64() _at_all_. Fix this.
This does looks problematic, but it's worth noting that this code path
is only applicable to 32b kernels running on 64b CPUs which is pretty
rare. The commit message as-is suggests to me that all configurations
are broken, which isn't the case (at least, not in this respect :) ).
>
> Cc: Paul Burton <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> arch/mips/include/asm/cmpxchg.h | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> --- a/arch/mips/include/asm/cmpxchg.h
> +++ b/arch/mips/include/asm/cmpxchg.h
> @@ -290,9 +290,11 @@ static inline unsigned long __cmpxchg64(
> * will cause a build error unless cpu_has_64bits is a \
> * compile-time constant 1. \
> */ \
> - if (cpu_has_64bits && kernel_uses_llsc) \
> + if (cpu_has_64bits && kernel_uses_llsc) { \
> + smp_mb__before_llsc(); \
> __res = __cmpxchg64((ptr), __old, __new); \
> - else \
> + smp_llsc_mb(); \
> + } else \
> __res = __cmpxchg64_unsupported(); \
It would be good to also add braces around the else block, and I believe
checkpatch should be complaining about that ("braces {} should be used
on all arms of this statement").
Thanks,
Paul
> \
> __res; \
>
>