From d9e1bb76fe1e01fb79bb65959b92051aa18fddbe Mon Sep 17 00:00:00 2001 From: Glenn Kasten Date: Thu, 2 Jun 2011 08:59:28 -0700 Subject: [PATCH] Remove redundant memory barrier pthread_create already includes the necessary memory barriers: - parent at pthread_create : pthread_mutex_unlock(start_mutex) - child at __thread_entry : pthread_mutex_lock(start_mutex) Add lock around uses of mThread. Added comments: - uses of mThread require lock - androidCreateRawThreadEtc returned ID is not safe for direct use from non-parent threads. Change-Id: I18cb296b41ddaf64cf127b57aab31154319b5970 --- include/utils/threads.h | 1 + libs/utils/Threads.cpp | 13 +++++-------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/include/utils/threads.h b/include/utils/threads.h index 41e5766a0..0bd69cf37 100644 --- a/include/utils/threads.h +++ b/include/utils/threads.h @@ -526,6 +526,7 @@ private: Thread& operator=(const Thread&); static int _threadLoop(void* user); const bool mCanCallJava; + // always hold mLock when reading or writing thread_id_t mThread; mutable Mutex mLock; Condition mThreadExitedCondition; diff --git a/libs/utils/Threads.cpp b/libs/utils/Threads.cpp index 8b5da0e58..c74822866 100644 --- a/libs/utils/Threads.cpp +++ b/libs/utils/Threads.cpp @@ -168,6 +168,9 @@ int androidCreateRawThreadEtc(android_thread_func_t entryFunction, return 0; } + // Note that *threadID is directly available to the parent only, as it is + // assigned after the child starts. Use memory barrier / lock if the child + // or other threads also need access. if (threadId != NULL) { *threadId = (android_thread_id_t)thread; // XXX: this is not portable } @@ -718,7 +721,6 @@ status_t Thread::run(const char* name, int32_t priority, size_t stack) res = androidCreateRawThreadEtc(_threadLoop, this, name, priority, stack, &mThread); } - // The new thread wakes up at _threadLoop, but immediately blocks on mLock if (res == false) { mStatus = UNKNOWN_ERROR; // something happened! @@ -742,11 +744,6 @@ int Thread::_threadLoop(void* user) { Thread* const self = static_cast(user); - // force a memory barrier before reading any fields, in particular mHoldSelf - { - Mutex::Autolock _l(self->mLock); - } - sp strong(self->mHoldSelf); wp weak(strong); self->mHoldSelf.clear(); @@ -816,6 +813,7 @@ void Thread::requestExit() status_t Thread::requestExitAndWait() { + Mutex::Autolock _l(mLock); if (mThread == getThreadId()) { LOGW( "Thread (this=%p): don't call waitForExit() from this " @@ -825,9 +823,8 @@ status_t Thread::requestExitAndWait() return WOULD_BLOCK; } - requestExit(); + mExitPending = true; - Mutex::Autolock _l(mLock); while (mRunning == true) { mThreadExitedCondition.wait(mLock); }