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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201124142905.GH2414@hirez.programming.kicks-ass.net>
Date:   Tue, 24 Nov 2020 15:29:05 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     guoren@...nel.org
Cc:     arnd@...db.de, palmerdabbelt@...gle.com, paul.walmsley@...ive.com,
        anup@...infault.org, linux-riscv@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-csky@...r.kernel.org,
        Guo Ren <guoren@...ux.alibaba.com>,
        Michael Clark <michaeljclark@....com>
Subject: Re: [PATCH 1/5] riscv: Coding convention for xchg

On Tue, Nov 24, 2020 at 01:43:53PM +0000, guoren@...nel.org wrote:
> From: Guo Ren <guoren@...ux.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?