Re: [PATCH 09/12] RISC-V: Atomic and Locking Routines

[Torvald Riegel <triegel at redhat dot com>]

On Wed, 2017-06-14 at 11:30 -0700, Palmer Dabbelt wrote:
> This patch implements various atomic and locking routines on RISC-V,
> either via the A extension (when present) or via a Linux system call
> that does a compare-and-exchange.  This contains both the library
> routines and the syscall wrapper.

In the overview email you seemed to say that you only support the HW
variants that have atomics.  If that's so, why don't you just check that
this is the case and produce a compile-time error if it isn't?

> diff --git a/sysdeps/riscv/atomic-machine.h b/sysdeps/riscv/atomic-machine.h
> new file mode 100644
> index 0000000000..c88dc1ce33
> --- /dev/null
> +++ b/sysdeps/riscv/atomic-machine.h
> @@ -0,0 +1,132 @@
> +/* Low-level functions for atomic operations. RISC-V version.
> +   Copyright (C) 2011-2016 Free Software Foundation, Inc.
> +
> +   Contributed by Andrew Waterman (andrew@sifive.com).
> +
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, write to the Free
> +   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> +   02111-1307 USA.  */
> +
> +#ifndef _RISCV_BITS_ATOMIC_H
> +#define _RISCV_BITS_ATOMIC_H 1
> +
> +#include <inttypes.h>
> +
> +typedef int32_t atomic32_t;
> +typedef uint32_t uatomic32_t;
> +typedef int_fast32_t atomic_fast32_t;
> +typedef uint_fast32_t uatomic_fast32_t;
> +
> +typedef int64_t atomic64_t;
> +typedef uint64_t uatomic64_t;
> +typedef int_fast64_t atomic_fast64_t;
> +typedef uint_fast64_t uatomic_fast64_t;

I believe the *fast* types aren't used anymore (yes we should clean that
up).

> +typedef intptr_t atomicptr_t;
> +typedef uintptr_t uatomicptr_t;
> +typedef intmax_t atomic_max_t;
> +typedef uintmax_t uatomic_max_t;
> +
> +#define atomic_full_barrier() __sync_synchronize()
> +
> +#ifdef __riscv_atomic
> +
> +#define __HAVE_64B_ATOMICS (__riscv_xlen >= 64)
> +#define USE_ATOMIC_COMPILER_BUILTINS 1

If the built-ins do what you want them to do, I suggest that you
implement the old-style glibc atomics using the compiler built-ins (see
aarch64 for example).

> +/* MIPS uses swap on machines that have it, and uses CAS on machines that

Refer to something else than MIPS I'd suggest, or clarify the link.

> + * don't.  This, we use amoswap when the A extension is enabled, and fall back
> + * to the atomic system call when the A extension is disabled.  */

That's not what the code does.

> +#ifdef __riscv_atomic

You are already in a__riscv_atomic #ifdef (see above).

If you assume that your HW is of the variant with atomics, then you
don't need to fall back to the kernel helper, or do you?

> +# define ATOMIC_EXCHANGE_USES_CAS 0
> +#else
> +# define ATOMIC_EXCHANGE_USES_CAS 1

The code for the old-style atomics doesn't seem to define an exchange;
do the compiler builtins have one?

> +#endif
> +
> +#define asm_amo(which, ordering, mem, value) ({ 		\
> +  typeof(*mem) __tmp; 						\
> +  if (sizeof(__tmp) == 4)					\
> +    asm volatile (which ".w" ordering "\t%0, %z2, %1"		\
> +		  : "=r"(__tmp), "+A"(*(mem))			\
> +		  : "rJ"(value));				\
> +  else if (sizeof(__tmp) == 8)					\
> +    asm volatile (which ".d" ordering "\t%0, %z2, %1"		\
> +		  : "=r"(__tmp), "+A"(*(mem))			\
> +		  : "rJ"(value));				\
> +  else								\
> +    abort();							\
> +  __tmp; })
> +
> +/* Atomic compare and exchange. */
> +
> +#define atomic_cas(ordering, mem, newval, oldval) ({ 		\
> +  typeof(*mem) __tmp; 						\
> +  int __tmp2; 							\
> +  if (sizeof(__tmp) == 4)					\
> +    asm volatile ("1: lr.w" ordering "\t%0, %2\n"		\
> +		  "bne\t%0, %z4, 1f\n"				\
> +		  "sc.w" ordering "\t%1, %z3, %2\n"		\
> +		  "bnez\t%1, 1b\n"				\
> +		  "1:"						\
> +		  : "=&r"(__tmp), "=&r"(__tmp2), "+A"(*(mem))	\
> +		  : "rJ"(newval), "rJ"(oldval));		\
> +  else if (sizeof(__tmp) == 8)					\
> +    asm volatile ("1: lr.d" ordering "\t%0, %2\n"		\
> +		  "bne\t%0, %z4, 1f\n"				\
> +		  "sc.d" ordering "\t%1, %z3, %2\n"		\
> +		  "bnez\t%1, 1b\n"				\
> +		  "1:"						\
> +		  : "=&r"(__tmp), "=&r"(__tmp2), "+A"(*(mem))	\
> +		  : "rJ"(newval), "rJ"(oldval));		\
> +  else								\
> +    abort();							\
> +  __tmp; })
> +
> +#define atomic_compare_and_exchange_val_acq(mem, newval, oldval) \
> +  atomic_cas(".aq", mem, newval, oldval)
> +
> +#define atomic_compare_and_exchange_val_rel(mem, newval, oldval) \
> +  atomic_cas(".rl", mem, newval, oldval)
> +
> +/* Atomic exchange (without compare).  */
> +
> +#define atomic_exchange_acq(mem, value) asm_amo("amoswap", ".aq", mem, value)
> +#define atomic_exchange_rel(mem, value) asm_amo("amoswap", ".rl", mem, value)
> +
> +
> +/* Atomically add value and return the previous (unincremented) value.  */
> +
> +#define atomic_exchange_and_add(mem, value) asm_amo("amoadd", "", mem, value)

The semantics of these old-style atomics aren't really well defined in
glibc (but we're moving to C11-like atomics anyway).  The default is for
atomic_exchange_and_add to be the same as atomic_exchange_and_add_acq,
see for example include/atomic.h:

#ifndef atomic_exchange_and_add_acq
# ifdef atomic_exchange_and_add
#  define atomic_exchange_and_add_acq(mem, value) \
  atomic_exchange_and_add (mem, value)

I suggest you keep it that way, instead of making them have relaxed MO.

This will go away once we've completed the transition to C11 atomics.

> +
> +#define atomic_max(mem, value) asm_amo("amomaxu", "", mem, value)
> +#define atomic_min(mem, value) asm_amo("amominu", "", mem, value)
> +
> +#define atomic_bit_test_set(mem, bit)                   \
> +  ({ typeof(*mem) __mask = (typeof(*mem))1 << (bit);    \
> +     asm_amo("amoor", "", mem, __mask) & __mask; })

Likewise.

> +#define catomic_exchange_and_add(mem, value)		\
> +  atomic_exchange_and_add(mem, value)
> +#define catomic_max(mem, value) atomic_max(mem, value)
> +
> +#else /* __riscv_atomic */
> +
> +#define __HAVE_64B_ATOMICS 0
> +#define USE_ATOMIC_COMPILER_BUILTINS 0

You need to provide at least a CAS to make this work.  Having this file
be included from the linux-specific variant is confusing, I think.  Do
you really need to support glibc not running on a Linux kernel?
Otherwise, perhaps just use the linux-specific atomic-machine.h? 

> +
> +#endif /* !__riscv_atomic */
> +
> +#endif /* bits/atomic.h */
> diff --git a/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h b/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> new file mode 100644
> index 0000000000..4473b0891a
> --- /dev/null
> +++ b/sysdeps/riscv/nptl/bits/pthreadtypes-arch.h
> @@ -0,0 +1,98 @@
> +/* Machine-specific pthread type layouts.  RISC-V version.
> +   Copyright (C) 2011-2014, 2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, write to the Free
> +   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> +   02111-1307 USA.  */
> +
> +#ifndef _BITS_PTHREADTYPES_ARCH_H
> +#define _BITS_PTHREADTYPES_ARCH_H	1
> +
> +#include <endian.h>
> +
> +#if __riscv_xlen == 64
> +# define __SIZEOF_PTHREAD_ATTR_T 56
> +# define __SIZEOF_PTHREAD_MUTEX_T 40
> +# define __SIZEOF_PTHREAD_MUTEXATTR_T 4
> +# define __SIZEOF_PTHREAD_COND_T 48
> +# define __SIZEOF_PTHREAD_CONDATTR_T 4
> +# define __SIZEOF_PTHREAD_RWLOCK_T 56
> +# define __SIZEOF_PTHREAD_RWLOCKATTR_T 8
> +# define __SIZEOF_PTHREAD_BARRIER_T 32
> +# define __SIZEOF_PTHREAD_BARRIERATTR_T 4
> +#else
> +# define __SIZEOF_PTHREAD_ATTR_T 36
> +# define __SIZEOF_PTHREAD_MUTEX_T 24
> +# define __SIZEOF_PTHREAD_MUTEXATTR_T 4
> +# define __SIZEOF_PTHREAD_COND_T 48
> +# define __SIZEOF_PTHREAD_CONDATTR_T 4
> +# define __SIZEOF_PTHREAD_RWLOCK_T 32
> +# define __SIZEOF_PTHREAD_RWLOCKATTR_T 8
> +# define __SIZEOF_PTHREAD_BARRIER_T 20
> +# define __SIZEOF_PTHREAD_BARRIERATTR_T 4
> +#endif
> +
> +#define __PTHREAD_COMPAT_PADDING_MID
> +#define __PTHREAD_COMPAT_PADDING_END
> +#define __PTHREAD_MUTEX_LOCK_ELISION	0
> +
> +#define __LOCK_ALIGNMENT
> +#define __ONCE_ALIGNMENT
> +
> +struct __pthread_rwlock_arch_t
> +{
> +# if __riscv_xlen == 64
> +    unsigned int __readers;
> +    unsigned int __writers;
> +    unsigned int __wrphase_futex;
> +    unsigned int __writers_futex;
> +    int __cur_writer;
> +    int __shared;
> +    int __pad3;
> +    int __pad4;
> +    unsigned long int __pad1;
> +    unsigned long int __pad2;
> +    /* FLAGS must stay at this position in the structure to maintain
> +       binary compatibility.  */

That's not the case, I assume, given that your port is new?

> +    unsigned int __flags;
> +# else
> +    unsigned int __readers;
> +    unsigned int __writers;
> +    unsigned int __wrphase_futex;
> +    unsigned int __writers_futex;
> +    int __cur_writer;
> +    int __pad3;
> +#if __BYTE_ORDER == __BIG_ENDIAN
> +    unsigned char __pad1;
> +    unsigned char __pad2;
> +    unsigned char __shared;
> +    /* FLAGS must stay at this position in the structure to maintain
> +       binary compatibility.  */
> +    unsigned char __flags;
> +#else
> +    /* FLAGS must stay at this position in the structure to maintain
> +       binary compatibility.  */
> +    unsigned char __flags;
> +    unsigned char __shared;
> +    unsigned char __pad1;
> +    unsigned char __pad2;
> +#endif
> +    int __writer;
> +# endif
> +};
> +
> +#define __PTHREAD_RWLOCK_ELISION_EXTRA 0
> +
> +#endif	/* bits/pthreadtypes.h */

> diff --git a/sysdeps/riscv/nptl/pthread_spin_lock.c b/sysdeps/riscv/nptl/pthread_spin_lock.c
> new file mode 100644
> index 0000000000..dd5ffae97f
> --- /dev/null
> +++ b/sysdeps/riscv/nptl/pthread_spin_lock.c
> @@ -0,0 +1,43 @@
> +/* Copyright (C) 2005 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, write to the Free
> +   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> +   02111-1307 USA.  */
> +
> +#ifdef __riscv_atomic
> +
> +#include <errno.h>
> +
> +int pthread_spin_lock(pthread_spinlock_t* lock)
> +{
> +  int tmp1, tmp2;
> +
> +  asm volatile ("\n\
> +  1:lw           %0, 0(%2)\n\
> +    li           %1, %3\n\
> +    bnez         %0, 1b\n\
> +    amoswap.w.aq %0, %1, 0(%2)\n\
> +    bnez         %0, 1b"
> +    : "=&r"(tmp1), "=&r"(tmp2) : "r"(lock), "i"(EBUSY)
> +  );

Why do you need a custom spinlock?  Why is the generic version not good
enough for you?
We try to have as little custom synchronization code as possible.

> +  return tmp1;
> +}
> +
> +#else  /* __riscv_atomic */
> +
> +#include <sysdeps/../nptl/pthread_spin_lock.c>
> +
> +#endif  /* !__riscv_atomic */
> diff --git a/sysdeps/riscv/nptl/pthread_spin_trylock.c b/sysdeps/riscv/nptl/pthread_spin_trylock.c
> new file mode 100644
> index 0000000000..9bd5d80fd6
> --- /dev/null
> +++ b/sysdeps/riscv/nptl/pthread_spin_trylock.c
> @@ -0,0 +1,43 @@
> +/* Copyright (C) 2005 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, write to the Free
> +   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> +   02111-1307 USA.  */
> +
> +#ifdef __riscv_atomic
> +
> +#include <errno.h>
> +
> +int pthread_spin_trylock(pthread_spinlock_t* lock)
> +{
> +  int tmp1, tmp2;
> +
> +  asm volatile ("\n\
> +    lw           %0, 0(%2)\n\
> +    li           %1, %3\n\
> +    bnez         %0, 1f\n\
> +    amoswap.w.aq %0, %1, 0(%2)\n\
> +  1:"
> +    : "=&r"(tmp1), "=&r"(tmp2) : "r"(lock), "i"(EBUSY)
> +  );

Likewise.

> +
> +  return tmp1;
> +}
> +
> +#else  /* __riscv_atomic */
> +
> +#include <sysdeps/../nptl/pthread_spin_trylock.c>
> +
> +#endif  /* !__riscv_atomic */
> diff --git a/sysdeps/unix/sysv/linux/riscv/atomic-machine.h b/sysdeps/unix/sysv/linux/riscv/atomic-machine.h
> new file mode 100644
> index 0000000000..5ce6f8da40
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/riscv/atomic-machine.h
> @@ -0,0 +1,53 @@
> +/* Low-level functions for atomic operations. RISC-V version.
> +   Copyright (C) 2014-2017 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, write to the Free
> +   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
> +   02111-1307 USA.  */
> +
> +#ifndef _LINUX_RISCV_BITS_ATOMIC_H
> +#define _LINUX_RISCV_BITS_ATOMIC_H 1
> +
> +#include_next <atomic-machine.h>

See above.

> +
> +#ifndef __riscv_atomic
> +
> +#include <sys/syscall.h>
> +#include <sysdep.h>
> +
> +#define __arch_compare_and_exchange_val_8_acq(mem, newval, oldval) \
> +  (abort (), (__typeof (*mem)) 0)
> +
> +#define __arch_compare_and_exchange_val_16_acq(mem, newval, oldval) \
> +  (abort (), (__typeof (*mem)) 0)
> +
> +/* The only basic operation needed is compare and exchange.  */
> +#define __arch_compare_and_exchange_val_32_acq(mem, newval, oldval) \
> +  ({									      \
> +    INTERNAL_SYSCALL_DECL (__err);					      \
> +    (__typeof (*mem)) INTERNAL_SYSCALL (sysriscv, __err, 4,		      \
> +		      RISCV_ATOMIC_CMPXCHG, mem, oldval, newval);	      \
> +  })
> +
> +#define __arch_compare_and_exchange_val_64_acq(mem, newval, oldval) \
> +  ({									      \
> +    INTERNAL_SYSCALL_DECL (__err);					      \
> +    (__typeof (*mem)) INTERNAL_SYSCALL (sysriscv, __err, 4,		      \
> +		      RISCV_ATOMIC_CMPXCHG64, mem, oldval, newval);	      \
> +  })
> +

You should add a comment explaining what your syscall does exactly, in
particular why you don't need to use it for atomic stores.