RFR: 8267585: JavaThread::set_thread_state generally needs release semantics

[Martin Doerr]

On Mon, 19 Jul 2021 11:57:43 GMT, David Holmes <dholmes at openjdk.org> wrote:

> Please review this small change to try and tidy up some of the memory ordering issues around the use of  `JavaThread::set_thread_state()` and `JavaThread::thread_state()`.
> 
> - Added: `release_set_thread_state()` and `thread_state_acquire()`
> - Replaced use of storestore barrier before `set_thread_state()` with `release_set_thread_state()`
> - Added missing release semantics in  ` vm_perform_shutdown_actions()`
> - Changed uses of `thread_state()` prior to checking `walkable()` to use acquire semantics, to match the release semantics used after `make_walkable()`
> - Use Atomic load/store instead of plain accesses on `_thread_state`
> - Removed unnecessary casts in the use of the Atomic and OrderAccess API's.
> 
> This makes some of the semantics clearer and removes redundant barriers on PPC64 and Aarch64 in some cases.
> 
> I question whether acquire/release semantics are needed by default on PPC64/Aarch64. I couldn't find any discussion related to that decision. But I can see that reasoning about when they are, and are not needed is quite difficult. So I didn't try to change that.
> 
> Testing:
>  - tiers 1-4 builds (as sanity check for cast removal etc)
>  - GHA (in progress)
> 
> Thanks,
> David

Hi David, the proposed change was not bad. It would just be even better if we could get rid of the nasty platform difference IMHO. The release/acquire semantics are not there "just in case". There were real problems on PPC64 and aarch64. I still think that what we currently have is worse:
If PPC64 or aarch64: use explicit release/acquire semantics for some not closer identified usages
Other platforms: rely on implicit release/acquire semantics which are ensured by CPU or ABI convention

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

PR: https://git.openjdk.java.net/jdk/pull/4825