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

[Robbin Ehn]

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

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