Hi Dan,
Thanks for looking at this, here is the update:
Inc: http://cr.openjdk.java.net/~rehn/8212933/v2/inc/webrev/
Full: http://cr.openjdk.java.net/~rehn/8212933/v2/webrev/
/Robbin
On 26/10/2018 17:38, Daniel D. Daugherty wrote:
On 10/26/18 10:33 AM, Robbin Ehn wrote:
Hi, please review.
When the VM thread executes a handshake it uses different ThreadsLists during
the execution. A JavaThread that is armed for the handshake when it is already
in the exit path in VM will cancel the handshake. Even if the VM thread cannot
see this thread after the initial ThreadsList which where used for arming, the
handshake can progress when the exiting thread cancels the handshake.
But if a third thread takes a ThreadsList where the exiting JavaThread is
present and tries to execute a VM operation, hence waiting on VM thread to
finish the handshake, the JavaThread in the exit path can never reach the
handshake cancellation point. VM thread cannot finishes the handshake and the
third thread is stuck waiting on the VM thread.
To allow holding a ThreadsList when executing a VM operation we instead let the
VM thread use the same ThreadsList over the entire handshake making all armed
threads visible to the VM thread at all time. And if VM thread spots a
terminated thread it will count that thread is already done by only clearing
it's operation.
Passes local stress testing, t1-5 and the deadlock is no longer reproduce-able.
Added a jtreg handshake + thread suspend test as a reproducer.
Issue: https://bugs.openjdk.java.net/browse/JDK-8212933
Code: http://cr.openjdk.java.net/~rehn/8212933/v1/webrev/
src/hotspot/share/runtime/handshake.hpp
No comments.
src/hotspot/share/runtime/handshake.cpp
L358: void HandshakeState::process_by_vmthread(JavaThread* target) {
L359: assert(Thread::current()->is_VM_thread(), "should call from vm
thread");
Both calls to handshake_process_by_vmthread() which calls this
function are made with the Threads_lock held:
MutexLockerEx ml(Threads_lock, Mutex::_no_safepoint_check_flag);
Looks like the lock is grabbed because of
possibly_vmthread_can_process_handshake() which asserts:
L351: // An externally suspended thread cannot be resumed while the
L352: // Threads_lock is held so it is safe.
L353: // Note that this method is allowed to produce false positives.
L354: assert(Threads_lock->owned_by_self(), "Not holding
Threads_lock.");
L355: if (target->is_ext_suspended()) {
L356: return true;
L357: }
Also looks like vmthread_can_process_handshake() needs the
Threads_lock for the same externally suspended thread check.
So I was going to ask that you add:
assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
after L359, but how about a comment instead:
// Threads_lock must be held here, but that is assert()ed in
// possibly_vmthread_can_process_handshake().
src/hotspot/share/runtime/thread.hpp
No comments.
src/hotspot/share/runtime/thread.cpp
No comments.
src/hotspot/share/runtime/threadSMR.cpp
No comments.
test/hotspot/jtreg/runtime/handshake/HandshakeWalkSuspendExitTest.java
Very nice test! It specifically exercises ThreadLocalHandshakes
with JavaThread suspend/resume. runtime/Thread/SuspendAtExit.java
only ran into this bug by accident (JDK-8212152) so I like the
targeted test.
L49: while(!exit_now) {
nit - please add a space before '('
L51: for (int i = 0; i < _threads.length; i+=2) {
L58: for (int i = 0; i < _threads.length; i+=2) {
nit - please added spaces around '+='
So why every other thread? A comment would be good...
L52: wb.handshakeWalkStack(null, true);
I'm guessing the 'null' parameter means current thread, but
that's a guess on my part. A comment would be good.
L82: for (int i = 0; i < _threads.length; i++) {
L83: _threads[i].join();
L84: }
Thanks for cleaning up the test_threads. That will make
the JTREG thread sweeper happy. However, you don't save
the test_exit_thread references and you don't clean those
up either. Yes, I realize that they are supposed to exit,
but if something hangs up on exit, I'd rather have a join()
hang failure in this test's code than have the JTREG thread
sweeper catch it.
Dan
Thanks, Robbin
