Re: [PATCH 1/5] riscv: Coding convention for xchg

Peter Zijlstra
2020-11-24T14:29:52Z

On Tue, Nov 24, 2020 at 01:43:53PM +0000, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
>
> This is prepare for QUEUED_SPINLOCKS which need xchg support short
> type value.
> - Remove unused codes (xchg32, xchg64, cmpxchg32 ...)
> - Combine xchg_relaxed, xchg_acquire, xchg_release into one asm
> - Make atomic.aq/rl with seperated fence acquire & release

Every time you find yourself doing multiple things, make it multiple
patches.

> @@ -242,58 +239,58 @@ static __always_inline s64 atomic64_fetch_add_unless(atomic64_t *v, s64 a, s64 u
> * atomic_{cmp,}xchg is required to have exactly the same ordering semantics as
> * {cmp,}xchg and the operations that return, so they need a full barrier.
> */
> +#define ATOMIC_OP(c_t, prefix) \
> static __always_inline \
> c_t atomic##prefix##_xchg_relaxed(atomic##prefix##_t *v, c_t n) \
> { \
> + return xchg_relaxed(&(v->counter), n); \
> } \
> static __always_inline \
> c_t atomic##prefix##_xchg_acquire(atomic##prefix##_t *v, c_t n) \
> { \
> + return xchg_acquire(&(v->counter), n); \
> } \
> static __always_inline \
> c_t atomic##prefix##_xchg_release(atomic##prefix##_t *v, c_t n) \
> { \
> + return xchg_release(&(v->counter), n); \
> } \
> static __always_inline \
> c_t atomic##prefix##_xchg(atomic##prefix##_t *v, c_t n) \
> { \
> + return xchg(&(v->counter), n); \
> } \
> static __always_inline \
> c_t atomic##prefix##_cmpxchg_relaxed(atomic##prefix##_t *v, \
> c_t o, c_t n) \
> { \
> + return cmpxchg_relaxed(&(v->counter), o, n); \
> } \
> static __always_inline \
> c_t atomic##prefix##_cmpxchg_acquire(atomic##prefix##_t *v, \
> c_t o, c_t n) \
> { \
> + return cmpxchg_acquire(&(v->counter), o, n); \
> } \
> static __always_inline \
> c_t atomic##prefix##_cmpxchg_release(atomic##prefix##_t *v, \
> c_t o, c_t n) \
> { \
> + return cmpxchg_release(&(v->counter), o, n); \
> } \
> static __always_inline \
> c_t atomic##prefix##_cmpxchg(atomic##prefix##_t *v, c_t o, c_t n) \
> { \
> + return cmpxchg(&(v->counter), o, n); \
> }

> diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
> index 262e5bb..5609185 100644
> --- a/arch/riscv/include/asm/cmpxchg.h
> +++ b/arch/riscv/include/asm/cmpxchg.h
> @@ -44,118 +44,31 @@
> _x_, sizeof(*(ptr))); \
> })
>
> #define xchg_acquire(ptr, x) \
> ({ \
> + __typeof__(*(ptr)) _x_ = (x); \
> + __ret = __xchg_relaxed((ptr), _x_, sizeof(*(ptr))); \
> + __acquire_fence(); \
> __ret; \
> })
>
> #define xchg_release(ptr, x) \
> ({ \
> __typeof__(*(ptr)) _x_ = (x); \
> + __release_fence(); \
> + (__typeof__(*(ptr))) __xchg_relaxed((ptr), \
> _x_, sizeof(*(ptr))); \
> })
>
> #define xchg(ptr, x) \
> ({ \
> + __typeof__(*(ptr)) __ret; \
> __typeof__(*(ptr)) _x_ = (x); \
> + __smp_mb(); \
> + __ret = __xchg_relaxed((ptr), _x_, sizeof(*(ptr))); \
> + __smp_mb(); \
> + __ret; \
> })
>
> /*

Why are you defining *{,_acquire,_release}() at all, doesn't
atomic-fallback.h DTRT for you?