Re: [RFC PATCH 05/25] kvx: Add atomic/locking headers
From: Yann Sionneau
Date: Wed Jan 18 2023 - 09:04:41 EST
Hi Mark,
On 10/01/2023 14:24, Mark Rutland wrote:
On Fri, Jan 06, 2023 at 03:11:58PM +0100, Jules Maselbas wrote:
Hi Mark,
On Wed, Jan 04, 2023 at 09:53:24AM +0000, Mark Rutland wrote:
On Tue, Jan 03, 2023 at 05:43:39PM +0100, Yann Sionneau wrote:
Add common headers (atomic, bitops, barrier and locking) for basic
kvx support.
CC: Will Deacon <will@xxxxxxxxxx>
CC: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
CC: Boqun Feng <boqun.feng@xxxxxxxxx>
CC: Mark Rutland <mark.rutland@xxxxxxx>
CC: linux-kernel@xxxxxxxxxxxxxxx
Co-developed-by: Clement Leger <clement.leger@xxxxxxxxxxx>
Signed-off-by: Clement Leger <clement.leger@xxxxxxxxxxx>
Co-developed-by: Jules Maselbas <jmaselbas@xxxxxxxxx>
Signed-off-by: Jules Maselbas <jmaselbas@xxxxxxxxx>
Co-developed-by: Julian Vetter <jvetter@xxxxxxxxx>
Signed-off-by: Julian Vetter <jvetter@xxxxxxxxx>
Co-developed-by: Julien Villette <jvillette@xxxxxxxxx>
Signed-off-by: Julien Villette <jvillette@xxxxxxxxx>
Co-developed-by: Yann Sionneau <ysionneau@xxxxxxxxx>
Signed-off-by: Yann Sionneau <ysionneau@xxxxxxxxx>
---
arch/kvx/include/asm/atomic.h | 104 +++++++++++++++++
arch/kvx/include/asm/barrier.h | 15 +++
arch/kvx/include/asm/bitops.h | 207 +++++++++++++++++++++++++++++++++
arch/kvx/include/asm/bitrev.h | 32 +++++
arch/kvx/include/asm/cmpxchg.h | 185 +++++++++++++++++++++++++++++
5 files changed, 543 insertions(+)
create mode 100644 arch/kvx/include/asm/atomic.h
create mode 100644 arch/kvx/include/asm/barrier.h
create mode 100644 arch/kvx/include/asm/bitops.h
create mode 100644 arch/kvx/include/asm/bitrev.h
create mode 100644 arch/kvx/include/asm/cmpxchg.h
diff --git a/arch/kvx/include/asm/atomic.h b/arch/kvx/include/asm/atomic.h
new file mode 100644
index 000000000000..eb8acbcbc70d
--- /dev/null
+++ b/arch/kvx/include/asm/atomic.h
@@ -0,0 +1,104 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2017-2023 Kalray Inc.
+ * Author(s): Clement Leger
+ */
+
+#ifndef _ASM_KVX_ATOMIC_H
+#define _ASM_KVX_ATOMIC_H
+
+#include <linux/types.h>
+
+#include <asm/cmpxchg.h>
+
+#define ATOMIC64_INIT(i) { (i) }
+
+#define arch_atomic64_cmpxchg(v, old, new) (arch_cmpxchg(&((v)->counter), old, new))
+#define arch_atomic64_xchg(v, new) (arch_xchg(&((v)->counter), new))
+
+static inline long arch_atomic64_read(const atomic64_t *v)
+{
+ return v->counter;
+}
This is a plain read, and is *not* atomic.
The compiler can replay a plain read an arbitrary number of times, and is
permitted to split it into smaller accesses.
At minimum this needs to be
READ_ONCE(v->counter)
... which will prevent replay. Whether or not that's actually atomic will
depend on the instructions the compiler generates, and how those instructions
are defines in your architecture.
Good point, we are going to use {READ,WRITE}_ONCE macros
Done for V2 (which we will send pretty soon).
Do you have a single instruction that can read a 64-bit memory location, and is
it guaranteed to result in a single access that cannot be split?
We do have a single instruction that can read a 64-bit memory location
(supported sizes are 8, 16, 32, 64, 128, 256).
All accesses are guaranteed to not be split, unless they are misaligned.
Furthermore, misaligned write accesses crossing a 32-byte boundary may
complete in a non-atomic way.
Perfect, thanks for confirming!
[...]
+static inline void arch_atomic64_set(atomic64_t *v, long i)
+{
+ v->counter = i;
+}
Same comments as for arch_atomic64_read(); at minimum this needs to be:
WRITE_ONCE(v->counter, i)
... but that may or may not actually be atomic on your architecture.
+#define ATOMIC64_RETURN_OP(op, c_op) \
+static inline long arch_atomic64_##op##_return(long i, atomic64_t *v) \
+{ \
+ long new, old, ret; \
+ \
+ do { \
+ old = v->counter; \
+ new = old c_op i; \
+ ret = arch_cmpxchg(&v->counter, old, new); \
+ } while (ret != old); \
+ \
+ return new; \
+}
+
+#define ATOMIC64_OP(op, c_op) \
+static inline void arch_atomic64_##op(long i, atomic64_t *v) \
+{ \
+ long new, old, ret; \
+ \
+ do { \
+ old = v->counter; \
+ new = old c_op i; \
+ ret = arch_cmpxchg(&v->counter, old, new); \
+ } while (ret != old); \
+}
+
+#define ATOMIC64_FETCH_OP(op, c_op) \
+static inline long arch_atomic64_fetch_##op(long i, atomic64_t *v) \
+{ \
+ long new, old, ret; \
+ \
+ do { \
+ old = v->counter; \
+ new = old c_op i; \
+ ret = arch_cmpxchg(&v->counter, old, new); \
+ } while (ret != old); \
+ \
+ return old; \
+}
These look ok, but it'd be nicer if we could teach the generic atomic64 code to
do this, like the generic atomic code does.
We could rename the existing asm-generic/atomic64 code to atomic64-spinlock,
and add a separate atomic64-cmpxchg (and likewise for the 32-bit code) to make
that clearer and consistent.
I am not sure what this implies and how big this change might be,
but I'll take a look at this.
Hmm... from a quick attempt just now it looks like that will be a bit more
churny than I thought.
We can always factor this out later, so feel free to leave it as-is, thgouh if
we could make this genric (and have it look like asm-generic/atomic.h), it'd be
nice for consistency and maintenance.
Ack for doing it later.
+
+#define ATOMIC64_OPS(op, c_op) \
+ ATOMIC64_OP(op, c_op) \
+ ATOMIC64_RETURN_OP(op, c_op) \
+ ATOMIC64_FETCH_OP(op, c_op)
+
+ATOMIC64_OPS(and, &)
+ATOMIC64_OPS(or, |)
+ATOMIC64_OPS(xor, ^)
+ATOMIC64_OPS(add, +)
+ATOMIC64_OPS(sub, -)
+
+#undef ATOMIC64_OPS
+#undef ATOMIC64_FETCH_OP
+#undef ATOMIC64_OP
+
+static inline int arch_atomic_add_return(int i, atomic_t *v)
+{
+ int new, old, ret;
+
+ do {
+ old = v->counter;
+ new = old + i;
+ ret = arch_cmpxchg(&v->counter, old, new);
+ } while (ret != old);
+
+ return new;
+}
+
+static inline int arch_atomic_sub_return(int i, atomic_t *v)
+{
+ return arch_atomic_add_return(-i, v);
+}
Likewise for these two.
The 32-bit atomics should come from asm-generic/atomic.h and not be necesary,
that has both arch_atomic_add_return() and arch_atomic_sub_return().
diff --git a/arch/kvx/include/asm/cmpxchg.h b/arch/kvx/include/asm/cmpxchg.h
new file mode 100644
index 000000000000..b1d128b060a2
--- /dev/null
+++ b/arch/kvx/include/asm/cmpxchg.h
@@ -0,0 +1,185 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2017-2023 Kalray Inc.
+ * Author(s): Clement Leger
+ * Yann Sionneau
+ */
+
+#ifndef _ASM_KVX_CMPXCHG_H
+#define _ASM_KVX_CMPXCHG_H
+
+#include <linux/bits.h>
+#include <linux/types.h>
+#include <linux/build_bug.h>
+
+/*
+ * On kvx, we have a boolean compare and swap which means that the operation
+ * returns only the success of operation.
+ * If operation succeed, this is simple, we just need to return the provided
+ * old value. However, if it fails, we need to load the value to return it for
+ * the caller. If the loaded value is different from the "old" provided by the
+ * caller, we can return it since it will means it failed.
+ * However, if for some reason the value we read is equal to the old value
+ * provided by the caller, we can't simply return it or the caller will think it
+ * succeeded. So if the value we read is the same as the "old" provided by
+ * the caller, we try again until either we succeed or we fail with a different
+ * value than the provided one.
+ */
+#define __cmpxchg(ptr, old, new, op_suffix, load_suffix) \
+({ \
+ register unsigned long __rn asm("r62"); \
+ register unsigned long __ro asm("r63"); \
Why do you need to specify the exact registers?
r62 and r63 are hardcoded in the inline assembly, they are caller saved.
I have a C implementation that uses builtins however this is not merged
in our tree yet (but I want to).
e.g. does some instruction use these implicitly, or do you need two adjacent
register for encoding reasons?
The atomic compare and swap (acswap) instruction needs a register "pair"
which can only exists with "adjacent" registers: $r0r1, $r2r3 ect.
Ok; and you don't have a way to ask GCC for an arbitrary register pair, so you
chose r62+r63 as they can be clobbered?
It might be worth looking into using an __int128_t to give you a register pair,
assuming your calling convention mandares adjacency of the two halves. That
could give you the pair while giving the compiler freedom to chose any suitable
pair (assuming you have a suitable operand modifier to extract the low/high
registers from the asm operand.
In the V2 we replaced the assembly code with C code.
acswap{d,w} instruction is generated by the mean of a compiler builtin.
This way
the code is much more readable and it allows to not hard code some
register pair, letting the compiler allocate them.
+ __asm__ __volatile__ ( \
+ /* Fence to guarantee previous store to be committed */ \
+ "fence\n" \
This implies you can implement the relaxed form of cmpxchg().
What ordering do you get by default, and do you have any other barriers (e.g.
for acquire/release semantics), or just "fence" ?
We have two barrier types:
- fence: ensure that all uncached memory operations are committed to
memory, typically used to ensure a write to memory is visible to
other cores.
- barrier: flush the core instruction pipeline and memory system
Ok.
In the absence of barriers do you have any ordering guarantee? e.g. weak like
arm/power, TSO like x86, or something else entirely?
Our memory ordering model is weak.
```
store @data_addr = new_value
;;
store @data_is_valid_addr = 1
;;
```
By running the previous pseudo-code, there is no guarantee that another
CPU would see the *data_addr == new_value just because it sees
*data_is_valid_addr == 1
The correct code would need an extra bundle containing a `fence`
instruction in-between the 2 other bundles.
When you say "uncached memory operations", does that mean that you have cached
memory operatins which are not affected by the barriers, or is "uncached memory
operations" just meant to read as "oustanding/incomplete memory operations" ?
The fence instruction will force all outstanding memory write operations
to finish and reach their target.
* For an uncached access it means that after the fence the DDR has been
updated (or the target device register).
* For a cached access it means the L2 cache has been updated, and
therefore all the corresponding L1D lines invalidated. (L1I is untouched!)
Bottom line is that after the fence, all other cores of the cluster
running Linux will see the new value. Except of course if the write is
through an uncached MMU mapping and the read is done through a cached
MMU mapping, but this seems normal.
Thanks for the review, it's much appreciated!
--
Yann