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

[Darius Rad <darius at bluespec dot com>]

On 06/23/2017 05:41 AM, Torvald Riegel wrote:
> 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?
> 

Yes, thank you.  We'll ensure that build-many-glibcs.py is consistent
with the features in the patch.

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

Understood.  We'll use a single file, and locate it in the Linux area
only if the atomic syscall is used.

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

Good to know.  We should be safe because the CAS syscall is atomic with
respect to loads and stores, due to the first reason you suggest (single
processor, uninterruptable).