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

[Daniel D. Daugherty]

On 8/15/18 7:12 AM, Robbin Ehn wrote:
> Hi David, truncated:
>
> On 2018-08-15 12:40, David Holmes wrote:
>> The context in which this is executed is not at all clear so 
>> assessing the suitability of the fix is also not clear. IIRC a thread 
>> can only be resumed at a safepoint so if the above code executes in 
>> the VMThread then it can't be racing with a resume and the 
>> Threads_lock holding seems unnecessary wrt. this action. ??
>
> The context is, VM thread calls this method to figure out if he can 
> executed the handshake on behalf of the thread in question.
>
> JavaThread::java_resume which does the notify on SR lock, says:
>
> // We need to guarantee the Threads_lock here, since resumes are not
> // allowed during safepoint synchronization
> // Can only resume from an external suspension
> void JavaThread::java_resume() {
>
> And if you look at java_suspend_self() after getting the notification 
> I see nothing that would keep it from continue executing e.g. java 
> code. It can be here with state _thread_in_Java since we skip the 
> proper transitions, thus not even needing todo a transition to 
> continue execute java code.
>
> Is there something I'm missing?

External Java thread suspend can come in from a couple of places:

- JVM/TI SuspendThread() and SuspendThreadList()
- JVM_SuspendThread() which is used by java.lang.Thread.suspend()

External Java thread resume can come in from a a couple of places:

- JVM/TI ResumeThread() and ResumeThreadList()
- JVM_ResumeThread() which is used by java.lang.Thread.resume()

JavaThread::java_resume() has this:

   assert_locked_or_safepoint(Threads_lock);

The two places that call JavaThread::java_resume() have this:

   MutexLocker ml(Threads_lock);

before the call.

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:

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