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 Fri, 23 Jun 2017 02:41:29 PDT (-0700), triegel@redhat.com 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?

Sorry, I guess I was a bit confused when answering the original question: we
only expect users to build standard Linux systems with the following
configurations

 * -march=rv32imac -mabi=ilp32
 * -march=rv32imafdc -mabi=ilp32d
 * -march=rv64imac -mabi=lp64
 * -march=rv64imafdc -mabi=lp64d

but we want to support the full RISC-V ISA, as we've gotten all the other
targets working and want to ensure they don't break.  Thus we'll add many
configurations to build-many-glibcs.py, including at least

 * -march=rv32i -mabi=ilp32
 * -march=rv32imafdc -mabi=ilp32
 * -march=rv32imafdc -mabi=ilp32f
 * -march=rv64i -mabi=lp64
 * -march=rv64imafdc -mabi=lp64
 * -march=rv64imafdc -mabi=lp64f

which should serve to test all the code: the rv{32,64}i targets test just the
base ISA (which will trigger the atomic and soft-float code), and the
floating-point ISA with soft-float ABI tests that difference.

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

There's two cases this syscall is used:

 * The kernel is built without A support.  The kernel requires A support in
   order to enable SMP, so this means it's a uniprocessor system.  In that case
   we just call preempt_{disable,enable} to get the atomicity, which isn't a
   big deal because we know we're on a uniprocessor.

 * The kernel is built with A support, but userspace is built without A
   support.  In this case we might be on an SMP system, but can just perform
   the atomic operation in the kernel directly.


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