This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 09/12] RISC-V: Atomic and Locking Routines
On 06/20/2017 10:20 AM, Torvald Riegel wrote:
> 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?
>
We do support variants without atomic instructions, although that is not
the highest priority now. The current implementation uses a Linux
syscall to emulate atomic operations (like Xtensa, ColdFire).
Ultimately, we plan to replace this with an operation in the VDSO (like
ARM) to improve performance.
>> 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?
>
Indeed, you are correct. The helper is not necessary if hardware
supports atomics.
>> +# 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?
>
Is that not atomic_exchange_acq and atomic_exchange_rel, or do I
misunderstand you?
>> +#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?
>
I'm not aware of a need to support glibc on anything but Linux, but I
don't think we want to preclude it unnecessarily, either. Though we can
use a Linux-specific file until there is a need otherwise.
>> +
>> +#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?
>
Correct, that should not be necessary.
>> + 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.
>
The generic spinlock should be fine.
>> + 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.
>
Understood about explaining the syscall.
Regarding atomic stores, this comment in
sysdeps/generic/atomic-machine.h suggests they are not necessary: "The
only basic operation needed is compare and exchange."
Separately, RISC-V guarantees naturally aligned stores are atomic in all
cases (even without atomic instructions), although that may not be
sufficient here.