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

Guo Ren
2020-11-25T14:19:01Z

On Tue, Nov 24, 2020 at 10:29 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> 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.
Ok.

>
> > @@ -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?
Yes, you are right. I could reuse that. Thx

#define cmpxchg_acquire(...) \
__atomic_op_acquire(cmpxchg, __VA_ARGS__)
#endif

#define __atomic_op_acquire(op, args...) \
({ \
typeof(op##_relaxed(args)) __ret = op##_relaxed(args); \
__atomic_acquire_fence(); \
__ret; \
})

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/