RFR: 8315856: RISC-V: Use Zacas extension for cmpxchg [v4]

[Andrew Haley]

On Tue, 12 Dec 2023 08:41:21 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> I have seen code, do not find it anymore in code base, which use cmpxchg for Atomic::release_store_fence().
>> (which is "new");
>> 
>> Atomic::store(_x, true);
>> Atomic::store_release_fence(_one_way_barrier, true);
>> bool z = Atomic::load(_z);
>> 
>> But it is written as:
>> 
>> Atomic::store(_x, true);
>> Atomic::cmpxchg(_one_way_barrier, false, true);
>> bool z = Atomic::load(_z);
>> 
>> Where load Z happens after store X. But without release the store may happens after.
>> 
>> As we say:
>> "All of the atomic operations that imply a read-modify-write action guarantee a two-way memory barrier across that operation."
>> 
>> I have look over the cmpxchg uses and not releasing 'seems' okay at first glance.
>> But I think before this line changes we should release on failed CAS.
>
> Note I mention the hotspot runtime atomic here because cmpxchgptr is used to manipulate the markword which follows hotspot atomics, not Java volatiles.

> I have seen code, do not find it anymore in code base, which use cmpxchg for Atomic::release_store_fence(). (which is "new");
> 
> ```
> Atomic::store(_x, true);
> Atomic::store_release_fence(_one_way_barrier, true);
> bool z = Atomic::load(_z);
> ```
> 
> But it is written as:
> 
> ```
> Atomic::store(_x, true);
> Atomic::cmpxchg(_one_way_barrier, false, true);
> bool z = Atomic::load(_z);
> ```
> 
> Where load Z happens after store X. But without release the store may happens after.

Yuck. I hope that is never going to return.

I think all such usages of cmpxchg were replaced by an atomic add of zero into the stack, which works on x86. 

> As we say: "All of the atomic operations that imply a read-modify-write action guarantee a two-way memory barrier across that operation."

Well, that's for the HotSpot internal atomics, rather than for Java code. And given that this particular version of `cmpxchg` is only used in a few places it suffices to see what they need. 

> I have look over the cmpxchg uses and not releasing 'seems' okay at first glance. But I think before this line changes we should release on failed CAS.

I have never agreed with that thinking. We should find the callers of CAS that depend on such a side effect and fix them. The problem is that no-one wants to do it. There may no longer be any such cases.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/16910#discussion_r1424352158