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

[Robbin Ehn]

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();
  }

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