Re: RFR: 8212933: Thread-SMR: requesting a VM operation whilst holding a ThreadsListHandle can cause deadlocks

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8212933: Thread-SMR: requesting a VM operation whilst holding a ThreadsListHandle can cause deadlocks

Daniel D. Daugherty
On 10/29/18 9:31 PM, David Holmes wrote:
> Thanks for the explanation Robbin.
>
> The inline patch also seems fine. I hope the other reviewers noticed it.

Yes, but I forgot to reply to it.

Thumbs up.

Dan


>
> David
>
> On 29/10/2018 7:05 PM, Robbin Ehn wrote:
>> Hi David,
>>
>> On 29/10/2018 07:20, David Holmes wrote:
>>> Hi Robbin,
>>>
>>> On 29/10/2018 6:08 AM, Robbin Ehn wrote:
>>>> Hi Dan,
>>>>
>>>> Thanks for looking at this, here is the update:
>>>> Inc: http://cr.openjdk.java.net/~rehn/8212933/v2/inc/webrev/
>>>> Full: http://cr.openjdk.java.net/~rehn/8212933/v2/webrev/
>>>
>>> I can't say I really understand the change in protocol here and why
>>> all the cancel operations are no longer needed. I see the handshake
>>> VM operations reusing the initial "threads list" but I'm unclear how
>>> they might be affected if additional threads are added to the system
>>> before the Threads_lock is acquired?
>>
>> The ThreadsList is a snapshot of all the JavaThreads at that time in
>> the VM.
>> Handshake all threads only handshake those JavaThreads. We do not
>> care about new
>> threads.
>>
>> The typical generic use-case is the similar to RCU. You first update
>> a global
>> state and emit the handshake when the handshake return no thread can
>> see the old
>> state.
>>
>> GlobalFuncPtr = some_new_func;
>> HandshakeAllThreads;
>> ------------------------------
>> No thread can be executing the old func.
>>
>> If the JavaThreads have a local copy of GlobalFuncPtr the handshake
>> operation would be to update the local copy to some_new_func.
>>
>> It works for both Java and for VM resources that respect safepoints.
>> For a pure VM resource it's much cheaper to use the GlobalCounter.
>>
>> The Threads_lock must only be held for S/R protocol.
>> With changes to the S/R protocol, such as using handshake instead, we
>> can remove
>> Threads_lock for handshakes completely. (with a other small fixes)
>>
>> The cancel is no longer needed since the terminated threads are
>> visible to the
>> VM thread when we keep the arming threadslist. We add terminated
>> threads as safe
>> for handshake. But if we handshake a terminated thread we do not
>> execute the
>> handshake operation, instead just clear the operation and increment the
>> completed counter. (the VM thread cancels the operation)
>>
>> I hope that helped?
>>
>>>
>>> A couple of specific comments:
>>>
>>> src/hotspot/share/runtime/handshake.hpp
>>>
>>> cancel_inner() is dead now.
>>>
>>> ---
>>>
>>> src/hotspot/share/runtime/handshake.cpp
>>>
>>> This was an odd looking for loop before your change and now looks
>>> even more strange:
>>>
>>>   for ( ; JavaThread *thr = jtiwh.next(); ) {
>>>
>>> can it not simply be a more normal looking:
>>>
>>>   for (JavaThread *thr = jtiwh.next(); thr != NULL; thr =
>>> jtiwh.next()) {
>>>
>>> ?
>>
>> Thanks, fixed with below patch.
>>
>> /Robbin
>>
>> diff -r 5f8b292c473f src/hotspot/share/runtime/handshake.cpp
>> --- a/src/hotspot/share/runtime/handshake.cpp    Sun Oct 28 20:57:24
>> 2018 +0100
>> +++ b/src/hotspot/share/runtime/handshake.cpp    Mon Oct 29 09:32:26
>> 2018 +0100
>> @@ -166,1 +166,1 @@
>> -    for ( ; JavaThread *thr = jtiwh.next(); ) {
>> +    for (JavaThread *thr = jtiwh.next(); thr != NULL; thr =
>> jtiwh.next()) {
>> @@ -198,1 +198,1 @@
>> -          for ( ; JavaThread *thr = jtiwh.next(); ) {
>> +          for (JavaThread *thr = jtiwh.next(); thr != NULL; thr =
>> jtiwh.next()) {
>> diff -r 5f8b292c473f src/hotspot/share/runtime/handshake.hpp
>> --- a/src/hotspot/share/runtime/handshake.hpp    Sun Oct 28 20:57:24
>> 2018 +0100
>> +++ b/src/hotspot/share/runtime/handshake.hpp    Mon Oct 29 09:32:26
>> 2018 +0100
>> @@ -63,1 +62,0 @@
>> -  void cancel_inner(JavaThread* thread);
>>
>>
>>>
>>> ---
>>>
>>> Thanks,
>>> David
>>>
>>>> /Robbin
>>>>
>>>> On 26/10/2018 17:38, Daniel D. Daugherty wrote:
>>>>> On 10/26/18 10:33 AM, Robbin Ehn wrote:
>>>>>> Hi, please review.
>>>>>>
>>>>>> When the VM thread executes a handshake it uses different
>>>>>> ThreadsLists during
>>>>>> the execution. A JavaThread that is armed for the handshake when
>>>>>> it is already
>>>>>> in the exit path in VM will cancel the handshake. Even if the VM
>>>>>> thread cannot
>>>>>> see this thread after the initial ThreadsList which where used
>>>>>> for arming, the
>>>>>> handshake can progress when the exiting thread cancels the
>>>>>> handshake.
>>>>>>
>>>>>> But if a third thread takes a ThreadsList where the exiting
>>>>>> JavaThread is present and tries to execute a VM operation, hence
>>>>>> waiting on VM thread to finish the handshake, the JavaThread in
>>>>>> the exit path can never reach the handshake cancellation point.
>>>>>> VM thread cannot finishes the handshake and the third thread is
>>>>>> stuck waiting on the VM thread.
>>>>>>
>>>>>> To allow holding a ThreadsList when executing a VM operation we
>>>>>> instead let the
>>>>>> VM thread use the same ThreadsList over the entire handshake
>>>>>> making all armed
>>>>>> threads visible to the VM thread at all time. And if VM thread
>>>>>> spots a terminated thread it will count that thread is already
>>>>>> done by only clearing
>>>>>> it's operation.
>>>>>>
>>>>>> Passes local stress testing, t1-5 and the deadlock is no longer
>>>>>> reproduce-able.
>>>>>> Added a jtreg handshake + thread suspend test as a reproducer.
>>>>>>
>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8212933
>>>>>> Code: http://cr.openjdk.java.net/~rehn/8212933/v1/webrev/
>>>>>
>>>>> src/hotspot/share/runtime/handshake.hpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/runtime/handshake.cpp
>>>>>      L358: void HandshakeState::process_by_vmthread(JavaThread*
>>>>> target) {
>>>>>      L359:   assert(Thread::current()->is_VM_thread(), "should
>>>>> call from vm thread");
>>>>>          Both calls to handshake_process_by_vmthread() which calls
>>>>> this
>>>>>          function are made with the Threads_lock held:
>>>>>
>>>>>          MutexLockerEx ml(Threads_lock,
>>>>> Mutex::_no_safepoint_check_flag);
>>>>>
>>>>>          Looks like the lock is grabbed because of
>>>>>          possibly_vmthread_can_process_handshake() which asserts:
>>>>>
>>>>>          L351:   // An externally suspended thread cannot be
>>>>> resumed while the
>>>>>          L352:   // Threads_lock is held so it is safe.
>>>>>          L353:   // Note that this method is allowed to produce
>>>>> false positives.
>>>>>          L354:   assert(Threads_lock->owned_by_self(), "Not
>>>>> holding Threads_lock.");
>>>>>          L355:   if (target->is_ext_suspended()) {
>>>>>          L356:     return true;
>>>>>          L357:   }
>>>>>
>>>>>          Also looks like vmthread_can_process_handshake() needs the
>>>>>          Threads_lock for the same externally suspended thread check.
>>>>>
>>>>>          So I was going to ask that you add:
>>>>>
>>>>>          assert(Threads_lock->owned_by_self(), "Not holding
>>>>> Threads_lock.");
>>>>>
>>>>>          after L359, but how about a comment instead:
>>>>>
>>>>>          // Threads_lock must be held here, but that is assert()ed in
>>>>>          // possibly_vmthread_can_process_handshake().
>>>>>
>>>>>
>>>>> src/hotspot/share/runtime/thread.hpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/runtime/thread.cpp
>>>>>      No comments.
>>>>>
>>>>> src/hotspot/share/runtime/threadSMR.cpp
>>>>>      No comments.
>>>>>
>>>>> test/hotspot/jtreg/runtime/handshake/HandshakeWalkSuspendExitTest.java
>>>>>
>>>>>      Very nice test! It specifically exercises ThreadLocalHandshakes
>>>>>      with JavaThread suspend/resume.
>>>>> runtime/Thread/SuspendAtExit.java
>>>>>      only ran into this bug by accident (JDK-8212152) so I like the
>>>>>      targeted test.
>>>>>
>>>>>      L49:         while(!exit_now) {
>>>>>          nit - please add a space before '('
>>>>>
>>>>>      L51:             for (int i = 0; i < _threads.length; i+=2) {
>>>>>      L58:             for (int i = 0; i < _threads.length; i+=2) {
>>>>>          nit - please added spaces around '+='
>>>>>
>>>>>          So why every other thread? A comment would be good...
>>>>>
>>>>>      L52:                 wb.handshakeWalkStack(null, true);
>>>>>          I'm guessing the 'null' parameter means current thread, but
>>>>>          that's a guess on my part. A comment would be good.
>>>>>
>>>>>      L82:         for (int i = 0; i < _threads.length; i++) {
>>>>>      L83:             _threads[i].join();
>>>>>      L84:         }
>>>>>          Thanks for cleaning up the test_threads. That will make
>>>>>          the JTREG thread sweeper happy. However, you don't save
>>>>>          the test_exit_thread references and you don't clean those
>>>>>          up either. Yes, I realize that they are supposed to exit,
>>>>>          but if something hangs up on exit, I'd rather have a join()
>>>>>          hang failure in this test's code than have the JTREG thread
>>>>>          sweeper catch it.
>>>>>
>>>>> Dan
>>>>>
>>>>>>
>>>>>> Thanks, Robbin
>>>>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8212933: Thread-SMR: requesting a VM operation whilst holding a ThreadsListHandle can cause deadlocks

Robbin Ehn
Thanks Dan, Robbin

On 10/30/18 2:18 PM, Daniel D. Daugherty wrote:

> On 10/29/18 9:31 PM, David Holmes wrote:
>> Thanks for the explanation Robbin.
>>
>> The inline patch also seems fine. I hope the other reviewers noticed it.
>
> Yes, but I forgot to reply to it.
>
> Thumbs up.
>
> Dan
>
>
>>
>> David
>>
>> On 29/10/2018 7:05 PM, Robbin Ehn wrote:
>>> Hi David,
>>>
>>> On 29/10/2018 07:20, David Holmes wrote:
>>>> Hi Robbin,
>>>>
>>>> On 29/10/2018 6:08 AM, Robbin Ehn wrote:
>>>>> Hi Dan,
>>>>>
>>>>> Thanks for looking at this, here is the update:
>>>>> Inc: http://cr.openjdk.java.net/~rehn/8212933/v2/inc/webrev/
>>>>> Full: http://cr.openjdk.java.net/~rehn/8212933/v2/webrev/
>>>>
>>>> I can't say I really understand the change in protocol here and why all the
>>>> cancel operations are no longer needed. I see the handshake VM operations
>>>> reusing the initial "threads list" but I'm unclear how they might be
>>>> affected if additional threads are added to the system before the
>>>> Threads_lock is acquired?
>>>
>>> The ThreadsList is a snapshot of all the JavaThreads at that time in the VM.
>>> Handshake all threads only handshake those JavaThreads. We do not care about new
>>> threads.
>>>
>>> The typical generic use-case is the similar to RCU. You first update a global
>>> state and emit the handshake when the handshake return no thread can see the old
>>> state.
>>>
>>> GlobalFuncPtr = some_new_func;
>>> HandshakeAllThreads;
>>> ------------------------------
>>> No thread can be executing the old func.
>>>
>>> If the JavaThreads have a local copy of GlobalFuncPtr the handshake operation
>>> would be to update the local copy to some_new_func.
>>>
>>> It works for both Java and for VM resources that respect safepoints.
>>> For a pure VM resource it's much cheaper to use the GlobalCounter.
>>>
>>> The Threads_lock must only be held for S/R protocol.
>>> With changes to the S/R protocol, such as using handshake instead, we can remove
>>> Threads_lock for handshakes completely. (with a other small fixes)
>>>
>>> The cancel is no longer needed since the terminated threads are visible to the
>>> VM thread when we keep the arming threadslist. We add terminated threads as safe
>>> for handshake. But if we handshake a terminated thread we do not execute the
>>> handshake operation, instead just clear the operation and increment the
>>> completed counter. (the VM thread cancels the operation)
>>>
>>> I hope that helped?
>>>
>>>>
>>>> A couple of specific comments:
>>>>
>>>> src/hotspot/share/runtime/handshake.hpp
>>>>
>>>> cancel_inner() is dead now.
>>>>
>>>> ---
>>>>
>>>> src/hotspot/share/runtime/handshake.cpp
>>>>
>>>> This was an odd looking for loop before your change and now looks even more
>>>> strange:
>>>>
>>>>   for ( ; JavaThread *thr = jtiwh.next(); ) {
>>>>
>>>> can it not simply be a more normal looking:
>>>>
>>>>   for (JavaThread *thr = jtiwh.next(); thr != NULL; thr = jtiwh.next()) {
>>>>
>>>> ?
>>>
>>> Thanks, fixed with below patch.
>>>
>>> /Robbin
>>>
>>> diff -r 5f8b292c473f src/hotspot/share/runtime/handshake.cpp
>>> --- a/src/hotspot/share/runtime/handshake.cpp    Sun Oct 28 20:57:24 2018 +0100
>>> +++ b/src/hotspot/share/runtime/handshake.cpp    Mon Oct 29 09:32:26 2018 +0100
>>> @@ -166,1 +166,1 @@
>>> -    for ( ; JavaThread *thr = jtiwh.next(); ) {
>>> +    for (JavaThread *thr = jtiwh.next(); thr != NULL; thr = jtiwh.next()) {
>>> @@ -198,1 +198,1 @@
>>> -          for ( ; JavaThread *thr = jtiwh.next(); ) {
>>> +          for (JavaThread *thr = jtiwh.next(); thr != NULL; thr =
>>> jtiwh.next()) {
>>> diff -r 5f8b292c473f src/hotspot/share/runtime/handshake.hpp
>>> --- a/src/hotspot/share/runtime/handshake.hpp    Sun Oct 28 20:57:24 2018 +0100
>>> +++ b/src/hotspot/share/runtime/handshake.hpp    Mon Oct 29 09:32:26 2018 +0100
>>> @@ -63,1 +62,0 @@
>>> -  void cancel_inner(JavaThread* thread);
>>>
>>>
>>>>
>>>> ---
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> /Robbin
>>>>>
>>>>> On 26/10/2018 17:38, Daniel D. Daugherty wrote:
>>>>>> On 10/26/18 10:33 AM, Robbin Ehn wrote:
>>>>>>> Hi, please review.
>>>>>>>
>>>>>>> When the VM thread executes a handshake it uses different ThreadsLists
>>>>>>> during
>>>>>>> the execution. A JavaThread that is armed for the handshake when it is
>>>>>>> already
>>>>>>> in the exit path in VM will cancel the handshake. Even if the VM thread
>>>>>>> cannot
>>>>>>> see this thread after the initial ThreadsList which where used for
>>>>>>> arming, the
>>>>>>> handshake can progress when the exiting thread cancels the handshake.
>>>>>>>
>>>>>>> But if a third thread takes a ThreadsList where the exiting JavaThread is
>>>>>>> present and tries to execute a VM operation, hence waiting on VM thread
>>>>>>> to finish the handshake, the JavaThread in the exit path can never reach
>>>>>>> the handshake cancellation point. VM thread cannot finishes the handshake
>>>>>>> and the third thread is stuck waiting on the VM thread.
>>>>>>>
>>>>>>> To allow holding a ThreadsList when executing a VM operation we instead
>>>>>>> let the
>>>>>>> VM thread use the same ThreadsList over the entire handshake making all
>>>>>>> armed
>>>>>>> threads visible to the VM thread at all time. And if VM thread spots a
>>>>>>> terminated thread it will count that thread is already done by only clearing
>>>>>>> it's operation.
>>>>>>>
>>>>>>> Passes local stress testing, t1-5 and the deadlock is no longer
>>>>>>> reproduce-able.
>>>>>>> Added a jtreg handshake + thread suspend test as a reproducer.
>>>>>>>
>>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8212933
>>>>>>> Code: http://cr.openjdk.java.net/~rehn/8212933/v1/webrev/
>>>>>>
>>>>>> src/hotspot/share/runtime/handshake.hpp
>>>>>>      No comments.
>>>>>>
>>>>>> src/hotspot/share/runtime/handshake.cpp
>>>>>>      L358: void HandshakeState::process_by_vmthread(JavaThread* target) {
>>>>>>      L359:   assert(Thread::current()->is_VM_thread(), "should call from
>>>>>> vm thread");
>>>>>>          Both calls to handshake_process_by_vmthread() which calls this
>>>>>>          function are made with the Threads_lock held:
>>>>>>
>>>>>>          MutexLockerEx ml(Threads_lock, Mutex::_no_safepoint_check_flag);
>>>>>>
>>>>>>          Looks like the lock is grabbed because of
>>>>>>          possibly_vmthread_can_process_handshake() which asserts:
>>>>>>
>>>>>>          L351:   // An externally suspended thread cannot be resumed while
>>>>>> the
>>>>>>          L352:   // Threads_lock is held so it is safe.
>>>>>>          L353:   // Note that this method is allowed to produce false
>>>>>> positives.
>>>>>>          L354:   assert(Threads_lock->owned_by_self(), "Not holding
>>>>>> Threads_lock.");
>>>>>>          L355:   if (target->is_ext_suspended()) {
>>>>>>          L356:     return true;
>>>>>>          L357:   }
>>>>>>
>>>>>>          Also looks like vmthread_can_process_handshake() needs the
>>>>>>          Threads_lock for the same externally suspended thread check.
>>>>>>
>>>>>>          So I was going to ask that you add:
>>>>>>
>>>>>>          assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
>>>>>>
>>>>>>          after L359, but how about a comment instead:
>>>>>>
>>>>>>          // Threads_lock must be held here, but that is assert()ed in
>>>>>>          // possibly_vmthread_can_process_handshake().
>>>>>>
>>>>>>
>>>>>> src/hotspot/share/runtime/thread.hpp
>>>>>>      No comments.
>>>>>>
>>>>>> src/hotspot/share/runtime/thread.cpp
>>>>>>      No comments.
>>>>>>
>>>>>> src/hotspot/share/runtime/threadSMR.cpp
>>>>>>      No comments.
>>>>>>
>>>>>> test/hotspot/jtreg/runtime/handshake/HandshakeWalkSuspendExitTest.java
>>>>>>      Very nice test! It specifically exercises ThreadLocalHandshakes
>>>>>>      with JavaThread suspend/resume. runtime/Thread/SuspendAtExit.java
>>>>>>      only ran into this bug by accident (JDK-8212152) so I like the
>>>>>>      targeted test.
>>>>>>
>>>>>>      L49:         while(!exit_now) {
>>>>>>          nit - please add a space before '('
>>>>>>
>>>>>>      L51:             for (int i = 0; i < _threads.length; i+=2) {
>>>>>>      L58:             for (int i = 0; i < _threads.length; i+=2) {
>>>>>>          nit - please added spaces around '+='
>>>>>>
>>>>>>          So why every other thread? A comment would be good...
>>>>>>
>>>>>>      L52:                 wb.handshakeWalkStack(null, true);
>>>>>>          I'm guessing the 'null' parameter means current thread, but
>>>>>>          that's a guess on my part. A comment would be good.
>>>>>>
>>>>>>      L82:         for (int i = 0; i < _threads.length; i++) {
>>>>>>      L83:             _threads[i].join();
>>>>>>      L84:         }
>>>>>>          Thanks for cleaning up the test_threads. That will make
>>>>>>          the JTREG thread sweeper happy. However, you don't save
>>>>>>          the test_exit_thread references and you don't clean those
>>>>>>          up either. Yes, I realize that they are supposed to exit,
>>>>>>          but if something hangs up on exit, I'd rather have a join()
>>>>>>          hang failure in this test's code than have the JTREG thread
>>>>>>          sweeper catch it.
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>>
>>>>>>> Thanks, Robbin
>>>>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8212933: Thread-SMR: requesting a VM operation whilst holding a ThreadsListHandle can cause deadlocks

Robbin Ehn
In reply to this post by Daniel D. Daugherty
Thanks Serguei, Robbin

On 10/29/18 6:35 PM, [hidden email] wrote:

> Hi Robbin,
>
> +1
>
> Thanks,
> Serguei
>
> On 10/29/18 06:52, Daniel D. Daugherty wrote:
>> On 10/28/18 4:08 PM, Robbin Ehn wrote:
>>> Hi Dan,
>>>
>>> Thanks for looking at this, here is the update:
>>> Inc: http://cr.openjdk.java.net/~rehn/8212933/v2/inc/webrev/
>>
>> src/hotspot/share/runtime/handshake.cpp
>>     No comments.
>>
>> test/hotspot/jtreg/runtime/handshake/HandshakeWalkSuspendExitTest.java
>>     No comments.
>>
>> Thumbs up!
>>
>> Thanks for making the updates.
>>
>> Dan
>>
>>
>>
>>> Full: http://cr.openjdk.java.net/~rehn/8212933/v2/webrev/
>>>
>>> /Robbin
>>>
>>> On 26/10/2018 17:38, Daniel D. Daugherty wrote:
>>>> On 10/26/18 10:33 AM, Robbin Ehn wrote:
>>>>> Hi, please review.
>>>>>
>>>>> When the VM thread executes a handshake it uses different ThreadsLists during
>>>>> the execution. A JavaThread that is armed for the handshake when it is already
>>>>> in the exit path in VM will cancel the handshake. Even if the VM thread cannot
>>>>> see this thread after the initial ThreadsList which where used for arming, the
>>>>> handshake can progress when the exiting thread cancels the handshake.
>>>>>
>>>>> But if a third thread takes a ThreadsList where the exiting JavaThread is
>>>>> present and tries to execute a VM operation, hence waiting on VM thread to
>>>>> finish the handshake, the JavaThread in the exit path can never reach the
>>>>> handshake cancellation point. VM thread cannot finishes the handshake and
>>>>> the third thread is stuck waiting on the VM thread.
>>>>>
>>>>> To allow holding a ThreadsList when executing a VM operation we instead let
>>>>> the
>>>>> VM thread use the same ThreadsList over the entire handshake making all armed
>>>>> threads visible to the VM thread at all time. And if VM thread spots a
>>>>> terminated thread it will count that thread is already done by only clearing
>>>>> it's operation.
>>>>>
>>>>> Passes local stress testing, t1-5 and the deadlock is no longer
>>>>> reproduce-able.
>>>>> Added a jtreg handshake + thread suspend test as a reproducer.
>>>>>
>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8212933
>>>>> Code: http://cr.openjdk.java.net/~rehn/8212933/v1/webrev/
>>>>
>>>> src/hotspot/share/runtime/handshake.hpp
>>>>      No comments.
>>>>
>>>> src/hotspot/share/runtime/handshake.cpp
>>>>      L358: void HandshakeState::process_by_vmthread(JavaThread* target) {
>>>>      L359:   assert(Thread::current()->is_VM_thread(), "should call from vm
>>>> thread");
>>>>          Both calls to handshake_process_by_vmthread() which calls this
>>>>          function are made with the Threads_lock held:
>>>>
>>>>          MutexLockerEx ml(Threads_lock, Mutex::_no_safepoint_check_flag);
>>>>
>>>>          Looks like the lock is grabbed because of
>>>>          possibly_vmthread_can_process_handshake() which asserts:
>>>>
>>>>          L351:   // An externally suspended thread cannot be resumed while the
>>>>          L352:   // Threads_lock is held so it is safe.
>>>>          L353:   // Note that this method is allowed to produce false
>>>> positives.
>>>>          L354:   assert(Threads_lock->owned_by_self(), "Not holding
>>>> Threads_lock.");
>>>>          L355:   if (target->is_ext_suspended()) {
>>>>          L356:     return true;
>>>>          L357:   }
>>>>
>>>>          Also looks like vmthread_can_process_handshake() needs the
>>>>          Threads_lock for the same externally suspended thread check.
>>>>
>>>>          So I was going to ask that you add:
>>>>
>>>>          assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
>>>>
>>>>          after L359, but how about a comment instead:
>>>>
>>>>          // Threads_lock must be held here, but that is assert()ed in
>>>>          // possibly_vmthread_can_process_handshake().
>>>>
>>>>
>>>> src/hotspot/share/runtime/thread.hpp
>>>>      No comments.
>>>>
>>>> src/hotspot/share/runtime/thread.cpp
>>>>      No comments.
>>>>
>>>> src/hotspot/share/runtime/threadSMR.cpp
>>>>      No comments.
>>>>
>>>> test/hotspot/jtreg/runtime/handshake/HandshakeWalkSuspendExitTest.java
>>>>      Very nice test! It specifically exercises ThreadLocalHandshakes
>>>>      with JavaThread suspend/resume. runtime/Thread/SuspendAtExit.java
>>>>      only ran into this bug by accident (JDK-8212152) so I like the
>>>>      targeted test.
>>>>
>>>>      L49:         while(!exit_now) {
>>>>          nit - please add a space before '('
>>>>
>>>>      L51:             for (int i = 0; i < _threads.length; i+=2) {
>>>>      L58:             for (int i = 0; i < _threads.length; i+=2) {
>>>>          nit - please added spaces around '+='
>>>>
>>>>          So why every other thread? A comment would be good...
>>>>
>>>>      L52:                 wb.handshakeWalkStack(null, true);
>>>>          I'm guessing the 'null' parameter means current thread, but
>>>>          that's a guess on my part. A comment would be good.
>>>>
>>>>      L82:         for (int i = 0; i < _threads.length; i++) {
>>>>      L83:             _threads[i].join();
>>>>      L84:         }
>>>>          Thanks for cleaning up the test_threads. That will make
>>>>          the JTREG thread sweeper happy. However, you don't save
>>>>          the test_exit_thread references and you don't clean those
>>>>          up either. Yes, I realize that they are supposed to exit,
>>>>          but if something hangs up on exit, I'd rather have a join()
>>>>          hang failure in this test's code than have the JTREG thread
>>>>          sweeper catch it.
>>>>
>>>> Dan
>>>>
>>>>>
>>>>> Thanks, Robbin
>>>>
>>
>