RFR: 8217618: JVM TI SuspendThread doesn't suspend the current thread before returning

[Daniel D. Daugherty]

On 1/26/19 7:06 PM, David Holmes wrote:
> On 27/01/2019 6:33 am, dean.long at oracle.com wrote:
>> I think there is going to be a race with resume wherever you put the 
>> self-suspend, because we do it without holding SR_lock.  So you 
>> should be able to move the self check and self suspend to before the 
>> SR_lock. This would just be a performance optimization.

Dean, let's see if I understand what race you're worried about.

The JavaThread is suspending itself so it has to be resumed from
another thread. The other thread cannot reliably call ResumeThread()
until it has observed the target thread as suspended via
GetThreadStatus(). The relevant thread status bits are set here:

open/src/hotspot/share/prims/jvmtiEnv.cpp:

     if (java_thread->is_being_ext_suspended()) {
       state |= JVMTI_THREAD_STATE_SUSPENDED;
     }

open/src/hotspot/share/runtime/thread.hpp:

   // utility methods to see if we are doing some kind of suspension
   bool is_being_ext_suspended() const            {
     MutexLockerEx ml(SR_lock(), Mutex::_no_safepoint_check_flag);
     return is_ext_suspended() || is_external_suspend();
   }

So it looks like 'other thread' can see is_external_suspend() on the
target thread and call ResumeThread(). And yes, that ResumeThread()
can happen after the SR_lock() is dropped in java_suspend() and
before the new java_suspend_self() call.

However, java_suspend_self() is careful and only self-suspends if
is_external_suspend() is still true (and it makes that check while
it owns the SR_lock). The code in java_suspend_self() has to be
careful in both directions. You don't want it to self-suspend
when it has been resumed by a racer and you don't want it to
resume if there was another SuspendThread() call was made and
has returned to it's caller. Check out the comments in
java_suspend_self(); they should help clarify things.


> I would prefer not to skip the resume checks even if they are racy.

The existing resume checks are not racy because the SR_lock is
held.


> The logic is very obvious in the current form: safepoint for "other" 
> thread else self-suspend.

Agreed.

Dan


>
> Thanks,
> David
>
>
>> dl
>>
>> On 1/24/19 6:46 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217618
>>> webrev: http://cr.openjdk.java.net/~dholmes/8217618/webrev/
>>>
>>> Lots of analysis in the bug report. Bottom line: SuspendThread of 
>>> the current thread was not actually suspending the thread until it 
>>> hit specific thread-state transitions. That meant that SuspendThread 
>>> would actually return and continue executing native code whilst 
>>> suspended, in violation of the specification for it.
>>>
>>> The fix is quite simple: in java_suspend() we check for the current 
>>> thread and call java_suspend_self().
>>>
>>> Testing:
>>>  - Any test that looked like it referred to thread suspension
>>>   - hotspot/jtreg/vmTestbase/nsk/jvmti/*
>>>   - jdk/
>>>      com/sun/jdi/*
>>>      java/lang/ThreadGroup/Suspend.java
>>> java/lang/management/CompositeData/ThreadInfoCompositeData.java
>>>      java/lang/management/ThreadMXBean/*
>>>      java/nio/channels/SocketChannel/SendUrgentData.java
>>> java/util/logging/LogManager/Configuration/TestConfigurationLock.java
>>>
>>>  - Mach 5 tiers 1-3 (in progress)
>>>
>>> Two tests were found to be erroneously relying on SuspendThread 
>>> returning whilst suspended:
>>>
>>> - vmTestbase/nsk/jvmti/scenarios/sampling/SP05/sp05t003/sp05t003.cpp
>>>
>>> The test updated a shared counter after the SuspendThread call, but 
>>> it needed to be updated before the call.
>>>
>>> - vmTestbase/nsk/jvmti/scenarios/hotswap/HS202/hs202t002/hs202t002.cpp
>>>
>>> The test was using a 0 return value from SuspendThread as an 
>>> indicator that the thread was in the suspended state - but that 
>>> value can't be seen until after SuspendThread returns, which is 
>>> after the thread is resumed. So I ripped out the custom state 
>>> tracking logic and replaced with a simple check of GetThreadState.
>>>
>>> Thanks,
>>> David
>>