Re: [PATCH v2 1/2] xen/arm: Remove cmpxchg_local() and drop _mb from the other helpers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 11 Sep 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> The current set of helpers are quite confusing to follow as the naming
> is not very consistent.
> 
> Given that cmpxchg_local() is not used in Xen, drop it completely.
> Furthermore, adopt a naming with _mb so all names are now consistent.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
>     Changes in v2:
>         - Patch added
> ---
>  xen/include/asm-arm/arm32/cmpxchg.h | 31 ++++++-----------------
>  xen/include/asm-arm/arm64/cmpxchg.h | 38 +++++++----------------------
>  xen/include/asm-arm/guest_atomics.h |  6 ++---
>  3 files changed, 19 insertions(+), 56 deletions(-)
> 
> diff --git a/xen/include/asm-arm/arm32/cmpxchg.h 
> b/xen/include/asm-arm/arm32/cmpxchg.h
> index 0770f272ee99..3ef1e5c63276 100644
> --- a/xen/include/asm-arm/arm32/cmpxchg.h
> +++ b/xen/include/asm-arm/arm32/cmpxchg.h
> @@ -112,23 +112,12 @@ static always_inline unsigned long __cmpxchg(volatile 
> void *ptr,
>                                            unsigned long new,
>                                            int size)
>  {
> +     smp_mb();
>       if (!__int_cmpxchg(ptr, &old, new, size, false, 0))
>               ASSERT_UNREACHABLE();
> -
> -     return old;
> -}
> -
> -static always_inline unsigned long __cmpxchg_mb(volatile void *ptr,
> -                                                unsigned long old,
> -                                                unsigned long new, int size)
> -{
> -     unsigned long ret;
> -
> -     smp_mb();
> -     ret = __cmpxchg(ptr, old, new, size);
>       smp_mb();
>  
> -     return ret;
> +     return old;
>  }
>  
>  /*
> @@ -141,11 +130,11 @@ static always_inline unsigned long 
> __cmpxchg_mb(volatile void *ptr,
>   * The helper will return true when the update has succeeded (i.e no
>   * timeout) and false if the update has failed.
>   */
> -static always_inline bool __cmpxchg_mb_timeout(volatile void *ptr,
> -                                            unsigned long *old,
> -                                            unsigned long new,
> -                                            int size,
> -                                            unsigned int max_try)
> +static always_inline bool __cmpxchg_timeout(volatile void *ptr,
> +                                         unsigned long *old,
> +                                         unsigned long new,
> +                                         int size,
> +                                         unsigned int max_try)
>  {
>       bool ret;
>  
> @@ -157,12 +146,6 @@ static always_inline bool __cmpxchg_mb_timeout(volatile 
> void *ptr,
>  }
>  
>  #define cmpxchg(ptr,o,n)                                             \
> -     ((__typeof__(*(ptr)))__cmpxchg_mb((ptr),                        \
> -                                       (unsigned long)(o),           \
> -                                       (unsigned long)(n),           \
> -                                       sizeof(*(ptr))))
> -
> -#define cmpxchg_local(ptr,o,n)                                               
> \
>       ((__typeof__(*(ptr)))__cmpxchg((ptr),                           \
>                                      (unsigned long)(o),              \
>                                      (unsigned long)(n),              \
> diff --git a/xen/include/asm-arm/arm64/cmpxchg.h 
> b/xen/include/asm-arm/arm64/cmpxchg.h
> index fc5c60f0bd74..f4a8c1ccba80 100644
> --- a/xen/include/asm-arm/arm64/cmpxchg.h
> +++ b/xen/include/asm-arm/arm64/cmpxchg.h
> @@ -125,23 +125,12 @@ static always_inline unsigned long __cmpxchg(volatile 
> void *ptr,
>                                            unsigned long new,
>                                            int size)
>  {
> +     smp_mb();
>       if (!__int_cmpxchg(ptr, &old, new, size, false, 0))
>               ASSERT_UNREACHABLE();
> -
> -     return old;
> -}
> -
> -static always_inline unsigned long __cmpxchg_mb(volatile void *ptr,
> -                                             unsigned long old,
> -                                             unsigned long new, int size)
> -{
> -     unsigned long ret;
> -
> -     smp_mb();
> -     ret = __cmpxchg(ptr, old, new, size);
>       smp_mb();
>  
> -     return ret;
> +     return old;
>  }
>  
>  /*
> @@ -154,11 +143,11 @@ static always_inline unsigned long 
> __cmpxchg_mb(volatile void *ptr,
>   * The helper will return true when the update has succeeded (i.e no
>   * timeout) and false if the update has failed.
>   */
> -static always_inline bool __cmpxchg_mb_timeout(volatile void *ptr,
> -                                            unsigned long *old,
> -                                            unsigned long new,
> -                                            int size,
> -                                            unsigned int max_try)
> +static always_inline bool __cmpxchg_timeout(volatile void *ptr,
> +                                         unsigned long *old,
> +                                         unsigned long new,
> +                                         int size,
> +                                         unsigned int max_try)
>  {
>       bool ret;
>  
> @@ -173,17 +162,8 @@ static always_inline bool __cmpxchg_mb_timeout(volatile 
> void *ptr,
>  ({ \
>       __typeof__(*(ptr)) __ret; \
>       __ret = (__typeof__(*(ptr))) \
> -             __cmpxchg_mb((ptr), (unsigned long)(o), (unsigned long)(n), \
> -                          sizeof(*(ptr))); \
> -     __ret; \
> -})
> -
> -#define cmpxchg_local(ptr, o, n) \
> -({ \
> -     __typeof__(*(ptr)) __ret; \
> -     __ret = (__typeof__(*(ptr))) \
> -             __cmpxchg((ptr), (unsigned long)(o), \
> -                       (unsigned long)(n), sizeof(*(ptr))); \
> +             __cmpxchg((ptr), (unsigned long)(o), (unsigned long)(n), \
> +                       sizeof(*(ptr))); \
>       __ret; \
>  })
>  
> diff --git a/xen/include/asm-arm/guest_atomics.h 
> b/xen/include/asm-arm/guest_atomics.h
> index af27cc627bf3..20347849e56c 100644
> --- a/xen/include/asm-arm/guest_atomics.h
> +++ b/xen/include/asm-arm/guest_atomics.h
> @@ -96,14 +96,14 @@ static inline unsigned long __guest_cmpxchg(struct domain 
> *d,
>  
>      perfc_incr(atomics_guest);
>  
> -    if ( __cmpxchg_mb_timeout(ptr, &oldval, new, size,
> -                              this_cpu(guest_safe_atomic_max)) )
> +    if ( __cmpxchg_timeout(ptr, &oldval, new, size,
> +                           this_cpu(guest_safe_atomic_max)) )
>          return oldval;
>  
>      perfc_incr(atomics_guest_paused);
>  
>      domain_pause_nosync(d);
> -    oldval = __cmpxchg_mb(ptr, old, new, size);
> +    oldval = __cmpxchg(ptr, old, new, size);
>      domain_unpause(d);
>  
>      return oldval;
> -- 
> 2.17.1
>