Let g_thread_list_lock only protect g_thread_list.
As glibc/netbsd don't protect access to thread struct members by a global
lock, we don't want to do it either. This change reduces the
responsibility of g_thread_list_lock to only protect g_thread_list.
Bug: 19636317
Change-Id: I897890710653dac165d8fa4452c7ecf74abdbf2b
diff --git a/libc/bionic/libc_init_common.cpp b/libc/bionic/libc_init_common.cpp
index 52ca0f2..36dc085 100644
--- a/libc/bionic/libc_init_common.cpp
+++ b/libc/bionic/libc_init_common.cpp
@@ -92,7 +92,7 @@
main_thread.attr.stack_size = 0; // User code should never see this; we'll compute it when asked.
// TODO: the main thread's sched_policy and sched_priority need to be queried.
- __init_thread(&main_thread, false);
+ __init_thread(&main_thread);
__init_tls(&main_thread);
__set_tls(main_thread.tls);
main_thread.tls[TLS_SLOT_BIONIC_PREINIT] = &args;
@@ -113,7 +113,7 @@
// Get the main thread from TLS and add it to the thread list.
pthread_internal_t* main_thread = __get_thread();
- _pthread_internal_add(main_thread);
+ __pthread_internal_add(main_thread);
__system_properties_init(); // Requires 'environ'.
diff --git a/libc/bionic/pthread_accessor.h b/libc/bionic/pthread_accessor.h
deleted file mode 100644
index df4a5a2..0000000
--- a/libc/bionic/pthread_accessor.h
+++ /dev/null
@@ -1,64 +0,0 @@
-/*
- * Copyright (C) 2013 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef PTHREAD_ACCESSOR_H
-#define PTHREAD_ACCESSOR_H
-
-#include <pthread.h>
-
-#include "private/bionic_macros.h"
-#include "pthread_internal.h"
-
-class pthread_accessor {
- public:
- explicit pthread_accessor(pthread_t desired_thread) {
- Lock();
- for (thread_ = g_thread_list; thread_ != NULL; thread_ = thread_->next) {
- if (thread_ == reinterpret_cast<pthread_internal_t*>(desired_thread)) {
- break;
- }
- }
- }
-
- ~pthread_accessor() {
- Unlock();
- }
-
- void Unlock() {
- if (is_locked_) {
- is_locked_ = false;
- thread_ = NULL;
- pthread_mutex_unlock(&g_thread_list_lock);
- }
- }
-
- pthread_internal_t& operator*() const { return *thread_; }
- pthread_internal_t* operator->() const { return thread_; }
- pthread_internal_t* get() const { return thread_; }
-
- private:
- pthread_internal_t* thread_;
- bool is_locked_;
-
- void Lock() {
- pthread_mutex_lock(&g_thread_list_lock);
- is_locked_ = true;
- }
-
- DISALLOW_COPY_AND_ASSIGN(pthread_accessor);
-};
-
-#endif // PTHREAD_ACCESSOR_H
diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp
index a4bd054..ef3ce05 100644
--- a/libc/bionic/pthread_create.cpp
+++ b/libc/bionic/pthread_create.cpp
@@ -83,7 +83,7 @@
}
}
-int __init_thread(pthread_internal_t* thread, bool add_to_thread_list) {
+int __init_thread(pthread_internal_t* thread) {
int error = 0;
if (__predict_true((thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) == 0)) {
@@ -108,10 +108,6 @@
thread->cleanup_stack = NULL;
- if (add_to_thread_list) {
- _pthread_internal_add(thread);
- }
-
return error;
}
@@ -265,7 +261,7 @@
return clone_errno;
}
- int init_errno = __init_thread(thread, true);
+ int init_errno = __init_thread(thread);
if (init_errno != 0) {
// Mark the thread detached and replace its start_routine with a no-op.
// Letting the thread run is the easiest way to clean up its resources.
@@ -276,7 +272,7 @@
}
// Publish the pthread_t and unlock the mutex to let the new thread start running.
- *thread_out = reinterpret_cast<pthread_t>(thread);
+ *thread_out = __pthread_internal_add(thread);
pthread_mutex_unlock(&thread->startup_handshake_mutex);
return 0;
diff --git a/libc/bionic/pthread_detach.cpp b/libc/bionic/pthread_detach.cpp
index dcdb7b1..fb8e0dd 100644
--- a/libc/bionic/pthread_detach.cpp
+++ b/libc/bionic/pthread_detach.cpp
@@ -29,27 +29,24 @@
#include <errno.h>
#include <pthread.h>
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
int pthread_detach(pthread_t t) {
- {
- pthread_accessor thread(t);
- if (thread.get() == NULL) {
- return ESRCH;
- }
-
- ThreadJoinState old_state = THREAD_NOT_JOINED;
- while (old_state == THREAD_NOT_JOINED &&
- !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_DETACHED)) {
- }
- switch (old_state) {
- case THREAD_NOT_JOINED: return 0;
- case THREAD_JOINED: return EINVAL;
- case THREAD_DETACHED: return EINVAL;
- case THREAD_EXITED_NOT_JOINED: break; // Call pthread_join out of scope of pthread_accessor.
- }
+ pthread_internal_t* thread = __pthread_internal_find(t);
+ if (thread == NULL) {
+ return ESRCH;
}
- // The thread is in THREAD_EXITED_NOT_JOINED, use pthread_join to clean it up.
- return pthread_join(t, NULL);
+ ThreadJoinState old_state = THREAD_NOT_JOINED;
+ while (old_state == THREAD_NOT_JOINED &&
+ !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_DETACHED)) {
+ }
+
+ if (old_state == THREAD_NOT_JOINED) {
+ return 0;
+ } else if (old_state == THREAD_EXITED_NOT_JOINED) {
+ // Use pthread_join to clean it up.
+ return pthread_join(t, NULL);
+ }
+ return EINVAL;
}
diff --git a/libc/bionic/pthread_exit.cpp b/libc/bionic/pthread_exit.cpp
index 81cc67b..c2232a9 100644
--- a/libc/bionic/pthread_exit.cpp
+++ b/libc/bionic/pthread_exit.cpp
@@ -100,9 +100,7 @@
__set_tid_address(NULL);
// pthread_internal_t is freed below with stack, not here.
- pthread_mutex_lock(&g_thread_list_lock);
- _pthread_internal_remove_locked(thread, false);
- pthread_mutex_unlock(&g_thread_list_lock);
+ __pthread_internal_remove(thread);
if (thread->mmap_size != 0) {
// We need to free mapped space for detached threads when they exit.
diff --git a/libc/bionic/pthread_getcpuclockid.cpp b/libc/bionic/pthread_getcpuclockid.cpp
index d11f56a..2bf2004 100644
--- a/libc/bionic/pthread_getcpuclockid.cpp
+++ b/libc/bionic/pthread_getcpuclockid.cpp
@@ -28,11 +28,11 @@
#include <errno.h>
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
int pthread_getcpuclockid(pthread_t t, clockid_t* clockid) {
- pthread_accessor thread(t);
- if (thread.get() == NULL) {
+ pthread_internal_t* thread = __pthread_internal_find(t);
+ if (thread == NULL) {
return ESRCH;
}
diff --git a/libc/bionic/pthread_getschedparam.cpp b/libc/bionic/pthread_getschedparam.cpp
index 2cdc11a..052fb05 100644
--- a/libc/bionic/pthread_getschedparam.cpp
+++ b/libc/bionic/pthread_getschedparam.cpp
@@ -29,13 +29,13 @@
#include <errno.h>
#include "private/ErrnoRestorer.h"
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
int pthread_getschedparam(pthread_t t, int* policy, sched_param* param) {
ErrnoRestorer errno_restorer;
- pthread_accessor thread(t);
- if (thread.get() == NULL) {
+ pthread_internal_t* thread = __pthread_internal_find(t);
+ if (thread == NULL) {
return ESRCH;
}
diff --git a/libc/bionic/pthread_internals.cpp b/libc/bionic/pthread_internal.cpp
similarity index 70%
rename from libc/bionic/pthread_internals.cpp
rename to libc/bionic/pthread_internal.cpp
index 0dd88fe..1967ccf 100644
--- a/libc/bionic/pthread_internals.cpp
+++ b/libc/bionic/pthread_internal.cpp
@@ -38,26 +38,10 @@
#include "private/libc_logging.h"
#include "private/ScopedPthreadMutexLocker.h"
-pthread_internal_t* g_thread_list = NULL;
-pthread_mutex_t g_thread_list_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_internal_t* g_thread_list = NULL;
+static pthread_mutex_t g_thread_list_lock = PTHREAD_MUTEX_INITIALIZER;
-void _pthread_internal_remove_locked(pthread_internal_t* thread, bool free_thread) {
- if (thread->next != NULL) {
- thread->next->prev = thread->prev;
- }
- if (thread->prev != NULL) {
- thread->prev->next = thread->next;
- } else {
- g_thread_list = thread->next;
- }
-
- if (free_thread && thread->mmap_size != 0) {
- // Free mapped space, including thread stack and pthread_internal_t.
- munmap(thread->attr.stack_base, thread->mmap_size);
- }
-}
-
-void _pthread_internal_add(pthread_internal_t* thread) {
+pthread_t __pthread_internal_add(pthread_internal_t* thread) {
ScopedPthreadMutexLocker locker(&g_thread_list_lock);
// We insert at the head.
@@ -67,4 +51,42 @@
thread->next->prev = thread;
}
g_thread_list = thread;
+ return reinterpret_cast<pthread_t>(thread);
+}
+
+void __pthread_internal_remove(pthread_internal_t* thread) {
+ ScopedPthreadMutexLocker locker(&g_thread_list_lock);
+
+ if (thread->next != NULL) {
+ thread->next->prev = thread->prev;
+ }
+ if (thread->prev != NULL) {
+ thread->prev->next = thread->next;
+ } else {
+ g_thread_list = thread->next;
+ }
+}
+
+static void __pthread_internal_free(pthread_internal_t* thread) {
+ if (thread->mmap_size != 0) {
+ // Free mapped space, including thread stack and pthread_internal_t.
+ munmap(thread->attr.stack_base, thread->mmap_size);
+ }
+}
+
+void __pthread_internal_remove_and_free(pthread_internal_t* thread) {
+ __pthread_internal_remove(thread);
+ __pthread_internal_free(thread);
+}
+
+pthread_internal_t* __pthread_internal_find(pthread_t thread_id) {
+ pthread_internal_t* thread = reinterpret_cast<pthread_internal_t*>(thread_id);
+ ScopedPthreadMutexLocker locker(&g_thread_list_lock);
+
+ for (pthread_internal_t* t = g_thread_list; t != NULL; t = t->next) {
+ if (t == thread) {
+ return thread;
+ }
+ }
+ return NULL;
}
diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h
index 538e0da..99c455e 100644
--- a/libc/bionic/pthread_internal.h
+++ b/libc/bionic/pthread_internal.h
@@ -105,10 +105,14 @@
char dlerror_buffer[__BIONIC_DLERROR_BUFFER_SIZE];
} __attribute__((aligned(16))); // Align it as thread stack top below it should be aligned.
-__LIBC_HIDDEN__ int __init_thread(pthread_internal_t* thread, bool add_to_thread_list);
+__LIBC_HIDDEN__ int __init_thread(pthread_internal_t* thread);
__LIBC_HIDDEN__ void __init_tls(pthread_internal_t* thread);
__LIBC_HIDDEN__ void __init_alternate_signal_stack(pthread_internal_t*);
-__LIBC_HIDDEN__ void _pthread_internal_add(pthread_internal_t* thread);
+
+__LIBC_HIDDEN__ pthread_t __pthread_internal_add(pthread_internal_t* thread);
+__LIBC_HIDDEN__ pthread_internal_t* __pthread_internal_find(pthread_t pthread_id);
+__LIBC_HIDDEN__ void __pthread_internal_remove(pthread_internal_t* thread);
+__LIBC_HIDDEN__ void __pthread_internal_remove_and_free(pthread_internal_t* thread);
// Make __get_thread() inlined for performance reason. See http://b/19825434.
static inline __always_inline pthread_internal_t* __get_thread() {
@@ -116,7 +120,6 @@
}
__LIBC_HIDDEN__ void pthread_key_clean_all(void);
-__LIBC_HIDDEN__ void _pthread_internal_remove_locked(pthread_internal_t* thread, bool free_thread);
/*
* Traditionally we gave threads a 1MiB stack. When we started
@@ -127,9 +130,6 @@
*/
#define PTHREAD_STACK_SIZE_DEFAULT ((1 * 1024 * 1024) - SIGSTKSZ)
-__LIBC_HIDDEN__ extern pthread_internal_t* g_thread_list;
-__LIBC_HIDDEN__ extern pthread_mutex_t g_thread_list_lock;
-
/* Needed by fork. */
__LIBC_HIDDEN__ extern void __bionic_atfork_run_prepare();
__LIBC_HIDDEN__ extern void __bionic_atfork_run_child();
diff --git a/libc/bionic/pthread_join.cpp b/libc/bionic/pthread_join.cpp
index 15543b4..4d852cb 100644
--- a/libc/bionic/pthread_join.cpp
+++ b/libc/bionic/pthread_join.cpp
@@ -29,35 +29,31 @@
#include <errno.h>
#include "private/bionic_futex.h"
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
int pthread_join(pthread_t t, void** return_value) {
if (t == pthread_self()) {
return EDEADLK;
}
- pid_t tid;
- volatile int* tid_ptr;
- {
- pthread_accessor thread(t);
- if (thread.get() == NULL) {
- return ESRCH;
- }
-
- ThreadJoinState old_state = THREAD_NOT_JOINED;
- while ((old_state == THREAD_NOT_JOINED || old_state == THREAD_EXITED_NOT_JOINED) &&
- !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_JOINED)) {
- }
-
- if (old_state == THREAD_DETACHED || old_state == THREAD_JOINED) {
- return EINVAL;
- }
-
- tid = thread->tid;
- tid_ptr = &thread->tid;
+ pthread_internal_t* thread = __pthread_internal_find(t);
+ if (thread == NULL) {
+ return ESRCH;
}
- // We set the PTHREAD_ATTR_FLAG_JOINED flag with the lock held,
+ ThreadJoinState old_state = THREAD_NOT_JOINED;
+ while ((old_state == THREAD_NOT_JOINED || old_state == THREAD_EXITED_NOT_JOINED) &&
+ !atomic_compare_exchange_weak(&thread->join_state, &old_state, THREAD_JOINED)) {
+ }
+
+ if (old_state == THREAD_DETACHED || old_state == THREAD_JOINED) {
+ return EINVAL;
+ }
+
+ pid_t tid = thread->tid;
+ volatile int* tid_ptr = &thread->tid;
+
+ // We set thread->join_state to THREAD_JOINED with atomic operation,
// so no one is going to remove this thread except us.
// Wait for the thread to actually exit, if it hasn't already.
@@ -65,14 +61,10 @@
__futex_wait(tid_ptr, tid, NULL);
}
- // Take the lock again so we can pull the thread's return value
- // and remove the thread from the list.
- pthread_accessor thread(t);
-
if (return_value) {
*return_value = thread->return_value;
}
- _pthread_internal_remove_locked(thread.get(), true);
+ __pthread_internal_remove_and_free(thread);
return 0;
}
diff --git a/libc/bionic/pthread_kill.cpp b/libc/bionic/pthread_kill.cpp
index 163317e..93513fa 100644
--- a/libc/bionic/pthread_kill.cpp
+++ b/libc/bionic/pthread_kill.cpp
@@ -30,26 +30,17 @@
#include <unistd.h>
#include "private/ErrnoRestorer.h"
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
extern "C" int tgkill(int tgid, int tid, int sig);
int pthread_kill(pthread_t t, int sig) {
ErrnoRestorer errno_restorer;
- pthread_accessor thread(t);
- if (thread.get() == NULL) {
+ pthread_internal_t* thread = __pthread_internal_find(t);
+ if (thread == NULL) {
return ESRCH;
}
- // There's a race here, but it's one we share with all other C libraries.
- pid_t tid = thread->tid;
- thread.Unlock();
-
- int rc = tgkill(getpid(), tid, sig);
- if (rc == -1) {
- return errno;
- }
-
- return 0;
+ return (tgkill(getpid(), thread->tid, sig) == -1) ? errno : 0;
}
diff --git a/libc/bionic/pthread_setname_np.cpp b/libc/bionic/pthread_setname_np.cpp
index c4e9fb8..bb1114e 100644
--- a/libc/bionic/pthread_setname_np.cpp
+++ b/libc/bionic/pthread_setname_np.cpp
@@ -36,9 +36,8 @@
#include <sys/types.h>
#include <unistd.h>
-#include "pthread_accessor.h"
-#include "pthread_internal.h"
#include "private/ErrnoRestorer.h"
+#include "pthread_internal.h"
// This value is not exported by kernel headers.
#define MAX_TASK_COMM_LEN 16
@@ -58,14 +57,12 @@
}
// We have to change another thread's name.
- pid_t tid = 0;
- {
- pthread_accessor thread(t);
- if (thread.get() == NULL) {
- return ENOENT;
- }
- tid = thread->tid;
+ pthread_internal_t* thread = __pthread_internal_find(t);
+ if (thread == NULL) {
+ return ENOENT;
}
+ pid_t tid = thread->tid;
+
char comm_name[sizeof(TASK_COMM_FMT) + 8];
snprintf(comm_name, sizeof(comm_name), TASK_COMM_FMT, tid);
int fd = open(comm_name, O_CLOEXEC | O_WRONLY);
diff --git a/libc/bionic/pthread_setschedparam.cpp b/libc/bionic/pthread_setschedparam.cpp
index 419cc6f..0ad68bb 100644
--- a/libc/bionic/pthread_setschedparam.cpp
+++ b/libc/bionic/pthread_setschedparam.cpp
@@ -29,13 +29,13 @@
#include <errno.h>
#include "private/ErrnoRestorer.h"
-#include "pthread_accessor.h"
+#include "pthread_internal.h"
int pthread_setschedparam(pthread_t t, int policy, const sched_param* param) {
ErrnoRestorer errno_restorer;
- pthread_accessor thread(t);
- if (thread.get() == NULL) {
+ pthread_internal_t* thread = __pthread_internal_find(t);
+ if (thread == NULL) {
return ESRCH;
}