This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]