commit a538e268e653d8fa1ea27659f507bc0ce5ceead0
author Glenn Kasten <gkasten@google.com> Thu Jun 02 08:59:28 2011 -0700
committer Alex Ray <aray@google.com> Tue Jul 30 13:56:57 2013 -0700
tree 4426a4dd3204a61e166868bd789dac6783e43157
parent bf7a884f22299ad424e350c497248bf6cdcd6467

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

diff --git a/include/utils/threads.h b/include/utils/threads.h
index 41e5766..0bd69cf 100644
--- a/include/utils/threads.h
+++ b/include/utils/threads.h
@@ -526,6 +526,7 @@
     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;

libs/utils/Threads.cpp

diff --git a/libs/utils/Threads.cpp b/libs/utils/Threads.cpp
index 8b5da0e..c748228 100644
--- a/libs/utils/Threads.cpp
+++ b/libs/utils/Threads.cpp
@@ -168,6 +168,9 @@
         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 @@
         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 @@
 {
     Thread* const self = static_cast<Thread*>(user);
 
-    // force a memory barrier before reading any fields, in particular mHoldSelf
-    {
-    Mutex::Autolock _l(self->mLock);
-    }
-
     sp<Thread> strong(self->mHoldSelf);
     wp<Thread> weak(strong);
     self->mHoldSelf.clear();
@@ -816,6 +813,7 @@
 
 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 @@
         return WOULD_BLOCK;
     }
     
-    requestExit();
+    mExitPending = true;
 
-    Mutex::Autolock _l(mLock);
     while (mRunning == true) {
         mThreadExitedCondition.wait(mLock);
     }