On Thu, 10 Apr 2025 06:41:42 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> This fixes the issue with lack of synchronization between JVMTI thread >> suspend and resume functions in a self-suspend case. More detailed fix >> description is in the first PR comment. >> >> Testing: Ran mach5 tiers 1-6. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: added general comment about sync between suspend_thread and > resume_thread Hi Serguei, The fix looks correct but I think we could probably do something that avoids this overhead and also simplifies the code. Looking at each suspend case we have, in terms of lack of sync between self-suspend and resume: - unmounted vthread: should not be an issue, unmounted implies it’s not self suspend. - platform thread (not carrying vthread): Currently we have an issue because we are setting `_carrier_thread_suspended` always, not only for the carrying vthread case. We then assume that `_carrier_thread_suspended`=true implies suspension, but that will only be the case for the carrying vthread case which checks for this and suspends in `finish_VTMS_transition`. A normal platform thread needs to still call handshake code to suspend. This is the issue with this test because a resumer sees the target is already suspended and calls resume before the target actually calls handshake suspend. We should just set `_carrier_thread_suspended` on the carrying vthread case. - platform thread carrying vthread: we can keep using `_carrier_thread_suspended` but should use atomic cmpxchg when updating. - mounted vthread: Currently we have an issue because we are doing two things that are not atomic, adding/removing to the list and calling handshake code to suspend/resume. We could move the update of the list inside the handshake instead. I’ve been experimenting with the above changes here: https://github.com/pchilano/jdk/commit/7cce48ef14bdb977b9c65a33370144f3d61fc974 Notice that the `suspend_thread/resume_thread` methods are simpler to read because the cases are clearly separated. This would also fix the lack of sync between self-suspend and suspend, which we still have if we just change resume to be a handshake. What do you think? The `JvmtiVTMSTransitionDisabler` changes could be done in a separate fix. ------------- PR Comment: https://git.openjdk.org/jdk/pull/24269#issuecomment-2840140925
