diff options
| -rw-r--r-- | libs/binder/ProcessState.cpp | 34 | ||||
| -rw-r--r-- | libs/binder/include/binder/IPCThreadState.h | 7 | ||||
| -rw-r--r-- | libs/binder/include/binder/ProcessState.h | 23 | ||||
| -rw-r--r-- | libs/binder/ndk/include_platform/android/binder_process.h | 21 | ||||
| -rw-r--r-- | libs/binder/rust/src/state.rs | 9 | ||||
| -rw-r--r-- | libs/binder/tests/binderLibTest.cpp | 28 |
6 files changed, 98 insertions, 24 deletions
diff --git a/libs/binder/ProcessState.cpp b/libs/binder/ProcessState.cpp index 3fa686782a..02b0447304 100644 --- a/libs/binder/ProcessState.cpp +++ b/libs/binder/ProcessState.cpp @@ -192,6 +192,7 @@ void ProcessState::startThreadPool() AutoMutex _l(mLock); if (!mThreadPoolStarted) { if (mMaxThreads == 0) { + // see also getThreadPoolMaxTotalThreadCount ALOGW("Extra binder thread started, but 0 threads requested. Do not use " "*startThreadPool when zero threads are requested."); } @@ -407,6 +408,11 @@ void ProcessState::spawnPooledThread(bool isMain) mKernelStartedThreads++; pthread_mutex_unlock(&mThreadCountLock); } + // TODO: if startThreadPool is called on another thread after the process + // starts up, the kernel might think that it already requested those + // binder threads, and additional won't be started. This is likely to + // cause deadlocks, and it will also cause getThreadPoolMaxTotalThreadCount + // to return too high of a value. } status_t ProcessState::setThreadPoolMaxThreadCount(size_t maxThreads) { @@ -426,12 +432,32 @@ size_t ProcessState::getThreadPoolMaxTotalThreadCount() const { pthread_mutex_lock(&mThreadCountLock); base::ScopeGuard detachGuard = [&]() { pthread_mutex_unlock(&mThreadCountLock); }; - // may actually be one more than this, if join is called if (mThreadPoolStarted) { - return mCurrentThreads < mKernelStartedThreads - ? mMaxThreads - : mMaxThreads + mCurrentThreads - mKernelStartedThreads; + LOG_ALWAYS_FATAL_IF(mKernelStartedThreads > mMaxThreads + 1, + "too many kernel-started threads: %zu > %zu + 1", mKernelStartedThreads, + mMaxThreads); + + // calling startThreadPool starts a thread + size_t threads = 1; + + // the kernel is configured to start up to mMaxThreads more threads + threads += mMaxThreads; + + // Users may call IPCThreadState::joinThreadPool directly. We don't + // currently have a way to count this directly (it could be added by + // adding a separate private joinKernelThread method in IPCThreadState). + // So, if we are in a race between the kernel thread variable being + // incremented in this file and mCurrentThreads being incremented + // in IPCThreadState, temporarily forget about the extra join threads. + // This is okay, because most callers of this method only care about + // having 0, 1, or more threads. + if (mCurrentThreads > mKernelStartedThreads) { + threads += mCurrentThreads - mKernelStartedThreads; + } + + return threads; } + // must not be initialized or maybe has poll thread setup, we // currently don't track this in libbinder LOG_ALWAYS_FATAL_IF(mKernelStartedThreads != 0, diff --git a/libs/binder/include/binder/IPCThreadState.h b/libs/binder/include/binder/IPCThreadState.h index d261c2143c..9347ce47a5 100644 --- a/libs/binder/include/binder/IPCThreadState.h +++ b/libs/binder/include/binder/IPCThreadState.h @@ -147,7 +147,12 @@ public: void flushCommands(); bool flushIfNeeded(); - // For main functions - dangerous for libraries to use + // Adds the current thread into the binder threadpool. + // + // This is in addition to any threads which are started + // with startThreadPool. Libraries should not call this + // function, as they may be loaded into processes which + // try to configure the threadpool differently. void joinThreadPool(bool isMain = true); // Stop the local process. diff --git a/libs/binder/include/binder/ProcessState.h b/libs/binder/include/binder/ProcessState.h index 81391e96e7..9dc370b412 100644 --- a/libs/binder/include/binder/ProcessState.h +++ b/libs/binder/include/binder/ProcessState.h @@ -52,7 +52,26 @@ public: sp<IBinder> getContextObject(const sp<IBinder>& caller); - // For main functions - dangerous for libraries to use + // This should be called before startThreadPool at the beginning + // of a program, and libraries should never call it because programs + // should configure their own threadpools. The threadpool size can + // never be decreased. + // + // The 'maxThreads' value refers to the total number of threads + // that will be started by the kernel. This is in addition to any + // threads started by 'startThreadPool' or 'joinRpcThreadpool'. + status_t setThreadPoolMaxThreadCount(size_t maxThreads); + + // Libraries should not call this, as processes should configure + // threadpools themselves. Should be called in the main function + // directly before any code executes or joins the threadpool. + // + // Starts one thread, PLUS those requested in setThreadPoolMaxThreadCount, + // PLUS those manually requested in joinThreadPool. + // + // For instance, if setThreadPoolMaxCount(3) is called and + // startThreadpPool (+1 thread) and joinThreadPool (+1 thread) + // are all called, then up to 5 threads can be started. void startThreadPool(); [[nodiscard]] bool becomeContextManager(); @@ -63,8 +82,6 @@ public: // TODO: deprecate. void spawnPooledThread(bool isMain); - // For main functions - dangerous for libraries to use - status_t setThreadPoolMaxThreadCount(size_t maxThreads); status_t enableOnewaySpamDetection(bool enable); // Set the name of the current thread to look like a threadpool diff --git a/libs/binder/ndk/include_platform/android/binder_process.h b/libs/binder/ndk/include_platform/android/binder_process.h index 3fbe90d70a..68528e1004 100644 --- a/libs/binder/ndk/include_platform/android/binder_process.h +++ b/libs/binder/ndk/include_platform/android/binder_process.h @@ -24,7 +24,14 @@ __BEGIN_DECLS /** - * This creates a threadpool for incoming binder transactions if it has not already been created. + * This creates a threadpool for incoming binder transactions if it has not already been created, + * spawning one thread, and allowing the kernel to lazily start threads according to the count + * that is specified in ABinderProcess_setThreadPoolMaxThreadCount. + * + * For instance, if ABinderProcess_setThreadPoolMaxThreadCount(3) is called, + * ABinderProcess_startThreadPool() is called (+1 thread) then the main thread calls + * ABinderProcess_joinThreadPool() (+1 thread), up to *5* total threads will be started + * (2 directly, and 3 more if the kernel starts them lazily). * * When using this, it is expected that ABinderProcess_setupPolling and * ABinderProcess_handlePolledCommands are not used. @@ -36,7 +43,12 @@ void ABinderProcess_startThreadPool(void); /** * This sets the maximum number of threads that can be started in the threadpool. By default, after * startThreadPool is called, this is 15. If it is called additional times, it will only prevent - * the kernel from starting new threads and will not delete already existing threads. + * the kernel from starting new threads and will not delete already existing threads. This should + * be called once before startThreadPool. The number of threads can never decrease. + * + * This count refers to the number of threads that will be created lazily by the kernel, in + * addition to the threads created by ABinderProcess_startThreadPool or + * ABinderProcess_joinThreadPool. * * Do not use this from a library. Apps setup their own threadpools, and otherwise, the main * function should be responsible for configuring the threadpool for the entire application. @@ -50,8 +62,9 @@ bool ABinderProcess_setThreadPoolMaxThreadCount(uint32_t numThreads); */ bool ABinderProcess_isThreadPoolStarted(void); /** - * This adds the current thread to the threadpool. This may cause the threadpool to exceed the - * maximum size. + * This adds the current thread to the threadpool. This thread will be in addition to the thread + * started by ABinderProcess_startThreadPool and the lazy kernel-started threads specified by + * ABinderProcess_setThreadPoolMaxThreadCount. * * Do not use this from a library. Apps setup their own threadpools, and otherwise, the main * function should be responsible for configuring the threadpool for the entire application. diff --git a/libs/binder/rust/src/state.rs b/libs/binder/rust/src/state.rs index cc18741a4e..b35511d74e 100644 --- a/libs/binder/rust/src/state.rs +++ b/libs/binder/rust/src/state.rs @@ -23,6 +23,9 @@ pub struct ProcessState; impl ProcessState { /// Start the Binder IPC thread pool + /// + /// Starts 1 thread, plus allows the kernel to lazily start up to 'num_threads' + /// additional threads as specified by set_thread_pool_max_thread_count. pub fn start_thread_pool() { unsafe { // Safety: Safe FFI @@ -33,8 +36,7 @@ impl ProcessState { /// Set the maximum number of threads that can be started in the threadpool. /// /// By default, after startThreadPool is called, this is 15. If it is called - /// additional times, it will only prevent the kernel from starting new - /// threads and will not delete already existing threads. + /// additional times, the thread pool size can only be increased. pub fn set_thread_pool_max_thread_count(num_threads: u32) { unsafe { // Safety: Safe FFI @@ -43,6 +45,9 @@ impl ProcessState { } /// Block on the Binder IPC thread pool + /// + /// This adds additional threads in addition to what is created by + /// set_thread_pool_max_thread_count and start_thread_pool. pub fn join_thread_pool() { unsafe { // Safety: Safe FFI diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp index abc423b669..e021af0264 100644 --- a/libs/binder/tests/binderLibTest.cpp +++ b/libs/binder/tests/binderLibTest.cpp @@ -82,7 +82,7 @@ static char binderserverarg[] = "--binderserver"; static constexpr int kSchedPolicy = SCHED_RR; static constexpr int kSchedPriority = 7; static constexpr int kSchedPriorityMore = 8; -static constexpr int kKernelThreads = 15; +static constexpr int kKernelThreads = 17; // anything different than the default static String16 binderLibTestServiceName = String16("test.binderLib"); @@ -1357,17 +1357,20 @@ TEST_F(BinderLibTest, ThreadPoolAvailableThreads) { EXPECT_THAT(server->transact(BINDER_LIB_TEST_GET_MAX_THREAD_COUNT, data, &reply), StatusEq(NO_ERROR)); int32_t replyi = reply.readInt32(); - // Expect 16 threads: kKernelThreads = 15 + Pool thread == 16 - EXPECT_TRUE(replyi == kKernelThreads || replyi == kKernelThreads + 1); + // see getThreadPoolMaxTotalThreadCount for why there is a race + EXPECT_TRUE(replyi == kKernelThreads + 1 || replyi == kKernelThreads + 2) << replyi; + EXPECT_THAT(server->transact(BINDER_LIB_TEST_PROCESS_LOCK, data, &reply), NO_ERROR); /* - * This will use all threads in the pool expect the main pool thread. - * The service should run fine without locking, and the thread count should - * not exceed 16 (15 Max + pool thread). + * This will use all threads in the pool but one. There are actually kKernelThreads+2 + * available in the other process (startThreadPool, joinThreadPool, + the kernel- + * started threads from setThreadPoolMaxThreadCount + * + * Adding one more will cause it to deadlock. */ std::vector<std::thread> ts; - for (size_t i = 0; i < kKernelThreads; i++) { + for (size_t i = 0; i < kKernelThreads + 1; i++) { ts.push_back(std::thread([&] { Parcel local_reply; EXPECT_THAT(server->transact(BINDER_LIB_TEST_LOCK_UNLOCK, data, &local_reply), @@ -1375,8 +1378,13 @@ TEST_F(BinderLibTest, ThreadPoolAvailableThreads) { })); } - data.writeInt32(500); - // Give a chance for all threads to be used + // make sure all of the above calls will be queued in parallel. Otherwise, most of + // the time, the below call will pre-empt them (presumably because we have the + // scheduler timeslice already + scheduler hint). + sleep(1); + + data.writeInt32(1000); + // Give a chance for all threads to be used (kKernelThreads + 1 thread in use) EXPECT_THAT(server->transact(BINDER_LIB_TEST_UNLOCK_AFTER_MS, data, &reply), NO_ERROR); for (auto &t : ts) { @@ -1386,7 +1394,7 @@ TEST_F(BinderLibTest, ThreadPoolAvailableThreads) { EXPECT_THAT(server->transact(BINDER_LIB_TEST_GET_MAX_THREAD_COUNT, data, &reply), StatusEq(NO_ERROR)); replyi = reply.readInt32(); - EXPECT_EQ(replyi, kKernelThreads + 1); + EXPECT_EQ(replyi, kKernelThreads + 2); } TEST_F(BinderLibTest, ThreadPoolStarted) { |