From 43e1baffe0f6e1148636c5f5a4a2fc2e07521326 Mon Sep 17 00:00:00 2001 From: xlu Date: Mon, 6 Apr 2009 15:47:39 -0700 Subject: [PATCH] 6699669: Hotspot server leaves synchronized block with monitor in bad state Summary: Remove usage of _highest_lock field in Thread so that is_lock_owned won't depend on the correct update of that field. Reviewed-by: never, dice, acorn --- .../sun/jvm/hotspot/runtime/JavaThread.java | 25 +++++++++++-- .../sun/jvm/hotspot/runtime/Thread.java | 6 --- .../sun/jvm/hotspot/runtime/Threads.java | 15 ++------ src/share/vm/runtime/javaCalls.cpp | 5 --- src/share/vm/runtime/synchronizer.cpp | 9 ++--- src/share/vm/runtime/thread.cpp | 37 +++---------------- src/share/vm/runtime/thread.hpp | 18 ++------- src/share/vm/runtime/vmStructs.cpp | 1 - 8 files changed, 37 insertions(+), 79 deletions(-) diff --git a/agent/src/share/classes/sun/jvm/hotspot/runtime/JavaThread.java b/agent/src/share/classes/sun/jvm/hotspot/runtime/JavaThread.java index 204156c3f..d3622b050 100644 --- a/agent/src/share/classes/sun/jvm/hotspot/runtime/JavaThread.java +++ b/agent/src/share/classes/sun/jvm/hotspot/runtime/JavaThread.java @@ -48,6 +48,8 @@ public class JavaThread extends Thread { private static AddressField lastJavaPCField; private static CIntegerField threadStateField; private static AddressField osThreadField; + private static AddressField stackBaseField; + private static CIntegerField stackSizeField; private static JavaThreadPDAccess access; @@ -83,6 +85,8 @@ public class JavaThread extends Thread { lastJavaPCField = anchorType.getAddressField("_last_Java_pc"); threadStateField = type.getCIntegerField("_thread_state"); osThreadField = type.getAddressField("_osthread"); + stackBaseField = type.getAddressField("_stack_base"); + stackSizeField = type.getCIntegerField("_stack_size"); UNINITIALIZED = db.lookupIntConstant("_thread_uninitialized").intValue(); NEW = db.lookupIntConstant("_thread_new").intValue(); @@ -312,6 +316,14 @@ public class JavaThread extends Thread { return (OSThread) VMObjectFactory.newObject(OSThread.class, osThreadField.getValue(addr)); } + public Address getStackBase() { + return stackBaseField.getValue(); + } + + public long getStackSize() { + return stackSizeField.getValue(); + } + /** Gets the Java-side thread object for this JavaThread */ public Oop getThreadObj() { return VM.getVM().getObjectHeap().newOop(threadObjField.getValue(addr)); @@ -345,11 +357,18 @@ public class JavaThread extends Thread { if (Assert.ASSERTS_ENABLED) { Assert.that(VM.getVM().isDebugging(), "Not yet implemented for non-debugging system"); } - Address highest = highestLock(); Address sp = lastSPDbg(); + Address stackBase = getStackBase(); // Be robust - if ((highest == null) || (sp == null)) return false; - return (highest.greaterThanOrEqual(a) && sp.lessThanOrEqual(a)); + if (sp == null) return false; + return stackBase.greaterThanOrEqual(a) && sp.lessThanOrEqual(a); + } + + public boolean isLockOwned(Address a) { + Address stackBase = getStackBase(); + Address stackLimit = stackBase.addOffsetTo(-getStackSize()); + + return stackBase.greaterThanOrEqual(a) && stackLimit.lessThanOrEqual(a); // FIXME: should traverse MonitorArray/MonitorChunks as in VM } diff --git a/agent/src/share/classes/sun/jvm/hotspot/runtime/Thread.java b/agent/src/share/classes/sun/jvm/hotspot/runtime/Thread.java index 2b28e9fb9..9ca93b931 100644 --- a/agent/src/share/classes/sun/jvm/hotspot/runtime/Thread.java +++ b/agent/src/share/classes/sun/jvm/hotspot/runtime/Thread.java @@ -38,7 +38,6 @@ public class Thread extends VMObject { private static int HAS_ASYNC_EXCEPTION; private static AddressField activeHandlesField; - private static AddressField highestLockField; private static AddressField currentPendingMonitorField; private static AddressField currentWaitingMonitorField; @@ -60,7 +59,6 @@ public class Thread extends VMObject { tlabFieldOffset = type.getField("_tlab").getOffset(); activeHandlesField = type.getAddressField("_active_handles"); - highestLockField = type.getAddressField("_highest_lock"); currentPendingMonitorField = type.getAddressField("_current_pending_monitor"); currentWaitingMonitorField = type.getAddressField("_current_waiting_monitor"); } @@ -121,10 +119,6 @@ public class Thread extends VMObject { // pending exception } - public Address highestLock() { - return highestLockField.getValue(addr); - } - public ObjectMonitor getCurrentPendingMonitor() { Address monitorAddr = currentPendingMonitorField.getValue(addr); if (monitorAddr == null) { diff --git a/agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java b/agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java index 1f7c9a1af..9be499b5b 100644 --- a/agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java +++ b/agent/src/share/classes/sun/jvm/hotspot/runtime/Threads.java @@ -164,20 +164,11 @@ public class Threads { } } - long leastDiff = 0; - boolean leastDiffInitialized = false; - JavaThread theOwner = null; for (JavaThread thread = first(); thread != null; thread = thread.next()) { - Address addr = thread.highestLock(); - if (addr == null || addr.lessThan(o)) continue; - long diff = addr.minus(o); - if (!leastDiffInitialized || diff < leastDiff) { - leastDiffInitialized = true; - leastDiff = diff; - theOwner = thread; - } + if (thread.isLockOwned(o)) + return thread; } - return theOwner; + return null; } public JavaThread owningThreadFromMonitor(ObjectMonitor monitor) { diff --git a/src/share/vm/runtime/javaCalls.cpp b/src/share/vm/runtime/javaCalls.cpp index 92773e809..a64fdb07a 100644 --- a/src/share/vm/runtime/javaCalls.cpp +++ b/src/share/vm/runtime/javaCalls.cpp @@ -37,11 +37,6 @@ JavaCallWrapper::JavaCallWrapper(methodHandle callee_method, Handle receiver, Ja guarantee(!thread->is_Compiler_thread(), "cannot make java calls from the compiler"); _result = result; - // Make sure that that the value of the higest_lock is at least the same as the current stackpointer, - // since, the Java code is highly likely to use locks. - // Use '(address)this' to guarantee that highest_lock address is conservative and inside our thread - thread->update_highest_lock((address)this); - // Allocate handle block for Java code. This must be done before we change thread_state to _thread_in_Java_or_stub, // since it can potentially block. JNIHandleBlock* new_handles = JNIHandleBlock::allocate_block(thread); diff --git a/src/share/vm/runtime/synchronizer.cpp b/src/share/vm/runtime/synchronizer.cpp index ca6bdb13a..dc77a6848 100644 --- a/src/share/vm/runtime/synchronizer.cpp +++ b/src/share/vm/runtime/synchronizer.cpp @@ -1117,10 +1117,10 @@ ObjectMonitor * ATTR ObjectSynchronizer::inflate (Thread * Self, oop object) { // Optimization: if the mark->locker stack address is associated // with this thread we could simply set m->_owner = Self and - // m->OwnerIsThread = 1. Note that a thread can inflate an object + // m->OwnerIsThread = 1. Note that a thread can inflate an object // that it has stack-locked -- as might happen in wait() -- directly // with CAS. That is, we can avoid the xchg-NULL .... ST idiom. - m->set_owner (mark->locker()); + m->set_owner(mark->locker()); m->set_object(object); // TODO-FIXME: assert BasicLock->dhw != 0. @@ -1214,10 +1214,9 @@ void ObjectSynchronizer::fast_enter(Handle obj, BasicLock* lock, bool attempt_re BiasedLocking::revoke_at_safepoint(obj); } assert(!obj->mark()->has_bias_pattern(), "biases should be revoked by now"); - } + } - THREAD->update_highest_lock((address)lock); - slow_enter (obj, lock, THREAD) ; + slow_enter (obj, lock, THREAD) ; } void ObjectSynchronizer::fast_exit(oop object, BasicLock* lock, TRAPS) { diff --git a/src/share/vm/runtime/thread.cpp b/src/share/vm/runtime/thread.cpp index 547ea7fe6..1834d1491 100644 --- a/src/share/vm/runtime/thread.cpp +++ b/src/share/vm/runtime/thread.cpp @@ -128,7 +128,6 @@ Thread::Thread() { debug_only(_allow_allocation_count = 0;) NOT_PRODUCT(_allow_safepoint_count = 0;) CHECK_UNHANDLED_OOPS_ONLY(_gc_locked_out_count = 0;) - _highest_lock = NULL; _jvmti_env_iteration_count = 0; _vm_operation_started_count = 0; _vm_operation_completed_count = 0; @@ -790,19 +789,6 @@ void Thread::check_for_valid_safepoint_state(bool potential_vm_operation) { } #endif -bool Thread::lock_is_in_stack(address adr) const { - assert(Thread::current() == this, "lock_is_in_stack can only be called from current thread"); - // High limit: highest_lock is set during thread execution - // Low limit: address of the local variable dummy, rounded to 4K boundary. - // (The rounding helps finding threads in unsafe mode, even if the particular stack - // frame has been popped already. Correct as long as stacks are at least 4K long and aligned.) - address end = os::current_stack_pointer(); - if (_highest_lock >= adr && adr >= end) return true; - - return false; -} - - bool Thread::is_in_stack(address adr) const { assert(Thread::current() == this, "is_in_stack can only be called from current thread"); address end = os::current_stack_pointer(); @@ -818,8 +804,7 @@ bool Thread::is_in_stack(address adr) const { // should be revisited, and they should be removed if possible. bool Thread::is_lock_owned(address adr) const { - if (lock_is_in_stack(adr) ) return true; - return false; + return (_stack_base >= adr && adr >= (_stack_base - _stack_size)); } bool Thread::set_as_starting_thread() { @@ -1664,7 +1649,7 @@ JavaThread* JavaThread::active() { } bool JavaThread::is_lock_owned(address adr) const { - if (lock_is_in_stack(adr)) return true; + if (Thread::is_lock_owned(adr)) return true; for (MonitorChunk* chunk = monitor_chunks(); chunk != NULL; chunk = chunk->next()) { if (chunk->contains(adr)) return true; @@ -2443,7 +2428,7 @@ void JavaThread::print_on(outputStream *st) const { if (thread_oop != NULL && java_lang_Thread::is_daemon(thread_oop)) st->print("daemon "); Thread::print_on(st); // print guess for valid stack memory region (assume 4K pages); helps lock debugging - st->print_cr("[" INTPTR_FORMAT ".." INTPTR_FORMAT "]", (intptr_t)last_Java_sp() & ~right_n_bits(12), highest_lock()); + st->print_cr("[" INTPTR_FORMAT "]", (intptr_t)last_Java_sp() & ~right_n_bits(12)); if (thread_oop != NULL && JDK_Version::is_gte_jdk15x_version()) { st->print_cr(" java.lang.Thread.State: %s", java_lang_Thread::thread_status_name(thread_oop)); } @@ -3733,25 +3718,13 @@ JavaThread *Threads::owning_thread_from_monitor_owner(address owner, bool doLock // heavyweight monitors, then the owner is the stack address of the // Lock Word in the owning Java thread's stack. // - // We can't use Thread::is_lock_owned() or Thread::lock_is_in_stack() because - // those routines rely on the "current" stack pointer. That would be our - // stack pointer which is not relevant to the question. Instead we use the - // highest lock ever entered by the thread and find the thread that is - // higher than and closest to our target stack address. - // - address least_diff = 0; - bool least_diff_initialized = false; JavaThread* the_owner = NULL; { MutexLockerEx ml(doLock ? Threads_lock : NULL); ALL_JAVA_THREADS(q) { - address addr = q->highest_lock(); - if (addr == NULL || addr < owner) continue; // thread has entered no monitors or is too low - address diff = (address)(addr - owner); - if (!least_diff_initialized || diff < least_diff) { - least_diff_initialized = true; - least_diff = diff; + if (q->is_lock_owned(owner)) { the_owner = q; + break; } } } diff --git a/src/share/vm/runtime/thread.hpp b/src/share/vm/runtime/thread.hpp index 59aea77b9..9043da17f 100644 --- a/src/share/vm/runtime/thread.hpp +++ b/src/share/vm/runtime/thread.hpp @@ -200,14 +200,6 @@ class Thread: public ThreadShadow { friend class ThreadLocalStorage; friend class GC_locker; - // In order for all threads to be able to use fast locking, we need to know the highest stack - // address of where a lock is on the stack (stacks normally grow towards lower addresses). This - // variable is initially set to NULL, indicating no locks are used by the thread. During the thread's - // execution, it will be set whenever locking can happen, i.e., when we call out to Java code or use - // an ObjectLocker. The value is never decreased, hence, it will over the lifetime of a thread - // approximate the real stackbase. - address _highest_lock; // Highest stack address where a JavaLock exist - ThreadLocalAllocBuffer _tlab; // Thread-local eden int _vm_operation_started_count; // VM_Operation support @@ -400,18 +392,14 @@ public: // Sweeper support void nmethods_do(); - // Fast-locking support - address highest_lock() const { return _highest_lock; } - void update_highest_lock(address base) { if (base > _highest_lock) _highest_lock = base; } - // Tells if adr belong to this thread. This is used // for checking if a lock is owned by the running thread. - // Warning: the method can only be used on the running thread - // Fast lock support uses these methods - virtual bool lock_is_in_stack(address adr) const; + + // Used by fast lock support virtual bool is_lock_owned(address adr) const; // Check if address is in the stack of the thread (not just for locks). + // Warning: the method can only be used on the running thread bool is_in_stack(address adr) const; // Sets this thread as starting thread. Returns failure if thread diff --git a/src/share/vm/runtime/vmStructs.cpp b/src/share/vm/runtime/vmStructs.cpp index 2de7bbdf5..23d1db279 100644 --- a/src/share/vm/runtime/vmStructs.cpp +++ b/src/share/vm/runtime/vmStructs.cpp @@ -656,7 +656,6 @@ static inline uint64_t cast_uint64_t(size_t x) \ volatile_nonstatic_field(Thread, _suspend_flags, uint32_t) \ nonstatic_field(Thread, _active_handles, JNIHandleBlock*) \ - nonstatic_field(Thread, _highest_lock, address) \ nonstatic_field(Thread, _tlab, ThreadLocalAllocBuffer) \ nonstatic_field(Thread, _current_pending_monitor, ObjectMonitor*) \ nonstatic_field(Thread, _current_pending_monitor_is_from_java, bool) \ -- GitLab