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