Messages in this thread Patch in this message | | | Date | Fri, 6 Jan 2023 15:11:58 +0100 | From | Jules Maselbas <> | Subject | Re: [RFC PATCH 05/25] kvx: Add atomic/locking headers |
| |
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@kernel.org> > > CC: Peter Zijlstra <peterz@infradead.org> > > CC: Boqun Feng <boqun.feng@gmail.com> > > CC: Mark Rutland <mark.rutland@arm.com> > > CC: linux-kernel@vger.kernel.org > > Co-developed-by: Clement Leger <clement.leger@bootlin.com> > > Signed-off-by: Clement Leger <clement.leger@bootlin.com> > > Co-developed-by: Jules Maselbas <jmaselbas@kalray.eu> > > Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu> > > Co-developed-by: Julian Vetter <jvetter@kalray.eu> > > Signed-off-by: Julian Vetter <jvetter@kalray.eu> > > Co-developed-by: Julien Villette <jvillette@kalray.eu> > > Signed-off-by: Julien Villette <jvillette@kalray.eu> > > Co-developed-by: Yann Sionneau <ysionneau@kalray.eu> > > Signed-off-by: Yann Sionneau <ysionneau@kalray.eu> > > --- > > 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
> 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.
> > > +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.
> > + > > +#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. > > > + > > +#include <asm-generic/atomic.h> > > + > > +#endif /* _ASM_KVX_ATOMIC_H */ > > diff --git a/arch/kvx/include/asm/barrier.h b/arch/kvx/include/asm/barrier.h > > new file mode 100644 > > index 000000000000..371f1c70746d > > --- /dev/null > > +++ b/arch/kvx/include/asm/barrier.h > > @@ -0,0 +1,15 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > +/* > > + * Copyright (C) 2017-2023 Kalray Inc. > > + * Author(s): Clement Leger > > + */ > > + > > +#ifndef _ASM_KVX_BARRIER_H > > +#define _ASM_KVX_BARRIER_H > > > +/* Bitmask modifiers */ > > +#define __NOP(x) (x) > > +#define __NOT(x) (~(x)) > > + > > + > > +#define __test_and_op_bit(nr, addr, op, mod) \ > > +({ \ > > + unsigned long __mask = BIT_MASK(nr); \ > > + unsigned long __new, __old, __ret; \ > > + do { \ > > + __old = *(&addr[BIT_WORD(nr)]); \ > > + __new = __old op mod(__mask); \ > > + __ret = cmpxchg(addr, __old, __new); \ > > + } while (__ret != __old); \ > > + (__old & __mask); \ > > +}) > > Please use <asm-generic/bitops/atomic.h> which should give you the common > bit operations "for free" atop your regular atomics. Yes
> > [...] > > > 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.
> > + __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
Thanks, -- Jules
| |