Revert^2 "Notify waiters when releasing the monitor"
This reverts commit 9cec9658ec0b7a6c715a154ec834faba853188e3.
Reason for revert: Changed lock ordering to not require reacquiring the
monitor lock while holding the wait lock.
Tested: 1000 iterations of ThreadStress
Bug: 117842465
Change-Id: I7b54943052c5eba367eac86da9646bfc81bc1163
diff --git a/runtime/monitor.cc b/runtime/monitor.cc
index 0f0a378..df2a8e2 100644
--- a/runtime/monitor.cc
+++ b/runtime/monitor.cc
@@ -97,6 +97,7 @@
lock_count_(0),
obj_(GcRoot<mirror::Object>(obj)),
wait_set_(nullptr),
+ wake_set_(nullptr),
hash_code_(hash_code),
locking_method_(nullptr),
locking_dex_pc_(0),
@@ -120,6 +121,7 @@
lock_count_(0),
obj_(GcRoot<mirror::Object>(obj)),
wait_set_(nullptr),
+ wake_set_(nullptr),
hash_code_(hash_code),
locking_method_(nullptr),
locking_dex_pc_(0),
@@ -226,7 +228,8 @@
}
void Monitor::AppendToWaitSet(Thread* thread) {
- DCHECK(owner_ == Thread::Current());
+ // Not checking that the owner is equal to this thread, since we've released
+ // the monitor by the time this method is called.
DCHECK(thread != nullptr);
DCHECK(thread->GetWaitNext() == nullptr) << thread->GetWaitNext();
if (wait_set_ == nullptr) {
@@ -245,24 +248,29 @@
void Monitor::RemoveFromWaitSet(Thread *thread) {
DCHECK(owner_ == Thread::Current());
DCHECK(thread != nullptr);
- if (wait_set_ == nullptr) {
- return;
- }
- if (wait_set_ == thread) {
- wait_set_ = thread->GetWaitNext();
- thread->SetWaitNext(nullptr);
- return;
- }
-
- Thread* t = wait_set_;
- while (t->GetWaitNext() != nullptr) {
- if (t->GetWaitNext() == thread) {
- t->SetWaitNext(thread->GetWaitNext());
- thread->SetWaitNext(nullptr);
- return;
+ auto remove = [&](Thread*& set){
+ if (set != nullptr) {
+ if (set == thread) {
+ set = thread->GetWaitNext();
+ thread->SetWaitNext(nullptr);
+ return true;
+ }
+ Thread* t = set;
+ while (t->GetWaitNext() != nullptr) {
+ if (t->GetWaitNext() == thread) {
+ t->SetWaitNext(thread->GetWaitNext());
+ thread->SetWaitNext(nullptr);
+ return true;
+ }
+ t = t->GetWaitNext();
+ }
}
- t = t->GetWaitNext();
+ return false;
+ };
+ if (remove(wait_set_)) {
+ return;
}
+ remove(wake_set_);
}
void Monitor::SetObject(mirror::Object* object) {
@@ -699,33 +707,81 @@
bool Monitor::Unlock(Thread* self) {
DCHECK(self != nullptr);
uint32_t owner_thread_id = 0u;
- {
- MutexLock mu(self, monitor_lock_);
- Thread* owner = owner_;
- if (owner != nullptr) {
- owner_thread_id = owner->GetThreadId();
- }
- if (owner == self) {
- // We own the monitor, so nobody else can be in here.
- AtraceMonitorUnlock();
- if (lock_count_ == 0) {
- owner_ = nullptr;
- locking_method_ = nullptr;
- locking_dex_pc_ = 0;
- // Wake a contender.
- monitor_contenders_.Signal(self);
- } else {
- --lock_count_;
- }
+ DCHECK(!monitor_lock_.IsExclusiveHeld(self));
+ monitor_lock_.Lock(self);
+ Thread* owner = owner_;
+ if (owner != nullptr) {
+ owner_thread_id = owner->GetThreadId();
+ }
+ if (owner == self) {
+ // We own the monitor, so nobody else can be in here.
+ AtraceMonitorUnlock();
+ if (lock_count_ == 0) {
+ owner_ = nullptr;
+ locking_method_ = nullptr;
+ locking_dex_pc_ = 0;
+ SignalContendersAndReleaseMonitorLock(self);
+ return true;
+ } else {
+ --lock_count_;
+ monitor_lock_.Unlock(self);
return true;
}
}
// We don't own this, so we're not allowed to unlock it.
// The JNI spec says that we should throw IllegalMonitorStateException in this case.
FailedUnlock(GetObject(), self->GetThreadId(), owner_thread_id, this);
+ monitor_lock_.Unlock(self);
return false;
}
+void Monitor::SignalContendersAndReleaseMonitorLock(Thread* self) {
+ // We want to signal one thread to wake up, to acquire the monitor that
+ // we are releasing. This could either be a Thread waiting on its own
+ // ConditionVariable, or a thread waiting on monitor_contenders_.
+ while (wake_set_ != nullptr) {
+ // No risk of waking ourselves here; since monitor_lock_ is not released until we're ready to
+ // return, notify can't move the current thread from wait_set_ to wake_set_ until this
+ // method is done checking wake_set_.
+ Thread* thread = wake_set_;
+ wake_set_ = thread->GetWaitNext();
+ thread->SetWaitNext(nullptr);
+
+ // Check to see if the thread is still waiting.
+ {
+ // In the case of wait(), we'll be acquiring another thread's GetWaitMutex with
+ // self's GetWaitMutex held. This does not risk deadlock, because we only acquire this lock
+ // for threads in the wake_set_. A thread can only enter wake_set_ from Notify or NotifyAll,
+ // and those hold monitor_lock_. Thus, the threads whose wait mutexes we acquire here must
+ // have already been released from wait(), since we have not released monitor_lock_ until
+ // after we've chosen our thread to wake, so there is no risk of the following lock ordering
+ // leading to deadlock:
+ // Thread 1 waits
+ // Thread 2 waits
+ // Thread 3 moves threads 1 and 2 from wait_set_ to wake_set_
+ // Thread 1 enters this block, and attempts to acquire Thread 2's GetWaitMutex to wake it
+ // Thread 2 enters this block, and attempts to acquire Thread 1's GetWaitMutex to wake it
+ //
+ // Since monitor_lock_ is not released until the thread-to-be-woken-up's GetWaitMutex is
+ // acquired, two threads cannot attempt to acquire each other's GetWaitMutex while holding
+ // their own and cause deadlock.
+ MutexLock wait_mu(self, *thread->GetWaitMutex());
+ if (thread->GetWaitMonitor() != nullptr) {
+ // Release the lock, so that a potentially awakened thread will not
+ // immediately contend on it. The lock ordering here is:
+ // monitor_lock_, self->GetWaitMutex, thread->GetWaitMutex
+ monitor_lock_.Unlock(self);
+ thread->GetWaitConditionVariable()->Signal(self);
+ return;
+ }
+ }
+ }
+ // If we didn't wake any threads that were originally waiting on us,
+ // wake a contender.
+ monitor_contenders_.Signal(self);
+ monitor_lock_.Unlock(self);
+}
+
void Monitor::Wait(Thread* self, int64_t ms, int32_t ns,
bool interruptShouldThrow, ThreadState why) {
DCHECK(self != nullptr);
@@ -755,17 +811,9 @@
}
/*
- * Add ourselves to the set of threads waiting on this monitor, and
- * release our hold. We need to let it go even if we're a few levels
+ * Release our hold - we need to let it go even if we're a few levels
* deep in a recursive lock, and we need to restore that later.
- *
- * We append to the wait set ahead of clearing the count and owner
- * fields so the subroutine can check that the calling thread owns
- * the monitor. Aside from that, the order of member updates is
- * not order sensitive as we hold the pthread mutex.
*/
- AppendToWaitSet(self);
- ++num_waiters_;
int prev_lock_count = lock_count_;
lock_count_ = 0;
owner_ = nullptr;
@@ -790,6 +838,17 @@
// Pseudo-atomically wait on self's wait_cond_ and release the monitor lock.
MutexLock mu(self, *self->GetWaitMutex());
+ /*
+ * Add ourselves to the set of threads waiting on this monitor.
+ * It's important that we are only added to the wait set after
+ * acquiring our GetWaitMutex, so that calls to Notify() that occur after we
+ * have released monitor_lock_ will not move us from wait_set_ to wake_set_
+ * until we've signalled contenders on this monitor.
+ */
+ AppendToWaitSet(self);
+ ++num_waiters_;
+
+
// Set wait_monitor_ to the monitor object we will be waiting on. When wait_monitor_ is
// non-null a notifying or interrupting thread must signal the thread's wait_cond_ to wake it
// up.
@@ -797,8 +856,7 @@
self->SetWaitMonitor(this);
// Release the monitor lock.
- monitor_contenders_.Signal(self);
- monitor_lock_.Unlock(self);
+ SignalContendersAndReleaseMonitorLock(self);
// Handle the case where the thread was interrupted before we called wait().
if (self->IsInterrupted()) {
@@ -874,18 +932,12 @@
ThrowIllegalMonitorStateExceptionF("object not locked by thread before notify()");
return;
}
- // Signal the first waiting thread in the wait set.
- while (wait_set_ != nullptr) {
- Thread* thread = wait_set_;
- wait_set_ = thread->GetWaitNext();
- thread->SetWaitNext(nullptr);
-
- // Check to see if the thread is still waiting.
- MutexLock wait_mu(self, *thread->GetWaitMutex());
- if (thread->GetWaitMonitor() != nullptr) {
- thread->GetWaitConditionVariable()->Signal(self);
- return;
- }
+ // Move one thread from waiters to wake set
+ Thread* to_move = wait_set_;
+ if (to_move != nullptr) {
+ wait_set_ = to_move->GetWaitNext();
+ to_move->SetWaitNext(wake_set_);
+ wake_set_ = to_move;
}
}
@@ -897,12 +949,20 @@
ThrowIllegalMonitorStateExceptionF("object not locked by thread before notifyAll()");
return;
}
- // Signal all threads in the wait set.
- while (wait_set_ != nullptr) {
- Thread* thread = wait_set_;
- wait_set_ = thread->GetWaitNext();
- thread->SetWaitNext(nullptr);
- thread->Notify();
+
+ // Move all threads from waiters to wake set
+ Thread* to_move = wait_set_;
+ if (to_move != nullptr) {
+ wait_set_ = nullptr;
+ Thread* move_to = wake_set_;
+ if (move_to == nullptr) {
+ wake_set_ = to_move;
+ return;
+ }
+ while (move_to->GetWaitNext() != nullptr) {
+ move_to = move_to->GetWaitNext();
+ }
+ move_to->SetWaitNext(to_move);
}
}