[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8217618: JVM TI SuspendThread doesn't suspend the current thread before returning
From:       David Holmes <david.holmes () oracle ! com>
Date:       2019-01-28 1:49:59
Message-ID: 85b0762a-f74b-37de-8c92-b357e13f5e5c () oracle ! com
[Download RAW message or body]

Thanks for explaining that Dan!

David

On 27/01/2019 1:43 pm, Daniel D. Daugherty wrote:
> On 1/26/19 7:06 PM, David Holmes wrote:
>> On 27/01/2019 6:33 am, dean.long@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
>>>
> 
[prev in list] [next in list] [prev in thread] [next in thread]