From 0f8e4e0a81257c678e948c341a241dc0b810494f Mon Sep 17 00:00:00 2001 From: Serguei Spitsyn Date: Tue, 19 Dec 2023 17:26:55 +0000 Subject: [PATCH] 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable Reviewed-by: lmesnik, alanb --- make/data/hotspot-symbols/symbols-unix | 1 + src/hotspot/share/classfile/vmIntrinsics.hpp | 1 + src/hotspot/share/classfile/vmSymbols.hpp | 1 + src/hotspot/share/include/jvm.h | 3 + src/hotspot/share/jvmci/vmStructs_jvmci.cpp | 1 + src/hotspot/share/opto/c2compiler.cpp | 1 + src/hotspot/share/opto/library_call.cpp | 26 ++++- src/hotspot/share/opto/library_call.hpp | 1 + src/hotspot/share/prims/jvm.cpp | 16 +++ src/hotspot/share/runtime/handshake.cpp | 4 + src/hotspot/share/runtime/javaThread.cpp | 1 + src/hotspot/share/runtime/javaThread.hpp | 5 + .../classes/java/lang/VirtualThread.java | 82 ++++++++----- .../share/native/libjava/VirtualThread.c | 11 +- .../SuspendWithInterruptLock.java | 108 ++++++++++++++++++ 15 files changed, 229 insertions(+), 33 deletions(-) create mode 100644 test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java diff --git a/make/data/hotspot-symbols/symbols-unix b/make/data/hotspot-symbols/symbols-unix index 9ca040794f3..fbb82a11fac 100644 --- a/make/data/hotspot-symbols/symbols-unix +++ b/make/data/hotspot-symbols/symbols-unix @@ -223,6 +223,7 @@ JVM_VirtualThreadEnd JVM_VirtualThreadMount JVM_VirtualThreadUnmount JVM_VirtualThreadHideFrames +JVM_VirtualThreadDisableSuspend # Scoped values JVM_EnsureMaterializedForStackWalk_func diff --git a/src/hotspot/share/classfile/vmIntrinsics.hpp b/src/hotspot/share/classfile/vmIntrinsics.hpp index 8ed99d74c0b..c9bc4acbeff 100644 --- a/src/hotspot/share/classfile/vmIntrinsics.hpp +++ b/src/hotspot/share/classfile/vmIntrinsics.hpp @@ -597,6 +597,7 @@ class methodHandle; do_intrinsic(_notifyJvmtiVThreadMount, java_lang_VirtualThread, notifyJvmtiMount_name, bool_void_signature, F_RN) \ do_intrinsic(_notifyJvmtiVThreadUnmount, java_lang_VirtualThread, notifyJvmtiUnmount_name, bool_void_signature, F_RN) \ do_intrinsic(_notifyJvmtiVThreadHideFrames, java_lang_VirtualThread, notifyJvmtiHideFrames_name, bool_void_signature, F_RN) \ + do_intrinsic(_notifyJvmtiVThreadDisableSuspend, java_lang_VirtualThread, notifyJvmtiDisableSuspend_name, bool_void_signature, F_RN) \ \ /* support for UnsafeConstants */ \ do_class(jdk_internal_misc_UnsafeConstants, "jdk/internal/misc/UnsafeConstants") \ diff --git a/src/hotspot/share/classfile/vmSymbols.hpp b/src/hotspot/share/classfile/vmSymbols.hpp index 16ddc2602b1..fb4b059b5bd 100644 --- a/src/hotspot/share/classfile/vmSymbols.hpp +++ b/src/hotspot/share/classfile/vmSymbols.hpp @@ -421,6 +421,7 @@ class SerializeClosure; template(notifyJvmtiMount_name, "notifyJvmtiMount") \ template(notifyJvmtiUnmount_name, "notifyJvmtiUnmount") \ template(notifyJvmtiHideFrames_name, "notifyJvmtiHideFrames") \ + template(notifyJvmtiDisableSuspend_name, "notifyJvmtiDisableSuspend") \ template(doYield_name, "doYield") \ template(enter_name, "enter") \ template(enterSpecial_name, "enterSpecial") \ diff --git a/src/hotspot/share/include/jvm.h b/src/hotspot/share/include/jvm.h index cc1b3d94cbd..5d6ab27a3a1 100644 --- a/src/hotspot/share/include/jvm.h +++ b/src/hotspot/share/include/jvm.h @@ -1154,6 +1154,9 @@ JVM_VirtualThreadUnmount(JNIEnv* env, jobject vthread, jboolean hide); JNIEXPORT void JNICALL JVM_VirtualThreadHideFrames(JNIEnv* env, jobject vthread, jboolean hide); +JNIEXPORT void JNICALL +JVM_VirtualThreadDisableSuspend(JNIEnv* env, jobject vthread, jboolean enter); + /* * Core reflection support. */ diff --git a/src/hotspot/share/jvmci/vmStructs_jvmci.cpp b/src/hotspot/share/jvmci/vmStructs_jvmci.cpp index 5fdc70d7bc7..bac9a786fd9 100644 --- a/src/hotspot/share/jvmci/vmStructs_jvmci.cpp +++ b/src/hotspot/share/jvmci/vmStructs_jvmci.cpp @@ -220,6 +220,7 @@ nonstatic_field(JavaThread, _lock_stack, LockStack) \ JVMTI_ONLY(nonstatic_field(JavaThread, _is_in_VTMS_transition, bool)) \ JVMTI_ONLY(nonstatic_field(JavaThread, _is_in_tmp_VTMS_transition, bool)) \ + JVMTI_ONLY(nonstatic_field(JavaThread, _is_disable_suspend, bool)) \ \ nonstatic_field(LockStack, _top, uint32_t) \ \ diff --git a/src/hotspot/share/opto/c2compiler.cpp b/src/hotspot/share/opto/c2compiler.cpp index 5e9dfcacafd..ba383a679e1 100644 --- a/src/hotspot/share/opto/c2compiler.cpp +++ b/src/hotspot/share/opto/c2compiler.cpp @@ -822,6 +822,7 @@ bool C2Compiler::is_intrinsic_supported(vmIntrinsics::ID id) { case vmIntrinsics::_notifyJvmtiVThreadMount: case vmIntrinsics::_notifyJvmtiVThreadUnmount: case vmIntrinsics::_notifyJvmtiVThreadHideFrames: + case vmIntrinsics::_notifyJvmtiVThreadDisableSuspend: #endif break; diff --git a/src/hotspot/share/opto/library_call.cpp b/src/hotspot/share/opto/library_call.cpp index 07504199f81..e8923d3d134 100644 --- a/src/hotspot/share/opto/library_call.cpp +++ b/src/hotspot/share/opto/library_call.cpp @@ -492,7 +492,8 @@ bool LibraryCallKit::try_to_inline(int predicate) { "notifyJvmtiMount", false, false); case vmIntrinsics::_notifyJvmtiVThreadUnmount: return inline_native_notify_jvmti_funcs(CAST_FROM_FN_PTR(address, OptoRuntime::notify_jvmti_vthread_unmount()), "notifyJvmtiUnmount", false, false); - case vmIntrinsics::_notifyJvmtiVThreadHideFrames: return inline_native_notify_jvmti_hide(); + case vmIntrinsics::_notifyJvmtiVThreadHideFrames: return inline_native_notify_jvmti_hide(); + case vmIntrinsics::_notifyJvmtiVThreadDisableSuspend: return inline_native_notify_jvmti_sync(); #endif #ifdef JFR_HAVE_INTRINSICS @@ -2950,6 +2951,29 @@ bool LibraryCallKit::inline_native_notify_jvmti_hide() { return true; } +// Always update the is_disable_suspend bit. +bool LibraryCallKit::inline_native_notify_jvmti_sync() { + if (!DoJVMTIVirtualThreadTransitions) { + return true; + } + IdealKit ideal(this); + + { + // unconditionally update the is_disable_suspend bit in current JavaThread + Node* thread = ideal.thread(); + Node* arg = _gvn.transform(argument(1)); // argument for notification + Node* addr = basic_plus_adr(thread, in_bytes(JavaThread::is_disable_suspend_offset())); + const TypePtr *addr_type = _gvn.type(addr)->isa_ptr(); + + sync_kit(ideal); + access_store_at(nullptr, addr, addr_type, arg, _gvn.type(arg), T_BOOLEAN, IN_NATIVE | MO_UNORDERED); + ideal.sync_kit(this); + } + final_sync(ideal); + + return true; +} + #endif // INCLUDE_JVMTI #ifdef JFR_HAVE_INTRINSICS diff --git a/src/hotspot/share/opto/library_call.hpp b/src/hotspot/share/opto/library_call.hpp index 55d1dc78f1f..47f9d7d4713 100644 --- a/src/hotspot/share/opto/library_call.hpp +++ b/src/hotspot/share/opto/library_call.hpp @@ -245,6 +245,7 @@ class LibraryCallKit : public GraphKit { #if INCLUDE_JVMTI bool inline_native_notify_jvmti_funcs(address funcAddr, const char* funcName, bool is_start, bool is_end); bool inline_native_notify_jvmti_hide(); + bool inline_native_notify_jvmti_sync(); #endif #ifdef JFR_HAVE_INTRINSICS diff --git a/src/hotspot/share/prims/jvm.cpp b/src/hotspot/share/prims/jvm.cpp index f0a30ee0311..bd6eda2d468 100644 --- a/src/hotspot/share/prims/jvm.cpp +++ b/src/hotspot/share/prims/jvm.cpp @@ -4008,6 +4008,22 @@ JVM_ENTRY(void, JVM_VirtualThreadHideFrames(JNIEnv* env, jobject vthread, jboole #endif JVM_END +// Notification from VirtualThread about disabling JVMTI Suspend in a sync critical section. +// Needed to avoid deadlocks with JVMTI suspend mechanism. +JVM_ENTRY(void, JVM_VirtualThreadDisableSuspend(JNIEnv* env, jobject vthread, jboolean enter)) +#if INCLUDE_JVMTI + if (!DoJVMTIVirtualThreadTransitions) { + assert(!JvmtiExport::can_support_virtual_threads(), "sanity check"); + return; + } + assert(thread->is_disable_suspend() != (bool)enter, + "nested or unbalanced monitor enter/exit is not allowed"); + thread->toggle_is_disable_suspend(); +#else + fatal("Should only be called with JVMTI enabled"); +#endif +JVM_END + /* * Return the current class's class file version. The low order 16 bits of the * returned jint contain the class's major version. The high order 16 bits diff --git a/src/hotspot/share/runtime/handshake.cpp b/src/hotspot/share/runtime/handshake.cpp index 50c93d666e2..a57076c021a 100644 --- a/src/hotspot/share/runtime/handshake.cpp +++ b/src/hotspot/share/runtime/handshake.cpp @@ -487,6 +487,10 @@ HandshakeOperation* HandshakeState::get_op_for_self(bool allow_suspend, bool che assert(_handshakee == Thread::current(), "Must be called by self"); assert(_lock.owned_by_self(), "Lock must be held"); assert(allow_suspend || !check_async_exception, "invalid case"); + if (allow_suspend && _handshakee->is_disable_suspend()) { + // filter out suspend operations while JavaThread is in disable_suspend mode + allow_suspend = false; + } if (!allow_suspend) { return _queue.peek(no_suspend_no_async_exception_filter); } else if (check_async_exception && !_async_exceptions_blocked) { diff --git a/src/hotspot/share/runtime/javaThread.cpp b/src/hotspot/share/runtime/javaThread.cpp index 98b2a569b72..a789988e379 100644 --- a/src/hotspot/share/runtime/javaThread.cpp +++ b/src/hotspot/share/runtime/javaThread.cpp @@ -440,6 +440,7 @@ JavaThread::JavaThread() : _carrier_thread_suspended(false), _is_in_VTMS_transition(false), _is_in_tmp_VTMS_transition(false), + _is_disable_suspend(false), #ifdef ASSERT _is_VTMS_transition_disabler(false), #endif diff --git a/src/hotspot/share/runtime/javaThread.hpp b/src/hotspot/share/runtime/javaThread.hpp index 71bf0cc1d49..fb1355b852a 100644 --- a/src/hotspot/share/runtime/javaThread.hpp +++ b/src/hotspot/share/runtime/javaThread.hpp @@ -317,6 +317,7 @@ class JavaThread: public Thread { volatile bool _carrier_thread_suspended; // Carrier thread is externally suspended bool _is_in_VTMS_transition; // thread is in virtual thread mount state transition bool _is_in_tmp_VTMS_transition; // thread is in temporary virtual thread mount state transition + bool _is_disable_suspend; // JVMTI suspend is temporarily disabled; used on current thread only #ifdef ASSERT bool _is_VTMS_transition_disabler; // thread currently disabled VTMS transitions #endif @@ -647,6 +648,9 @@ private: void set_is_in_VTMS_transition(bool val); void toggle_is_in_tmp_VTMS_transition() { _is_in_tmp_VTMS_transition = !_is_in_tmp_VTMS_transition; }; + bool is_disable_suspend() const { return _is_disable_suspend; } + void toggle_is_disable_suspend() { _is_disable_suspend = !_is_disable_suspend; }; + #ifdef ASSERT bool is_VTMS_transition_disabler() const { return _is_VTMS_transition_disabler; } void set_is_VTMS_transition_disabler(bool val); @@ -811,6 +815,7 @@ private: #if INCLUDE_JVMTI static ByteSize is_in_VTMS_transition_offset() { return byte_offset_of(JavaThread, _is_in_VTMS_transition); } static ByteSize is_in_tmp_VTMS_transition_offset() { return byte_offset_of(JavaThread, _is_in_tmp_VTMS_transition); } + static ByteSize is_disable_suspend_offset() { return byte_offset_of(JavaThread, _is_disable_suspend); } #endif // Returns the jni environment for this thread diff --git a/src/java.base/share/classes/java/lang/VirtualThread.java b/src/java.base/share/classes/java/lang/VirtualThread.java index 2707b3ba9bd..13ba543348e 100644 --- a/src/java.base/share/classes/java/lang/VirtualThread.java +++ b/src/java.base/share/classes/java/lang/VirtualThread.java @@ -743,11 +743,16 @@ final class VirtualThread extends BaseVirtualThread { } } else if ((s == PINNED) || (s == TIMED_PINNED)) { // unpark carrier thread when pinned - synchronized (carrierThreadAccessLock()) { - Thread carrier = carrierThread; - if (carrier != null && ((s = state()) == PINNED || s == TIMED_PINNED)) { - U.unpark(carrier); + notifyJvmtiDisableSuspend(true); + try { + synchronized (carrierThreadAccessLock()) { + Thread carrier = carrierThread; + if (carrier != null && ((s = state()) == PINNED || s == TIMED_PINNED)) { + U.unpark(carrier); + } } + } finally { + notifyJvmtiDisableSuspend(false); } } } @@ -844,16 +849,21 @@ final class VirtualThread extends BaseVirtualThread { public void interrupt() { if (Thread.currentThread() != this) { checkAccess(); - synchronized (interruptLock) { - interrupted = true; - Interruptible b = nioBlocker; - if (b != null) { - b.interrupt(this); - } + notifyJvmtiDisableSuspend(true); + try { + synchronized (interruptLock) { + interrupted = true; + Interruptible b = nioBlocker; + if (b != null) { + b.interrupt(this); + } - // interrupt carrier thread if mounted - Thread carrier = carrierThread; - if (carrier != null) carrier.setInterrupt(); + // interrupt carrier thread if mounted + Thread carrier = carrierThread; + if (carrier != null) carrier.setInterrupt(); + } + } finally { + notifyJvmtiDisableSuspend(false); } } else { interrupted = true; @@ -872,9 +882,14 @@ final class VirtualThread extends BaseVirtualThread { assert Thread.currentThread() == this; boolean oldValue = interrupted; if (oldValue) { - synchronized (interruptLock) { - interrupted = false; - carrierThread.clearInterrupt(); + notifyJvmtiDisableSuspend(true); + try { + synchronized (interruptLock) { + interrupted = false; + carrierThread.clearInterrupt(); + } + } finally { + notifyJvmtiDisableSuspend(false); } } return oldValue; @@ -899,11 +914,16 @@ final class VirtualThread extends BaseVirtualThread { return Thread.State.RUNNABLE; case RUNNING: // if mounted then return state of carrier thread - synchronized (carrierThreadAccessLock()) { - Thread carrierThread = this.carrierThread; - if (carrierThread != null) { - return carrierThread.threadState(); + notifyJvmtiDisableSuspend(true); + try { + synchronized (carrierThreadAccessLock()) { + Thread carrierThread = this.carrierThread; + if (carrierThread != null) { + return carrierThread.threadState(); + } } + } finally { + notifyJvmtiDisableSuspend(false); } // runnable, mounted return Thread.State.RUNNABLE; @@ -1019,14 +1039,19 @@ final class VirtualThread extends BaseVirtualThread { Thread carrier = carrierThread; if (carrier != null) { // include the carrier thread state and name when mounted - synchronized (carrierThreadAccessLock()) { - carrier = carrierThread; - if (carrier != null) { - String stateAsString = carrier.threadState().toString(); - sb.append(stateAsString.toLowerCase(Locale.ROOT)); - sb.append('@'); - sb.append(carrier.getName()); + notifyJvmtiDisableSuspend(true); + try { + synchronized (carrierThreadAccessLock()) { + carrier = carrierThread; + if (carrier != null) { + String stateAsString = carrier.threadState().toString(); + sb.append(stateAsString.toLowerCase(Locale.ROOT)); + sb.append('@'); + sb.append(carrier.getName()); + } } + } finally { + notifyJvmtiDisableSuspend(false); } } // include virtual thread state when not mounted @@ -1125,6 +1150,9 @@ final class VirtualThread extends BaseVirtualThread { @JvmtiMountTransition private native void notifyJvmtiHideFrames(boolean hide); + @IntrinsicCandidate + private native void notifyJvmtiDisableSuspend(boolean enter); + private static native void registerNatives(); static { registerNatives(); diff --git a/src/java.base/share/native/libjava/VirtualThread.c b/src/java.base/share/native/libjava/VirtualThread.c index a0b3e082087..94dbe0b7e37 100644 --- a/src/java.base/share/native/libjava/VirtualThread.c +++ b/src/java.base/share/native/libjava/VirtualThread.c @@ -32,11 +32,12 @@ #define VIRTUAL_THREAD "Ljava/lang/VirtualThread;" static JNINativeMethod methods[] = { - { "notifyJvmtiStart", "()V", (void *)&JVM_VirtualThreadStart }, - { "notifyJvmtiEnd", "()V", (void *)&JVM_VirtualThreadEnd }, - { "notifyJvmtiMount", "(Z)V", (void *)&JVM_VirtualThreadMount }, - { "notifyJvmtiUnmount", "(Z)V", (void *)&JVM_VirtualThreadUnmount }, - { "notifyJvmtiHideFrames", "(Z)V", (void *)&JVM_VirtualThreadHideFrames }, + { "notifyJvmtiStart", "()V", (void *)&JVM_VirtualThreadStart }, + { "notifyJvmtiEnd", "()V", (void *)&JVM_VirtualThreadEnd }, + { "notifyJvmtiMount", "(Z)V", (void *)&JVM_VirtualThreadMount }, + { "notifyJvmtiUnmount", "(Z)V", (void *)&JVM_VirtualThreadUnmount }, + { "notifyJvmtiHideFrames", "(Z)V", (void *)&JVM_VirtualThreadHideFrames }, + { "notifyJvmtiDisableSuspend", "(Z)V", (void *)&JVM_VirtualThreadDisableSuspend }, }; JNIEXPORT void JNICALL diff --git a/test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java b/test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java new file mode 100644 index 00000000000..d679f382d93 --- /dev/null +++ b/test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ + +/** + * @test id=default + * @summary Do not suspend virtual threads in a critical section. + * @bug 8311218 + * @requires vm.continuations + * @library /testlibrary + * @run main/othervm SuspendWithInterruptLock + */ + +/** + * @test id=xint + * @summary Do not suspend virtual threads in a critical section. + * @bug 8311218 + * @requires vm.continuations + * @library /testlibrary + * @run main/othervm -Xint SuspendWithInterruptLock + */ + +import jvmti.JVMTIUtils; + +public class SuspendWithInterruptLock { + static volatile boolean done; + + public static void main(String[] args) throws Exception { + Thread yielder = Thread.ofVirtual().name("yielder").start(() -> yielder()); + Thread stateReader = Thread.ofVirtual().name("stateReader").start(() -> stateReader(yielder)); + Thread suspender = new Thread(() -> suspender(stateReader)); + suspender.start(); + + yielder.join(); + stateReader.join(); + suspender.join(); + } + + static private void yielder() { + int iterations = 100_000; + while (iterations-- > 0) { + Thread.yield(); + } + done = true; + } + + static private void stateReader(Thread target) { + while (!done) { + target.getState(); + } + } + + static private void suspender(Thread target) { + while (!done) { + suspendThread(target); + sleep(1); + resumeThread(target); + // Allow progress + sleep(5); + } + } + + static void suspendThread(Thread t) { + try { + JVMTIUtils.suspendThread(t); + } catch (JVMTIUtils.JvmtiException e) { + if (e.getCode() != JVMTIUtils.JVMTI_ERROR_THREAD_NOT_ALIVE) { + throw e; + } + } + } + + static void resumeThread(Thread t) { + try { + JVMTIUtils.resumeThread(t); + } catch (JVMTIUtils.JvmtiException e) { + if (e.getCode() != JVMTIUtils.JVMTI_ERROR_THREAD_NOT_ALIVE) { + throw e; + } + } + } + + static private void sleep(long millis) { + try { + Thread.sleep(millis); + } catch (InterruptedException e) {} + } +} +