Re: [PATCH] AArch64: Fix __sync_val_compare_and_swap [PR111404]

[Ramana Radhakrishnan <ramana.gcc@googlemail.com>]

Hi Wilco,

Thanks for your email.

On Tue, Sep 26, 2023 at 12:07 AM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> Hi Ramana,
>
> >> __sync_val_compare_and_swap may be used on 128-bit types and either calls the
> >> outline atomic code or uses an inline loop.  On AArch64 LDXP is only atomic if
> >> the value is stored successfully using STXP, but the current implementations
> >> do not perform the store if the comparison fails.  In this case the value returned
> >> is not read atomically.
> >
> > IIRC, the previous discussions in this space revolved around the
> > difficulty with the store writing to readonly memory which is why I
> > think we went with LDXP in this form.
>
> That's not related to this patch - this fixes a serious atomicity bug that may
> affect the Linux kernel since it uses the older sync primitives. Given that LDXP
> is not atomic on its own, you have to execute the STXP even in the failure case.
> Note that you can't rely on compare&swap not to write memory: load-exclusive
> loops may either always write or avoid writes in the failure case if the load is
> atomic. CAS instructions always write.
>

I am aware of the capabilities of the architecture.

> > Has something changed from then ?
>
> Yes, we now know that using locking atomics was a bad decision. Developers
> actually require efficient and lock-free atomics. Since we didn't support them,
> many applications were forced to add their own atomic implementations using
> hacky inline assembler. It also resulted in a nasty ABI incompatibility between
> GCC and LLVM. Yes - atomics are part of the ABI!

I agree that atomics are part of the ABI.

>
> All that is much worse than worrying about a theoretical corner case that
> can't happen in real applications - atomics only work on writeable memory
> since their purpose is to synchronize reads with writes.


I remember this to be the previous discussions and common understanding.

https://gcc.gnu.org/legacy-ml/gcc/2016-06/msg00017.html

and here

https://gcc.gnu.org/legacy-ml/gcc-patches/2017-02/msg00168.html

Can you point any discussion recently that shows this has changed and
point me at that discussion if any anywhere ? I can't find it in my
searches . Perhaps you've had the discussion some place to show it has
changed.


regards
Ramana



>
> Cheers,
> Wilco