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

Robbin Ehn robbin.ehn at oracle.com
Wed Oct 31 07:03:30 UTC 2018


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
>>>>>>
> 


More information about the serviceability-dev mailing list