On Mon, 19 Jan 2026 11:18:22 GMT, Anton Artemov <[email protected]> wrote:
>> Hi, please consider the following changes: >> >> If suspension is allowed when a thread is re-entering an object monitor >> (OM), then a following liveness issues can happen in the >> `ObjectMonitor::wait()` method. >> >> The waiting thread is made to be a successor and is unparked. Upon a >> suspension request, the thread will suspend itself whilst clearing the >> successor. The OM will be left unlocked (not grabbed by any thread), while >> the other threads are parked until a thread grabs the OM and the exits it. >> The suspended thread is on the entry-list and can be selected as a successor >> again. None of other threads can be woken up to grab the OM until the >> suspended thread has been resumed and successfully releases the OM. >> >> This can happen in three places where the successor could be suspended: >> >> 1: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1897 >> >> 2: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1149 >> >> 3: >> https://github.com/openjdk/jdk/blob/6322aaba63b235cb6c73d23a932210af318404ec/src/hotspot/share/runtime/objectMonitor.cpp#L1951 >> >> The issues are addressed by not allowing suspension in case 1, and by >> handling the suspension request at a later stage, after the thread has >> grabbed the OM in `reenter_internal()` in case 2. In case of a suspension >> request, the thread exits the OM and enters it again once resumed. >> >> Case 3 is handled by not transferring a thread to the `entry_list` in >> `notify_internal()` in case the corresponding JVMTI event is allowed. >> Instead, a tread is unparked and let run. Since it is not on the >> `entry_list`, it will not be chosen as a successor and it is no harm to >> suspend it if needed when posting the event. >> >> Possible issue of posting a `waited` event while still be suspended is >> addressed by adding a suspension check just before the posting of event. >> >> Tests are added. >> >> Tested in tiers 1 - 7. > > Anton Artemov has updated the pull request incrementally with one additional > commit since the last revision: > > 8366659: Addressed reviewers' comments, added comments, renamed tests. Thanks for the updates. A few follow ups and queries. src/hotspot/share/runtime/objectMonitor.cpp line 2055: > 2053: // This is is now conditional as if the monitor_waited even > 2054: // is allowed, then a thread, even virtual, should not be moved > to > 2055: // the entry_list, but rather unparked and let run. See the > comment below. Suggestion: // If we will add the vthread to the entry list below then we need to // increment the counter *before* doing so. // Adding to _entry_list uses Atomic::cmpxchg() which already provides // a fence that prevents reordering of the stores. if (!JvmtiExport::should_post_monitor_waited()) { src/hotspot/share/runtime/objectMonitor.cpp line 2066: > 2064: // If the monitor_waited JVMTI event is not allowed, a thread is > 2065: // transferred to the entry_list, and it will eventually be > unparked > 2066: // only when it is chosen to become a successor. Suggestion: // only when it is chosen to become the successor. src/hotspot/share/runtime/objectMonitor.cpp line 2077: > 2075: // avoid a problem of having a suspension point when posting > 2076: // the monitor_waited JVMTI event, as suspending such a thread > 2077: // is no harm. Suggestion: // However, if the monitor_waited event is allowed, then // the thread is set to state TS_RUN and unparked. The thread // will then contend directly to reacquire the monitor and // avoids being flagged as the successor. This avoids the problem // of having a thread suspend whilst it is the successor. src/hotspot/share/runtime/objectMonitor.cpp line 2103: > 2101: evt->unpark(); > 2102: } > 2103: else if (java_lang_VirtualThread::set_onWaitingList(vthread, > vthread_list_head())) { Suggestion: } else if (java_lang_VirtualThread::set_onWaitingList(vthread, vthread_list_head())) { src/hotspot/share/runtime/objectMonitor.cpp line 2296: > 2294: // unparked directly in notify_internal(). Its state is then TS_RUN. > 2295: if (state == ObjectWaiter::TS_RUN) { > 2296: bool acquired = vthread_monitor_enter(current, node); If we get here due to the direct unpark is it possible for `acquired` to be false? If so then I think the else clause starting at line 2312 below will be incorrect - it expects the thread to be on the entry list which it won't be. ------------- PR Review: https://git.openjdk.org/jdk/pull/27040#pullrequestreview-3679921028 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706376692 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706377771 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706384082 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706385541 PR Review Comment: https://git.openjdk.org/jdk/pull/27040#discussion_r2706403647
