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

[Torvald Riegel <triegel at redhat dot com>]

On Thu, 2017-06-22 at 19:42 -0400, Darius Rad wrote:
> 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.

Thanks for providing some background.  However, elsewhere in the thread
for the whole patch set, you seemed to say that you'll only officially
support target configurations that do have atomics (IIRC, that was a
discussion with Joseph about the target triples).  These officially
supported ones are the configurations we'll test with
build-many-glibcs.py.  I think that it would be better to only have code
in master that will be compiled by some build through
build-many-glibcs.py (ie, that there's no dead code from that build
coverage perspective).

Does this clarify my question / concern?

> >> +# 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?

You're right, you do make amoswap available.  The compiler builtins
should use it to.

> > 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.

The latter sounds better.  But if it turns out that you officially
support only target configurations with native atomics, then I'd suggest
to just include that, and then you won't need the syscall fallback.

> >> +#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."

That is true under the assumption that the CAS really is atomic wrt to
atomic stores and loads (maybe we should clarify that...).  If the
syscall cannot be interrupted by stores, then you're good (eg, because
there's just a single HW thread and your syscall cannot be interrupted,
or because you stop all other threads in the syscall).