Bug ID: JDK-8267585 JavaThread::set_thread_state generally needs release semantics

Versions (Unresolved/Resolved/Fixed)

The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.

JDK 18
18Resolved

As discussed in JDK-8265753 we have code in the thread state transition logic that does:

thread->frame_anchor()->make_walkable(thread);
thread->set_thread_state(_thread_blocked);

this logically requires a storestore barrier in between them, or more generally for the set_thread_state to have release semantics.

Now set_thread_state actually does have release semantics on PPC and Aarch64, but that is not how we normally handle memory barriers in shared code. Shared code should be written to express all needed barriers assuming the loosest memory model. It is then up to the implementation code for the barriers to reduce it to nothing on platforms that don't need it.

There is some history as to how the current situation came about when the PPC and ARM ports were merged into mainline, but I think it would be much clearer if the barriers were more directly evident in the shared code.

There is no interest in this change nor agreement around it, so leaving things as-is and closing this RFE.

30-07-2021

The PPC port originally added release/acquire semantics to set_thread_state()/thread_state() under JDK-8029396. But this was then conditionalized to only affect PPC64 using JDK-8032634, due to performance concerns on other platforms. Later AARCH64 was added and it followed what PPC64 did. I cannot find any discussion relating to the need for acquire/release semantics, but other than the make_walkable() case I can't see where release semantics would be needed.

29-07-2021

There are actually only three cases where we seem to need release semantics in relation to make_walkable: diff --git a/src/hotspot/share/runtime/interfaceSupport.inline.hpp b/src/hotspot/share/runtime/interfaceSupport.inline.hpp index 3a4ffb80168..8fa93ce57cc 100644 --- a/src/hotspot/share/runtime/interfaceSupport.inline.hpp +++ b/src/hotspot/share/runtime/interfaceSupport.inline.hpp @@ -210,8 +210,7 @@ class ThreadInVMfromNative : public ThreadStateTransition { _thread->check_possible_safepoint(); // Once we are in native vm expects stack to be walkable _thread->frame_anchor()->make_walkable(_thread); - OrderAccess::storestore(); // Keep thread_state change and make_walkable() separate. - _thread->set_thread_state(_thread_in_native); + _thread->release_set_thread_state(_thread_in_native); } }; @@ -249,8 +248,7 @@ class ThreadBlockInVMPreprocess : public ThreadStateTransition { thread->check_possible_safepoint(); // Once we are blocked vm expects stack to be walkable thread->frame_anchor()->make_walkable(thread); - OrderAccess::storestore(); // Keep thread_state change and make_walkable() separate. - thread->set_thread_state(_thread_blocked); + thread->release_set_thread_state(_thread_blocked); } ~ThreadBlockInVMPreprocess() { assert(_thread->thread_state() == _thread_blocked, "coming from wrong thread state"); diff --git a/src/hotspot/share/runtime/java.cpp b/src/hotspot/share/runtime/java.cpp index f1c591659a5..8682ecabec2 100644 --- a/src/hotspot/share/runtime/java.cpp +++ b/src/hotspot/share/runtime/java.cpp @@ -605,7 +605,7 @@ void vm_perform_shutdown_actions() { // Must always be walkable or have no last_Java_frame when in // thread_in_native jt->frame_anchor()->make_walkable(jt); - jt->set_thread_state(_thread_in_native); + jt->release_set_thread_state(_thread_in_native); } } notify_vm_shutdown();

19-07-2021