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, [email protected] 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
