From 673b15e4ee2c6d99b150aedddc0f389e29f98e1b Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Thu, 19 Mar 2015 14:19:19 -0700 Subject: [PATCH] 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 --- libc/Android.mk | 2 +- libc/bionic/libc_init_common.cpp | 4 +- libc/bionic/pthread_accessor.h | 64 ------------------- libc/bionic/pthread_create.cpp | 10 +-- libc/bionic/pthread_detach.cpp | 35 +++++----- libc/bionic/pthread_exit.cpp | 4 +- libc/bionic/pthread_getcpuclockid.cpp | 6 +- libc/bionic/pthread_getschedparam.cpp | 6 +- ...ead_internals.cpp => pthread_internal.cpp} | 60 +++++++++++------ libc/bionic/pthread_internal.h | 12 ++-- libc/bionic/pthread_join.cpp | 44 ++++++------- libc/bionic/pthread_kill.cpp | 17 ++--- libc/bionic/pthread_setname_np.cpp | 15 ++--- libc/bionic/pthread_setschedparam.cpp | 6 +- 14 files changed, 107 insertions(+), 178 deletions(-) delete mode 100644 libc/bionic/pthread_accessor.h rename libc/bionic/{pthread_internals.cpp => pthread_internal.cpp} (70%) diff --git a/libc/Android.mk b/libc/Android.mk index 7bbdd9901..6f430ccfc 100644 --- a/libc/Android.mk +++ b/libc/Android.mk @@ -521,7 +521,7 @@ libc_pthread_src_files := \ bionic/pthread_getcpuclockid.cpp \ bionic/pthread_getschedparam.cpp \ bionic/pthread_gettid_np.cpp \ - bionic/pthread_internals.cpp \ + bionic/pthread_internal.cpp \ bionic/pthread_join.cpp \ bionic/pthread_key.cpp \ bionic/pthread_kill.cpp \ diff --git a/libc/bionic/libc_init_common.cpp b/libc/bionic/libc_init_common.cpp index 52ca0f228..36dc0854b 100644 --- a/libc/bionic/libc_init_common.cpp +++ b/libc/bionic/libc_init_common.cpp @@ -92,7 +92,7 @@ void __libc_init_tls(KernelArgumentBlock& args) { 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 @@ void __libc_init_common(KernelArgumentBlock& args) { // 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 df4a5a22b..000000000 --- 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 - -#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(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 a4bd054c7..ef3ce0572 100644 --- a/libc/bionic/pthread_create.cpp +++ b/libc/bionic/pthread_create.cpp @@ -83,7 +83,7 @@ void __init_alternate_signal_stack(pthread_internal_t* thread) { } } -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 @@ int __init_thread(pthread_internal_t* thread, bool add_to_thread_list) { thread->cleanup_stack = NULL; - if (add_to_thread_list) { - _pthread_internal_add(thread); - } - return error; } @@ -265,7 +261,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr, 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 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr, } // Publish the pthread_t and unlock the mutex to let the new thread start running. - *thread_out = reinterpret_cast(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 dcdb7b135..fb8e0dd01 100644 --- a/libc/bionic/pthread_detach.cpp +++ b/libc/bionic/pthread_detach.cpp @@ -29,27 +29,24 @@ #include #include -#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 81cc67be9..c2232a939 100644 --- a/libc/bionic/pthread_exit.cpp +++ b/libc/bionic/pthread_exit.cpp @@ -100,9 +100,7 @@ void pthread_exit(void* return_value) { __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 d11f56a2a..2bf200480 100644 --- a/libc/bionic/pthread_getcpuclockid.cpp +++ b/libc/bionic/pthread_getcpuclockid.cpp @@ -28,11 +28,11 @@ #include -#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 2cdc11a4c..052fb05f9 100644 --- a/libc/bionic/pthread_getschedparam.cpp +++ b/libc/bionic/pthread_getschedparam.cpp @@ -29,13 +29,13 @@ #include #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 0dd88fe80..1967ccf8c 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 @@ void _pthread_internal_add(pthread_internal_t* thread) { thread->next->prev = thread; } g_thread_list = thread; + return reinterpret_cast(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(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 538e0daaf..99c455e7b 100644 --- a/libc/bionic/pthread_internal.h +++ b/libc/bionic/pthread_internal.h @@ -105,10 +105,14 @@ struct pthread_internal_t { 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 @@ static inline __always_inline pthread_internal_t* __get_thread() { } __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 @@ __LIBC_HIDDEN__ void _pthread_internal_remove_locked(pthread_internal_t* thread, */ #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 15543b48e..4d852cb4d 100644 --- a/libc/bionic/pthread_join.cpp +++ b/libc/bionic/pthread_join.cpp @@ -29,35 +29,31 @@ #include #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 @@ int pthread_join(pthread_t t, void** return_value) { __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 163317efc..93513fac3 100644 --- a/libc/bionic/pthread_kill.cpp +++ b/libc/bionic/pthread_kill.cpp @@ -30,26 +30,17 @@ #include #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 c4e9fb85c..bb1114e3d 100644 --- a/libc/bionic/pthread_setname_np.cpp +++ b/libc/bionic/pthread_setname_np.cpp @@ -36,9 +36,8 @@ #include #include -#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 @@ int pthread_setname_np(pthread_t t, const char* thread_name) { } // 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 419cc6f81..0ad68bb38 100644 --- a/libc/bionic/pthread_setschedparam.cpp +++ b/libc/bionic/pthread_setschedparam.cpp @@ -29,13 +29,13 @@ #include #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; }