Shrink ART Mutex exclusive_owner_ field to Atomic<pid_t>
The old volatile uint64_t version had a data race, and was thus
technically incorrect. Since it's unclear whether volatile uint64_t
updates are actually atomic on 32-bit platforms, even the informal
correctness argument here already effectively assumed that the upper
32 bits were zero. Don't store them. Explicitly complain if a pid_t
might be too big to support lock-free atomic operations.
Remove many explicit references to exclusive_owner to avoid
littering the code with LoadRelaxed calls.
The return convention for GetExclusiveOwnerTid() was unclear
for the shared ownership case. It was previously treated
inconsistently as 0 (pthread locks), (uint64_t)(-1U) and
(uint64_t)(-1). Make it as consistent as easily possible, and
document remaining weirdness.
Bug: 65171052
Test: AOSP builds. Host tests pass.
Change-Id: Ia99aca268952597a90b3c798b714cddbdc2c365e
diff --git a/runtime/base/mutex-inl.h b/runtime/base/mutex-inl.h
index 0ac2399..c9d48ff 100644
--- a/runtime/base/mutex-inl.h
+++ b/runtime/base/mutex-inl.h
@@ -44,11 +44,15 @@
}
#endif // ART_USE_FUTEXES
-static inline uint64_t SafeGetTid(const Thread* self) {
+// The following isn't strictly necessary, but we want updates on Atomic<pid_t> to be lock-free.
+// TODO: Use std::atomic::is_always_lock_free after switching to C++17 atomics.
+static_assert(sizeof(pid_t) <= sizeof(int32_t), "pid_t should fit in 32 bits");
+
+static inline pid_t SafeGetTid(const Thread* self) {
if (self != nullptr) {
- return static_cast<uint64_t>(self->GetTid());
+ return self->GetTid();
} else {
- return static_cast<uint64_t>(GetTid());
+ return GetTid();
}
}
@@ -142,14 +146,14 @@
#else
CHECK_MUTEX_CALL(pthread_rwlock_rdlock, (&rwlock_));
#endif
- DCHECK(exclusive_owner_ == 0U || exclusive_owner_ == -1U);
+ DCHECK(GetExclusiveOwnerTid() == 0 || GetExclusiveOwnerTid() == -1);
RegisterAsLocked(self);
AssertSharedHeld(self);
}
inline void ReaderWriterMutex::SharedUnlock(Thread* self) {
DCHECK(self == nullptr || self == Thread::Current());
- DCHECK(exclusive_owner_ == 0U || exclusive_owner_ == -1U);
+ DCHECK(GetExclusiveOwnerTid() == 0 || GetExclusiveOwnerTid() == -1);
AssertSharedHeld(self);
RegisterAsUnlocked(self);
#if ART_USE_FUTEXES
@@ -190,8 +194,8 @@
return result;
}
-inline uint64_t Mutex::GetExclusiveOwnerTid() const {
- return exclusive_owner_;
+inline pid_t Mutex::GetExclusiveOwnerTid() const {
+ return exclusive_owner_.LoadRelaxed();
}
inline void Mutex::AssertExclusiveHeld(const Thread* self) const {
@@ -216,7 +220,7 @@
return result;
}
-inline uint64_t ReaderWriterMutex::GetExclusiveOwnerTid() const {
+inline pid_t ReaderWriterMutex::GetExclusiveOwnerTid() const {
#if ART_USE_FUTEXES
int32_t state = state_.LoadRelaxed();
if (state == 0) {
@@ -224,10 +228,10 @@
} else if (state > 0) {
return -1; // Shared.
} else {
- return exclusive_owner_;
+ return exclusive_owner_.LoadRelaxed();
}
#else
- return exclusive_owner_;
+ return exclusive_owner_.LoadRelaxed();
#endif
}
diff --git a/runtime/base/mutex.cc b/runtime/base/mutex.cc
index 6392198..d1ea13a 100644
--- a/runtime/base/mutex.cc
+++ b/runtime/base/mutex.cc
@@ -361,14 +361,13 @@
Mutex::Mutex(const char* name, LockLevel level, bool recursive)
- : BaseMutex(name, level), recursive_(recursive), recursion_count_(0) {
+ : BaseMutex(name, level), exclusive_owner_(0), recursive_(recursive), recursion_count_(0) {
#if ART_USE_FUTEXES
DCHECK_EQ(0, state_.LoadRelaxed());
DCHECK_EQ(0, num_contenders_.LoadRelaxed());
#else
CHECK_MUTEX_CALL(pthread_mutex_init, (&mutex_, nullptr));
#endif
- exclusive_owner_ = 0;
}
// Helper to allow checking shutdown while locking for thread safety.
@@ -382,9 +381,9 @@
#if ART_USE_FUTEXES
if (state_.LoadRelaxed() != 0) {
LOG(safe_to_call_abort ? FATAL : WARNING)
- << "destroying mutex with owner: " << exclusive_owner_;
+ << "destroying mutex with owner: " << GetExclusiveOwnerTid();
} else {
- if (exclusive_owner_ != 0) {
+ if (GetExclusiveOwnerTid() != 0) {
LOG(safe_to_call_abort ? FATAL : WARNING)
<< "unexpectedly found an owner on unlocked mutex " << name_;
}
@@ -439,8 +438,8 @@
#else
CHECK_MUTEX_CALL(pthread_mutex_lock, (&mutex_));
#endif
- DCHECK_EQ(exclusive_owner_, 0U);
- exclusive_owner_ = SafeGetTid(self);
+ DCHECK_EQ(GetExclusiveOwnerTid(), 0);
+ exclusive_owner_.StoreRelaxed(SafeGetTid(self));
RegisterAsLocked(self);
}
recursion_count_++;
@@ -479,8 +478,8 @@
PLOG(FATAL) << "pthread_mutex_trylock failed for " << name_;
}
#endif
- DCHECK_EQ(exclusive_owner_, 0U);
- exclusive_owner_ = SafeGetTid(self);
+ DCHECK_EQ(GetExclusiveOwnerTid(), 0);
+ exclusive_owner_.StoreRelaxed(SafeGetTid(self));
RegisterAsLocked(self);
}
recursion_count_++;
@@ -506,7 +505,7 @@
<< " Thread::Current()=" << name2;
}
AssertHeld(self);
- DCHECK_NE(exclusive_owner_, 0U);
+ DCHECK_NE(GetExclusiveOwnerTid(), 0);
recursion_count_--;
if (!recursive_ || recursion_count_ == 0) {
if (kDebugLocking) {
@@ -520,9 +519,9 @@
int32_t cur_state = state_.LoadRelaxed();
if (LIKELY(cur_state == 1)) {
// We're no longer the owner.
- exclusive_owner_ = 0;
+ exclusive_owner_.StoreRelaxed(0);
// Change state to 0 and impose load/store ordering appropriate for lock release.
- // Note, the relaxed loads below musn't reorder before the CompareExchange.
+ // Note, the relaxed loads below mustn't reorder before the CompareExchange.
// TODO: the ordering here is non-trivial as state is split across 3 fields, fix by placing
// a status bit into the state on contention.
done = state_.CompareExchangeWeakSequentiallyConsistent(cur_state, 0 /* new state */);
@@ -547,7 +546,7 @@
}
} while (!done);
#else
- exclusive_owner_ = 0;
+ exclusive_owner_.StoreRelaxed(0);
CHECK_MUTEX_CALL(pthread_mutex_unlock, (&mutex_));
#endif
}
@@ -588,13 +587,13 @@
#if !ART_USE_FUTEXES
CHECK_MUTEX_CALL(pthread_rwlock_init, (&rwlock_, nullptr));
#endif
- exclusive_owner_ = 0;
+ exclusive_owner_.StoreRelaxed(0);
}
ReaderWriterMutex::~ReaderWriterMutex() {
#if ART_USE_FUTEXES
CHECK_EQ(state_.LoadRelaxed(), 0);
- CHECK_EQ(exclusive_owner_, 0U);
+ CHECK_EQ(GetExclusiveOwnerTid(), 0);
CHECK_EQ(num_pending_readers_.LoadRelaxed(), 0);
CHECK_EQ(num_pending_writers_.LoadRelaxed(), 0);
#else
@@ -640,8 +639,8 @@
#else
CHECK_MUTEX_CALL(pthread_rwlock_wrlock, (&rwlock_));
#endif
- DCHECK_EQ(exclusive_owner_, 0U);
- exclusive_owner_ = SafeGetTid(self);
+ DCHECK_EQ(GetExclusiveOwnerTid(), 0);
+ exclusive_owner_.StoreRelaxed(SafeGetTid(self));
RegisterAsLocked(self);
AssertExclusiveHeld(self);
}
@@ -650,14 +649,14 @@
DCHECK(self == nullptr || self == Thread::Current());
AssertExclusiveHeld(self);
RegisterAsUnlocked(self);
- DCHECK_NE(exclusive_owner_, 0U);
+ DCHECK_NE(GetExclusiveOwnerTid(), 0);
#if ART_USE_FUTEXES
bool done = false;
do {
int32_t cur_state = state_.LoadRelaxed();
if (LIKELY(cur_state == -1)) {
// We're no longer the owner.
- exclusive_owner_ = 0;
+ exclusive_owner_.StoreRelaxed(0);
// Change state from -1 to 0 and impose load/store ordering appropriate for lock release.
// Note, the relaxed loads below musn't reorder before the CompareExchange.
// TODO: the ordering here is non-trivial as state is split across 3 fields, fix by placing
@@ -675,7 +674,7 @@
}
} while (!done);
#else
- exclusive_owner_ = 0;
+ exclusive_owner_.StoreRelaxed(0);
CHECK_MUTEX_CALL(pthread_rwlock_unlock, (&rwlock_));
#endif
}
@@ -731,7 +730,7 @@
PLOG(FATAL) << "pthread_rwlock_timedwrlock failed for " << name_;
}
#endif
- exclusive_owner_ = SafeGetTid(self);
+ exclusive_owner_.StoreRelaxed(SafeGetTid(self));
RegisterAsLocked(self);
AssertSharedHeld(self);
return true;
@@ -955,11 +954,11 @@
CHECK_GE(guard_.num_contenders_.LoadRelaxed(), 0);
guard_.num_contenders_--;
#else
- uint64_t old_owner = guard_.exclusive_owner_;
- guard_.exclusive_owner_ = 0;
+ pid_t old_owner = guard_.GetExclusiveOwnerTid();
+ guard_.exclusive_owner.StoreRelaxed(0);
guard_.recursion_count_ = 0;
CHECK_MUTEX_CALL(pthread_cond_wait, (&cond_, &guard_.mutex_));
- guard_.exclusive_owner_ = old_owner;
+ guard_.exclusive_owner_.StoreRelaxed(old_owner);
#endif
guard_.recursion_count_ = old_recursion_count;
}
@@ -1001,8 +1000,8 @@
#else
int clock = CLOCK_REALTIME;
#endif
- uint64_t old_owner = guard_.exclusive_owner_;
- guard_.exclusive_owner_ = 0;
+ pid_t old_owner = guard_.GetExclusiveOwnerTid();
+ guard_.exclusive_owner_.StoreRelaxed(0);
guard_.recursion_count_ = 0;
timespec ts;
InitTimeSpec(true, clock, ms, ns, &ts);
@@ -1013,7 +1012,7 @@
errno = rc;
PLOG(FATAL) << "TimedWait failed for " << name_;
}
- guard_.exclusive_owner_ = old_owner;
+ guard_.exclusive_owner_.StoreRelaxed(old_owner);
#endif
guard_.recursion_count_ = old_recursion_count;
return timed_out;
diff --git a/runtime/base/mutex.h b/runtime/base/mutex.h
index 7a472e7..189c0d0 100644
--- a/runtime/base/mutex.h
+++ b/runtime/base/mutex.h
@@ -19,6 +19,7 @@
#include <pthread.h>
#include <stdint.h>
+#include <unistd.h> // for pid_t
#include <iosfwd>
#include <string>
@@ -263,7 +264,7 @@
// Id associated with exclusive owner. No memory ordering semantics if called from a thread other
// than the owner.
- uint64_t GetExclusiveOwnerTid() const;
+ pid_t GetExclusiveOwnerTid() const;
// Returns how many times this Mutex has been locked, it is better to use AssertHeld/NotHeld.
unsigned int GetDepth() const {
@@ -282,12 +283,12 @@
// 0 is unheld, 1 is held.
AtomicInteger state_;
// Exclusive owner.
- volatile uint64_t exclusive_owner_;
+ Atomic<pid_t> exclusive_owner_;
// Number of waiting contenders.
AtomicInteger num_contenders_;
#else
pthread_mutex_t mutex_;
- volatile uint64_t exclusive_owner_; // Guarded by mutex_.
+ Atomic<pid_t> exclusive_owner_; // Guarded by mutex_. Asynchronous reads are OK.
#endif
const bool recursive_; // Can the lock be recursively held?
unsigned int recursion_count_;
@@ -385,8 +386,9 @@
}
// Id associated with exclusive owner. No memory ordering semantics if called from a thread other
- // than the owner.
- uint64_t GetExclusiveOwnerTid() const;
+ // than the owner. Returns 0 if the lock is not held. Returns either 0 or -1 if it is held by
+ // one or more readers.
+ pid_t GetExclusiveOwnerTid() const;
virtual void Dump(std::ostream& os) const;
@@ -403,14 +405,14 @@
// -1 implies held exclusive, +ve shared held by state_ many owners.
AtomicInteger state_;
// Exclusive owner. Modification guarded by this mutex.
- volatile uint64_t exclusive_owner_;
+ Atomic<pid_t> exclusive_owner_;
// Number of contenders waiting for a reader share.
AtomicInteger num_pending_readers_;
// Number of contenders waiting to be the writer.
AtomicInteger num_pending_writers_;
#else
pthread_rwlock_t rwlock_;
- volatile uint64_t exclusive_owner_; // Guarded by rwlock_.
+ Atomic<pid_t> exclusive_owner_; // Writes guarded by rwlock_. Asynchronous reads are OK.
#endif
DISALLOW_COPY_AND_ASSIGN(ReaderWriterMutex);
};
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 57b3a75..8cceeaa 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -1736,7 +1736,7 @@
os << " \"" << mutex->GetName() << "\"";
if (mutex->IsReaderWriterMutex()) {
ReaderWriterMutex* rw_mutex = down_cast<ReaderWriterMutex*>(mutex);
- if (rw_mutex->GetExclusiveOwnerTid() == static_cast<uint64_t>(tid)) {
+ if (rw_mutex->GetExclusiveOwnerTid() == tid) {
os << "(exclusive held)";
} else {
os << "(shared held)";
@@ -2291,7 +2291,7 @@
return tlsPtr_.managed_stack.ShadowFramesContain(hs_entry);
}
-void Thread::HandleScopeVisitRoots(RootVisitor* visitor, uint32_t thread_id) {
+void Thread::HandleScopeVisitRoots(RootVisitor* visitor, pid_t thread_id) {
BufferedRootVisitor<kDefaultBufferedRootCount> buffered_visitor(
visitor, RootInfo(kRootNativeStack, thread_id));
for (BaseHandleScope* cur = tlsPtr_.top_handle_scope; cur; cur = cur->GetLink()) {
@@ -3457,7 +3457,7 @@
template <bool kPrecise>
void Thread::VisitRoots(RootVisitor* visitor) {
- const uint32_t thread_id = GetThreadId();
+ const pid_t thread_id = GetThreadId();
visitor->VisitRootIfNonNull(&tlsPtr_.opeer, RootInfo(kRootThreadObject, thread_id));
if (tlsPtr_.exception != nullptr && tlsPtr_.exception != GetDeoptimizationException()) {
visitor->VisitRoot(reinterpret_cast<mirror::Object**>(&tlsPtr_.exception),
diff --git a/runtime/thread.h b/runtime/thread.h
index ad4506e..59c4fa9 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -838,7 +838,7 @@
// Is the given obj in this thread's stack indirect reference table?
bool HandleScopeContains(jobject obj) const;
- void HandleScopeVisitRoots(RootVisitor* visitor, uint32_t thread_id)
+ void HandleScopeVisitRoots(RootVisitor* visitor, pid_t thread_id)
REQUIRES_SHARED(Locks::mutator_lock_);
BaseHandleScope* GetTopHandleScope() {