Fix and enable ART futexes.
Change-Id: I5c49e370de0dcf438352f0c657e77d8ba53f4d27
diff --git a/src/mutex.cc b/src/mutex.cc
index 1cdbc4d..905d462 100644
--- a/src/mutex.cc
+++ b/src/mutex.cc
@@ -491,7 +491,7 @@
#if ART_USE_FUTEXES
bool done = false;
timespec end_abs_ts;
- InitTimeSpec(self, true, CLOCK_REALTIME, ms, ns, &end_abs_ts);
+ InitTimeSpec(true, CLOCK_REALTIME, ms, ns, &end_abs_ts);
do {
int32_t cur_state = state_;
if (cur_state == 0) {
@@ -500,7 +500,7 @@
} else {
// Failed to acquire, hang up.
timespec now_abs_ts;
- InitTimeSpec(self, true, CLOCK_REALTIME, 0, 0, &now_abs_ts);
+ InitTimeSpec(true, CLOCK_REALTIME, 0, 0, &now_abs_ts);
timespec rel_ts;
if (ComputeRelativeTimeSpec(&rel_ts, end_abs_ts, now_abs_ts)) {
return false; // Timed out.
@@ -688,7 +688,14 @@
}
ConditionVariable::~ConditionVariable() {
-#if !ART_USE_FUTEXES
+#if ART_USE_FUTEXES
+ if (num_waiters_!= 0) {
+ MutexLock mu(Thread::Current(), *Locks::runtime_shutdown_lock_);
+ Runtime* runtime = Runtime::Current();
+ bool shutting_down = (runtime == NULL) || runtime->IsShuttingDown();
+ LOG(shutting_down ? WARNING : FATAL) << "~ConditionVariable failed for " << name_;
+ }
+#else
// We can't use CHECK_MUTEX_CALL here because on shutdown a suspended daemon thread
// may still be using condition variables.
int rc = pthread_cond_destroy(&cond_);
@@ -708,25 +715,29 @@
// guard_.AssertExclusiveHeld(self);
DCHECK_EQ(guard_.GetExclusiveOwnerTid(), SafeGetTid(self));
#if ART_USE_FUTEXES
- if (num_waiters_ > 0) {
- android_atomic_inc(&state_); // Indicate a wake has occurred to waiters coming in.
+ // Compute number of waiters to be requeued and add to mutex contenders.
+ int32_t num_requeued = num_waiters_ - num_awoken_;
+ if (num_requeued != 0) {
bool done = false;
+ android_atomic_inc(&state_); // Indicate the broadcast occurred.
do {
- int32_t cur_state = state_;
- // Compute number of waiters requeued and add to mutex contenders.
- int32_t num_requeued = num_waiters_ - num_awoken_;
- android_atomic_add(num_requeued, &guard_.num_contenders_);
- // Requeue waiters onto contenders.
- done = futex(&state_, FUTEX_CMP_REQUEUE, 0,
- reinterpret_cast<const timespec*>(std::numeric_limits<int32_t>::max()),
- &guard_.state_, cur_state) != -1;
- if (!done) {
- if (errno != EAGAIN) {
- PLOG(FATAL) << "futex cmp requeue failed for " << name_;
- }
- } else {
- num_awoken_ = num_waiters_;
- }
+ int32_t cur_state = state_;
+ // Requeue waiters onto contenders.
+ done = futex(&state_, FUTEX_CMP_REQUEUE, 0,
+ reinterpret_cast<const timespec*>(std::numeric_limits<int32_t>::max()),
+ &guard_.state_, cur_state) != -1;
+ if (!done) {
+ if (errno != EAGAIN) {
+ PLOG(FATAL) << "futex cmp requeue failed for " << name_;
+ }
+ } else {
+ // Successful requeue, add the requeued waiters to the contenders of the guard_ to ensure
+ // that unlocks of guard_ will wake the waiters. Reflect that we've requeued the waiters
+ // in the awoken count.
+ DCHECK_EQ(num_awoken_ + num_requeued, num_waiters_);
+ android_atomic_add(num_requeued, &guard_.num_contenders_);
+ num_awoken_ = num_waiters_;
+ }
} while (!done);
}
#else
@@ -738,17 +749,17 @@
DCHECK(self == NULL || self == Thread::Current());
guard_.AssertExclusiveHeld(self);
#if ART_USE_FUTEXES
- if (num_waiters_ > 0) {
- android_atomic_inc(&state_); // Indicate a wake has occurred to waiters coming in.
+ if (num_waiters_ > num_awoken_) {
+ android_atomic_inc(&state_); // Indicate a signal occurred.
// Futex wake 1 waiter who will then come and in contend on mutex. It'd be nice to requeue them
// to avoid this, however, requeueing can only move all waiters.
- if (futex(&state_, FUTEX_WAKE, 1, NULL, NULL, 0) == 1) {
- // Wake success.
- // We weren't requeued, however, to make accounting simpler in the Wait code, increment the
- // number of contenders on the mutex.
- num_awoken_++;
- android_atomic_inc(&guard_.num_contenders_);
- }
+ int num_woken = futex(&state_, FUTEX_WAKE, 1, NULL, NULL, 0);
+ // Check something was woken or else we changed state_ before they had chance to wait.
+ CHECK((num_woken == 0) || (num_woken == 1));
+ // We weren't requeued, however, to make accounting simpler in the Wait code, increment the
+ // number of contenders on the mutex.
+ num_awoken_++;
+ android_atomic_inc(&guard_.num_contenders_);
}
#else
CHECK_MUTEX_CALL(pthread_cond_signal, (&cond_));
@@ -764,12 +775,10 @@
num_waiters_++;
guard_.recursion_count_ = 1;
guard_.ExclusiveUnlock(self);
- bool woken = true;
while (futex(&state_, FUTEX_WAIT, cur_state, NULL, NULL, 0) != 0) {
- if (errno == EINTR || errno == EAGAIN) {
+ if ((errno == EINTR) || (errno == EAGAIN)) {
if (state_ != cur_state) {
- // We failed and a signal has come in.
- woken = false;
+ // We failed and a ConditionVariable::Signal has come in.
break;
}
} else {
@@ -777,9 +786,13 @@
}
}
guard_.ExclusiveLock(self);
+ CHECK_NE(num_waiters_, 0);
num_waiters_--;
- if (woken) {
- // If we were woken we were requeued on the mutex, decrement the mutex's contender count.
+ if (num_awoken_ > 0) {
+ // Note: there is a subtle race where a single signal may appear to wake multiple threads due
+ // to state_ changing. We detect this race using num_awoken_ and the mutual exclusion
+ // guaranteed by guard_.
+ CHECK_NE(guard_.num_contenders_, 0);
android_atomic_dec(&guard_.num_contenders_);
num_awoken_--;
}
@@ -798,31 +811,25 @@
// Record the original end time so that if the futex call fails we can recompute the appropriate
// relative time.
timespec end_abs_ts;
- InitTimeSpec(self, true, CLOCK_REALTIME, ms, ns, &end_abs_ts);
+ InitTimeSpec(true, CLOCK_REALTIME, ms, ns, &end_abs_ts);
timespec rel_ts;
- InitTimeSpec(self, false, CLOCK_REALTIME, ms, ns, &rel_ts);
+ InitTimeSpec(false, CLOCK_REALTIME, ms, ns, &rel_ts);
// Read state so that we can know if a signal comes in during before we sleep.
int32_t cur_state = state_;
num_waiters_++;
guard_.recursion_count_ = 1;
guard_.ExclusiveUnlock(self);
- bool woken = true; // Did the futex wait end because we were awoken?
while (futex(&state_, FUTEX_WAIT, cur_state, &rel_ts, NULL, 0) != 0) {
if (errno == ETIMEDOUT) {
- woken = false;
- break;
- }
- if ((errno == EINTR) || (errno == EAGAIN)) {
+ break; // Timed out we're done.
+ } else if ((errno == EINTR) || (errno == EAGAIN)) {
if (state_ != cur_state) {
- // We failed and a signal has come in.
- woken = false;
- break;
+ break; // We failed and a ConditionVariable::Signal has come in.
}
timespec now_abs_ts;
- InitTimeSpec(self, true, CLOCK_REALTIME, 0, 0, &now_abs_ts);
+ InitTimeSpec(true, CLOCK_REALTIME, 0, 0, &now_abs_ts);
if (ComputeRelativeTimeSpec(&rel_ts, end_abs_ts, now_abs_ts)) {
// futex failed and we timed out in the meantime.
- woken = false;
break;
}
} else {
@@ -831,8 +838,15 @@
}
guard_.ExclusiveLock(self);
num_waiters_--;
- if (woken) {
- // If we were woken we were requeued on the mutex, decrement the mutex's contender count.
+ if ((cur_state != state_) && (num_awoken_ > 0)) {
+ // TODO: We were woken and didn't timeout (ie state_ changed - ignoring overflow). This check
+ // is racy given the overflow, however, getting a good causal picture from errno is complex
+ // and racy. From practice it hasn't yet been found to work, so we live with the highly
+ // unlikely race.
+ // Note: there is a subtle race where a single signal may appear to wake multiple threads due
+ // to state_ changing. We detect this race using num_awoken_ and the mutual exclusion
+ // guaranteed by guard_.
+ CHECK_NE(guard_.num_contenders_, 0);
android_atomic_dec(&guard_.num_contenders_);
num_awoken_--;
}