On Tue, Jan 03, 2023 at 01:25:35PM +0000, Mark Rutland wrote:
On Tue, Dec 20, 2022 at 12:08:16PM +0100, Peter Zijlstra wrote:
quoted
On Mon, Dec 19, 2022 at 12:07:25PM -0800, Boqun Feng wrote:
quoted
On Mon, Dec 19, 2022 at 04:35:30PM +0100, Peter Zijlstra wrote:
quoted
For all architectures that currently support cmpxchg_double()
implement the cmpxchg128() family of functions that is basically the
same but with a saner interface.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/arm64/include/asm/atomic_ll_sc.h | 38 +++++++++++++++++++++++
arch/arm64/include/asm/atomic_lse.h | 33 +++++++++++++++++++-
arch/arm64/include/asm/cmpxchg.h | 26 ++++++++++++++++
arch/s390/include/asm/cmpxchg.h | 33 ++++++++++++++++++++
arch/x86/include/asm/cmpxchg_32.h | 3 +
arch/x86/include/asm/cmpxchg_64.h | 55 +++++++++++++++++++++++++++++++++-
6 files changed, 185 insertions(+), 3 deletions(-)
--- a/arch/arm64/include/asm/atomic_ll_sc.h
+++ b/arch/arm64/include/asm/atomic_ll_sc.h
@@ -326,6 +326,44 @@ __CMPXCHG_DBL( , , , )
__CMPXCHG_DBL(_mb, dmb ish, l, "memory")
#undef __CMPXCHG_DBL
+
+union __u128_halves {
+ u128 full;
+ struct {
+ u64 low, high;
+ };
+};
+
+#define __CMPXCHG128(name, mb, rel, cl) \
+static __always_inline u128 \
+__ll_sc__cmpxchg128##name(volatile u128 *ptr, u128 old, u128 new) \
+{ \
+ union __u128_halves r, o = { .full = (old) }, \
+ n = { .full = (new) }; \
+ \
+ asm volatile("// __cmpxchg128" #name "\n" \
+ " prfm pstl1strm, %2\n" \
+ "1: ldxp %0, %1, %2\n" \
+ " eor %3, %0, %3\n" \
+ " eor %4, %1, %4\n" \
+ " orr %3, %4, %3\n" \
+ " cbnz %3, 2f\n" \
+ " st" #rel "xp %w3, %5, %6, %2\n" \
+ " cbnz %w3, 1b\n" \
+ " " #mb "\n" \
+ "2:" \
+ : "=&r" (r.low), "=&r" (r.high), "+Q" (*(unsigned long *)ptr) \
I wonder whether we should use "(*(u128 *)ptr)" instead of "(*(unsigned
long *) ptr)"? Because compilers may think only 64bit value pointed by
"ptr" gets modified, and they are allowed to do "useful" optimization.
In this I've copied the existing cmpxchg_double() code; I'll have to let
the arch folks speak here, I've no clue.
We definitely need to ensure the compiler sees we poke the whole thing, or it
can get this horribly wrong, so that is a latent bug.
See commit:
fee960bed5e857eb ("arm64: xchg: hazard against entire exchange variable")
... for examples of GCC being clever, where I overlooked the *_double() cases.
Ugh; with GCC 12.1.0, arm64 defconfig, and the following:
| struct big {
| u64 lo, hi;
| } __aligned(128);
|
| unsigned long foo(struct big *b)
| {
| u64 hi_old, hi_new;
|
| hi_old = b->hi;
|
| cmpxchg_double_local(&b->lo, &b->hi, 0x12, 0x34, 0x56, 0x78);
|
| hi_new = b->hi;
|
| return hi_old ^ hi_new;
| }
GCC clearly figures out the high half isn't modified, and constant folds hi_old
^ hi_new down to zero, regardless of whether we use LL/SC or LSE:
<foo>:
0: d503233f paciasp
4: aa0003e4 mov x4, x0
8: 1400000e b 40 <foo+0x40>
c: d2800240 mov x0, #0x12 // #18
10: d2800681 mov x1, #0x34 // #52
14: aa0003e5 mov x5, x0
18: aa0103e6 mov x6, x1
1c: d2800ac2 mov x2, #0x56 // #86
20: d2800f03 mov x3, #0x78 // #120
24: 48207c82 casp x0, x1, x2, x3, [x4]
28: ca050000 eor x0, x0, x5
2c: ca060021 eor x1, x1, x6
30: aa010000 orr x0, x0, x1
34: d2800000 mov x0, #0x0 // #0 <--- BANG
38: d50323bf autiasp
3c: d65f03c0 ret
40: d2800240 mov x0, #0x12 // #18
44: d2800681 mov x1, #0x34 // #52
48: d2800ac2 mov x2, #0x56 // #86
4c: d2800f03 mov x3, #0x78 // #120
50: f9800091 prfm pstl1strm, [x4]
54: c87f1885 ldxp x5, x6, [x4]
58: ca0000a5 eor x5, x5, x0
5c: ca0100c6 eor x6, x6, x1
60: aa0600a6 orr x6, x5, x6
64: b5000066 cbnz x6, 70 <foo+0x70>
68: c8250c82 stxp w5, x2, x3, [x4]
6c: 35ffff45 cbnz w5, 54 <foo+0x54>
70: d2800000 mov x0, #0x0 // #0 <--- BANG
74: d50323bf autiasp
78: d65f03c0 ret
7c: d503201f nop
... so we *definitely* need to fix that.
Using __uint128_t instead, e.g.
diff --git a/arch/arm64/include/asm/atomic_ll_sc.h b/arch/arm64/include/asm/atomic_ll_sc.h
index 0890e4f568fb7..cbb3d961123b1 100644
--- a/arch/arm64/include/asm/atomic_ll_sc.h
+++ b/arch/arm64/include/asm/atomic_ll_sc.h
@@ -315,7 +315,7 @@ __ll_sc__cmpxchg_double##name(unsigned long old1, \
" cbnz %w0, 1b\n" \
" " #mb "\n" \
"2:" \
- : "=&r" (tmp), "=&r" (ret), "+Q" (*(unsigned long *)ptr) \
+ : "=&r" (tmp), "=&r" (ret), "+Q" (*(__uint128_t *)ptr) \
: "r" (old1), "r" (old2), "r" (new1), "r" (new2) \
: cl); \
\diff --git a/arch/arm64/include/asm/atomic_lse.h b/arch/arm64/include/asm/atomic_lse.h
index 52075e93de6c0..a94d6dacc0292 100644
--- a/arch/arm64/include/asm/atomic_lse.h
+++ b/arch/arm64/include/asm/atomic_lse.h
@@ -311,7 +311,7 @@ __lse__cmpxchg_double##name(unsigned long old1, \
" eor %[old2], %[old2], %[oldval2]\n" \
" orr %[old1], %[old1], %[old2]" \
: [old1] "+&r" (x0), [old2] "+&r" (x1), \
- [v] "+Q" (*(unsigned long *)ptr) \
+ [v] "+Q" (*(__uint128_t *)ptr) \
: [new1] "r" (x2), [new2] "r" (x3), [ptr] "r" (x4), \
[oldval1] "r" (oldval1), [oldval2] "r" (oldval2) \
: cl); \
... makes GCC much happier:
<foo>:
0: f9400407 ldr x7, [x0, #8]
4: d503233f paciasp
8: aa0003e4 mov x4, x0
c: 1400000f b 48 <foo+0x48>
10: d2800240 mov x0, #0x12 // #18
14: d2800681 mov x1, #0x34 // #52
18: aa0003e5 mov x5, x0
1c: aa0103e6 mov x6, x1
20: d2800ac2 mov x2, #0x56 // #86
24: d2800f03 mov x3, #0x78 // #120
28: 48207c82 casp x0, x1, x2, x3, [x4]
2c: ca050000 eor x0, x0, x5
30: ca060021 eor x1, x1, x6
34: aa010000 orr x0, x0, x1
38: f9400480 ldr x0, [x4, #8]
3c: d50323bf autiasp
40: ca0000e0 eor x0, x7, x0
44: d65f03c0 ret
48: d2800240 mov x0, #0x12 // #18
4c: d2800681 mov x1, #0x34 // #52
50: d2800ac2 mov x2, #0x56 // #86
54: d2800f03 mov x3, #0x78 // #120
58: f9800091 prfm pstl1strm, [x4]
5c: c87f1885 ldxp x5, x6, [x4]
60: ca0000a5 eor x5, x5, x0
64: ca0100c6 eor x6, x6, x1
68: aa0600a6 orr x6, x5, x6
6c: b5000066 cbnz x6, 78 <foo+0x78>
70: c8250c82 stxp w5, x2, x3, [x4]
74: 35ffff45 cbnz w5, 5c <foo+0x5c>
78: f9400480 ldr x0, [x4, #8]
7c: d50323bf autiasp
80: ca0000e0 eor x0, x7, x0
84: d65f03c0 ret
88: d503201f nop
8c: d503201f nop
... I'll go check whether clang is happy with that, and how far back that can
go, otherwise we'll need to blat the high half with a separate constaint that
(ideally) doesn't end up allocating a pointless address register.
Mark.