On Thu, 15 Apr 2021 07:14:09 GMT, Robbin Ehn <r...@openjdk.org> wrote:
>> A suspend request is done by handshaking thread target thread(s). When
>> executing the handshake operation we know the target mutator thread is in a
>> dormant state (as in safepoint safe state). We have a guarantee that it will
>> check it's poll before leaving the dormant state. To stop the thread from
>> leaving the the dormant state we install a second asynchronous handshake to
>> be executed by the targeted thread. The asynchronous handshake will wait on
>> a monitor while the thread is suspended. The target thread cannot not leave
>> the dormant state without a resume request.
>>
>> Per thread suspend requests are naturally serialized by the per thread
>> HandshakeState lock (we can only execute one handshake at a time per thread).
>> Instead of having a separate lock we use this to our advantage and use
>> HandshakeState lock for serializing access to the suspend flag and for
>> wait/notify.
>>
>> Suspend:
>> Requesting thread -> synchronous handshake -> target thread
>> Inside synchronus handshake (HandshakeState lock is locked while
>> executing any handshake):
>> - Set suspended flag
>> - Install asynchronous handshake
>>
>> Target thread -> tries to leave dormant state -> Executes handshakes
>> Target only executes asynchronous handshake:
>> - While suspended
>> - Go to blocked
>> - Wait on HandshakeState lock
>>
>> Resume:
>> Resuming thread:
>> - Lock HandshakeState lock
>> - Clear suspended flag
>> - Notify HandshakeState lock
>> - Unlock HandshakeState lock
>>
>> The "suspend requested" flag is an optimization, without it a dormant thread
>> could be suspended and resumed many times and each would add a new
>> asynchronous handshake. Suspend requested flag means there is already an
>> asynchronous suspend handshake in queue which can be re-used, only the
>> suspend flag needs to be set.
>>
>> ----
>> Some code can be simplified or done in a smarter way but I refrained from
>> doing such changes instead tried to keep existing code as is as far as
>> possible. This concerns especially raw monitors.
>>
>> ----
>> Regarding the changed test, the documentation says:
>> "If the calling thread is specified in the request_list array, this function
>> will not return until some other thread resumes it."
>>
>> But the code:
>> LOG("suspendTestedThreads: before JVMTI SuspendThreadList");
>> err = jvmti->SuspendThreadList(threads_count, threads, results);
>> ...
>> // Allow the Main thread to inspect the result of tested threads
>> suspension
>> agent_unlock(jni);
>>
>> The thread will never return from SuspendThreadList until resumed, so it
>> cannot unlock with agent_unlock().
>> Thus main thread is stuck forever on:
>> // Block until the suspender thread competes the tested threads suspension
>> agent_lock(jni);
>>
>> And never checks and resumes the threads. So I removed that lock instead
>> just sleep and check until all thread have the expected suspended state.
>>
>> ----
>>
>> This version already contains updates after pre-review comments from
>> @dcubed-ojdk, @pchilano, @coleenp.
>> (Pre-review comments here:
>> https://github.com/openjdk/jdk/pull/2625)
>>
>> ----
>> Testing t1-t8, nsk_jdi/nsk_jvmti/jdk_jdi/tck, KS24, RunThese and
>> combinations like running with -XX:ZCollectionInterval=0.01 -
>> XX:ZFragmentationLimit=0.
>> Running above some of above concurrently (load ~240), slow debug,
>> etc...
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 13 commits:
>
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 4
> - Fixed flag undef dep + spelling error
> - Obsolete unused flags
> - Review fixes 3
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes 2
> - White space fixes
> - Merge branch 'master' into SuspendInHandshake
> - Review fixes
> - ... and 3 more:
> https://git.openjdk.java.net/jdk/compare/b224b566...27bf041c
Only nits on the incremental.
Like Dan I need to re-examine the complete set of changes to see if anything
else sticks out (other than the non-encapsulation of the state changes around
locking/unlocking when suspended).
Thanks,
David
src/hotspot/share/prims/jvmtiEnv.cpp line 952:
> 950: if (!JvmtiSuspendControl::suspend(java_thread)) {
> 951: // Either the thread is already suspended or
> 952: // it was in process of exiting.
Nit: the previous comment was perfectly correct. It is okay to replace the
second "the thread" with "it" but it should still read "in _the_ process of
exiting".
src/hotspot/share/prims/jvmtiEnv.cpp line 993:
> 991: if (!JvmtiSuspendControl::suspend(java_thread)) {
> 992: // Either the thread is already suspended or
> 993: // it was in process of exiting.
Ditto - "in the process ..."
src/hotspot/share/prims/jvmtiEnv.cpp line 1006:
> 1004: if (!JvmtiSuspendControl::suspend(current)) {
> 1005: // Either the thread is already suspended or
> 1006: // it was in process of exiting.
Ditto - "in the process ..."
-------------
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3191
