Re: [RFC][PATCH 0/2] Make sparcv8 work again on cas enabled hardware

[Torvald Riegel <triegel@redhat.com>]

On Tue, 2016-11-01 at 16:07 +0100, Andreas Larsson wrote:
> This patch series:
> 
> 1) Fixes a sparcv8 bug introduced since the #error was added to
> sysdeps/sparc/sparc32/pthread_barrier_wait.c in 2.23. This fix stops
> incorrect usage of sendmsg and recvmsg Linux system calls for sparcv8.

I don't know whether the changes this patch applies make sense, but
otherwise the patch looks okay to me.  This could also be a separate
patch I think.

Do you have a copyright assignment in place for glibc?

> 2) Makes use of the CASA compare and swap instruction for atomic_*
> functions sparcv8, that is available for most LEON3 and LEON4 designs
> and implied by -mcpu=leon3, but not part of the sparcv8 standard. To
> allow for easy kernel emulation on systems that lack the instruction,
> the CASA instruction is used for all writing atomic_* functions. This
> approach is discussed in thread [1].

Before I can review the patch in detail, I think there are a few
high-level things that need to be taken care of.

We need to document what sparc32 systems we actually support.  Your
patch looks like for now, CAS is required.  If this is true, this should
be documented (e.g., I guess this would need a NEWS item too).

Is there a way to test this new requirement at build time, or is this
just a runtime requirement?  If this is a build-time requirement, is the
assembler actually already aware that it can use a CAS (which would
remove the need to hand-code the instruction).

If we require CAS, the 24b exchanges should just be removed altogether;
it seems the only remaining user is the low-level lock.

I don't think you need to make all modifying atomic accesses use a CAS
underneath, at least if we require CAS.  If we will also allow for
kernel emulation in the future, it would also be possible to check
whether emulation is required and only then route all modifying accesses
through the kernel.

In the future, we will most likely require 8b atomic loads and stores
(perhaps we can do without an 8b CAS, though doing that with a 32b cas
is possible).

I suggest to also look at whether you can use the new __atomic builtins
(ie, as noted by the USE_ATOMIC_COMPILER_BUILTINS define).  On the sparc
systems that we want to support, will GCC do the right thing for CAS
etc. (eg, if the requirement is to build with -mcpu=leon3)?  In
particular, will it either always emit a CAS instruction or will
libatomic use a CAS instruction?
If so, you could also just rely on the new atomic builtins and define
the legacy atomic operations (ie, those not in C11 style) on top of
those. 

> Any comments are most welcome. The spin lock based sparcv8 semaphore
> implementation is currently unchanged by this patchset, but I would say
> that that should go as well.

Agreed.