RFR(XS): 8207334: VM times out in VM_HandshakeAllThreads::doit() with RunThese30M

Robbin Ehn robbin.ehn at oracle.com
Fri Aug 17 10:22:58 UTC 2018


On 2018-08-16 16:29, Daniel D. Daugherty wrote:
> On 8/16/18 5:12 AM, Robbin Ehn wrote:
>> Same mail with corrected subject.
>>
>> On 08/16/2018 11:03 AM, Robbin Ehn wrote:
>>> Hi Dan, I took your suggestion on comment, thanks!
>>>
>>> Full patch looks like:
>>>
>>> diff -r 6bb7b8d0da76 src/hotspot/share/runtime/handshake.cpp
>>> --- a/src/hotspot/share/runtime/handshake.cpp    Thu Aug 16 09:46:09 2018 +0200
>>> +++ b/src/hotspot/share/runtime/handshake.cpp    Thu Aug 16 10:56:27 2018 +0200
>>> @@ -340,3 +340,9 @@
>>>   bool HandshakeState::vmthread_can_process_handshake(JavaThread* target) {
>>> -  return SafepointSynchronize::safepoint_safe(target, target->thread_state());
>>> +  // SafepointSynchronize::safepoint_safe() does not consider an externally
>>> +  // suspended thread to be safe. However, this function must be called with
>>> +  // the Threads_lock held so an externally suspended thread cannot be
>>> +  // resumed thus it is safe.
>>> +  assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
>>> +  return SafepointSynchronize::safepoint_safe(target, 
>>> target->thread_state()) ||
>>> +         target->is_ext_suspended();
>>>   }
>>
>> David, you also think this comment is better?
>>
>> /Robbin
> 
> Thumbs up from me.

Thanks Dan!

/Robbin

> 
> Dan
> 
> 
>>
>>>
>>> /Robbin
>>>
>>> On 08/15/2018 10:34 PM, Daniel D. Daugherty wrote:
>>>> On 8/15/18 4:21 PM, Robbin Ehn wrote:
>>>>> Hi Dan, thanks,
>>>>>
>>>>> On 2018-08-15 22:02, Daniel D. Daugherty wrote:
>>>>>> So if HandshakeState::vmthread_can_process_handshake() is only
>>>>>> called by the VMThread while at a safepoint, then the target
>>>>>> JavaThread cannot be externally resumed while that function is
>>>>>> running so this new check:
>>>>>
>>>>> No we are not at safepoint, this method is never called at safepoint.
>>>>
>>>> Okay I definitely misinterpreted what you were saying in this
>>>> email thread... let's try this again.
>>>>
>>>>
>>>>> All
>>>>> other threads should be considered running. Therefore I need to stop 
>>>>> resuming targeted suspended thread while VM thread is executing the 
>>>>> handshake on behalf of the suspended thread.
>>>>>
>>>>> Does my comment/assert make more sense in the light of that?
>>>>
>>>> Hmmm.... Here's your comment:
>>>>
>>>> +  // We must block the S/R protocol to resume a thread while the vm thread
>>>> +  // is executing the operation, by holding the Threads_lock.
>>>> +  // This is problematic, since we now can't remove the Threads_lock in 
>>>> handshakes.
>>>>
>>>> Mostly, but I still suggest a rewrite:
>>>>
>>>>    // SafepointSynchronize::safepoint_safe() does not consider an externally
>>>>    // suspended thread to be safe. However, this function must be called with
>>>>    // the Threads_lock held so an externally suspended thread cannot be
>>>>    // resumed thus it is safe.
>>>>
>>>> Of course, this comment assumes that the Threads_lock is held in the caller
>>>> over the appropriate interval of code...
>>>>
>>>> Dan
>>>>
>>>>
>>>>
>>>>>
>>>>> /Robbin
>>>>>
>>>>>>
>>>>>>    +  return SafepointSynchronize::safepoint_safe(target, 
>>>>>> target->thread_state()) ||
>>>>>>    +         target->is_ext_suspended();
>>>>>>
>>>>>> is okay because that externally suspended thread is also safepoint safe.
>>>>>>
>>>>>> This assert:
>>>>>>
>>>>>> +  assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
>>>>>>
>>>>>> doesn't quite match your intent. If 
>>>>>> HandshakeState::vmthread_can_process_handshake()
>>>>>> should only be called by the VMThread at a safepoint, then a different
>>>>>> check would be better:
>>>>>>
>>>>>>    assert(SafepointSynchronize::is_at_safepoint() && 
>>>>>> Thread::current()->is_VM_thread(),
>>>>>>           "must be VMThread at a safepoint");
>>>>>>
>>>>>> Perhaps this for your comment:
>>>>>>
>>>>>>    // SafepointSynchronize::safepoint_safe() does not consider an externally
>>>>>>    // suspended thread to be safe. However, if we are at a safepoint, then
>>>>>>    // the externally suspended thread cannot be resumed so it is safe.
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Thanks, Robbin
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>
>>>>>
>>>>
> 


More information about the hotspot-runtime-dev mailing list