From 5ba4411cd94081c422d419cdc65298290c09ef9b Mon Sep 17 00:00:00 2001 From: "henrike@webrtc.org" Date: Fri, 5 Oct 2012 14:36:54 +0000 Subject: [PATCH] Makes it such that calling ThreadWrapper::Start(..) only returns false if a thread was never started. I.e. it will not return false if it was unable to configure the thread (however it does log such failures). Review URL: https://webrtc-codereview.appspot.com/869004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@2879 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../interface/thread_wrapper.h | 8 ++- .../source/system_wrappers.gyp | 4 ++ src/system_wrappers/source/thread_posix.cc | 72 +++++++++++++------ src/system_wrappers/source/thread_posix.h | 2 + .../source/thread_posix_unittest.cc | 30 ++++++++ src/system_wrappers/source/thread_unittest.cc | 8 +++ src/system_wrappers/source/thread_win.cc | 3 + 7 files changed, 102 insertions(+), 25 deletions(-) create mode 100644 src/system_wrappers/source/thread_posix_unittest.cc diff --git a/src/system_wrappers/interface/thread_wrapper.h b/src/system_wrappers/interface/thread_wrapper.h index 030ac8a6f7..cda4827b01 100644 --- a/src/system_wrappers/interface/thread_wrapper.h +++ b/src/system_wrappers/interface/thread_wrapper.h @@ -65,8 +65,12 @@ public: // to delete this class until the spawned thread has been reclaimed. virtual void SetNotAlive() = 0; - // Spawns the thread. This will start the triggering of the callback - // function. + // Tries to spawns a thread and returns true if that was successful. + // Additionally, it tries to set thread priority according to the priority + // from when CreateThread was called. However, failure to set priority will + // not result in a false return value. + // TODO(henrike): add a function for polling whether priority was set or + // not. virtual bool Start(unsigned int& id) = 0; // Sets the threads CPU affinity. CPUs are listed 0 - (number of CPUs - 1). diff --git a/src/system_wrappers/source/system_wrappers.gyp b/src/system_wrappers/source/system_wrappers.gyp index dcde1c7deb..f834709138 100644 --- a/src/system_wrappers/source/system_wrappers.gyp +++ b/src/system_wrappers/source/system_wrappers.gyp @@ -216,6 +216,7 @@ 'data_log_c_helpers_unittest.c', 'data_log_c_helpers_unittest.h', 'thread_unittest.cc', + 'thread_posix_unittest.cc', 'trace_unittest.cc', 'unittest_utilities_unittest.cc', ], @@ -225,6 +226,9 @@ }, { 'sources!': [ 'data_log_unittest.cc', ], }], + ['os_posix!=1', { + 'sources!': [ 'thread_posix_unittest.cc', ], + }], ], }, ], # targets diff --git a/src/system_wrappers/source/thread_posix.cc b/src/system_wrappers/source/thread_posix.cc index 14b3338224..35c9f8171a 100644 --- a/src/system_wrappers/source/thread_posix.cc +++ b/src/system_wrappers/source/thread_posix.cc @@ -44,6 +44,9 @@ #include "thread_posix.h" +#include + +#include #include #include // strncpy #include // nanosleep @@ -65,6 +68,32 @@ #include "system_wrappers/interface/trace.h" namespace webrtc { + +int ConvertToSystemPriority(ThreadPriority priority, int minPrio, int maxPrio) +{ + assert(maxPrio - minPrio > 2); + const int topPrio = maxPrio - 1; + const int lowPrio = minPrio + 1; + + switch (priority) + { + case kLowPriority: + return lowPrio; + case kNormalPriority: + // The -1 ensures that the kHighPriority is always greater or equal to + // kNormalPriority. + return (lowPrio + topPrio - 1) / 2; + case kHighPriority: + return std::max(topPrio - 2, lowPrio); + case kHighestPriority: + return std::max(topPrio - 1, lowPrio); + case kRealtimePriority: + return topPrio; + } + assert(false); + return lowPrio; +} + extern "C" { static void* StartThread(void* lpParameter) @@ -177,6 +206,11 @@ bool ThreadPosix::Start(unsigned int& /*threadID*/) const int policy = SCHED_FIFO; #endif _event->Reset(); + // If pthread_create was successful, a thread was created and is running. + // Don't return false if it was successful since if there are any other + // failures the state will be: thread was started but not configured as + // asked for. However, the caller of this API will assume that a false + // return value means that the thread never started. result |= pthread_create(&_thread, &_attr, &StartThread, this); if (result != 0) { @@ -187,9 +221,11 @@ bool ThreadPosix::Start(unsigned int& /*threadID*/) // race condition if Stop() is called too quickly after start. if (kEventSignaled != _event->Wait(WEBRTC_EVENT_10_SEC)) { + WEBRTC_TRACE(kTraceError, kTraceUtility, -1, + "posix thread event never triggered"); // Timed out. Something went wrong. _runFunction = NULL; - return false; + return true; } #if HAS_THREAD_ID @@ -201,31 +237,21 @@ bool ThreadPosix::Start(unsigned int& /*threadID*/) const int maxPrio = sched_get_priority_max(policy); if ((minPrio == EINVAL) || (maxPrio == EINVAL)) { - return false; + WEBRTC_TRACE(kTraceError, kTraceUtility, -1, + "unable to retreive min or max priority for threads"); + return true; } - - switch (_prio) + if (maxPrio - minPrio <= 2) { - case kLowPriority: - param.sched_priority = minPrio + 1; - break; - case kNormalPriority: - param.sched_priority = (minPrio + maxPrio) / 2; - break; - case kHighPriority: - param.sched_priority = maxPrio - 3; - break; - case kHighestPriority: - param.sched_priority = maxPrio - 2; - break; - case kRealtimePriority: - param.sched_priority = maxPrio - 1; - break; + // There is no room for setting priorities with any granularity. + return true; } + param.sched_priority = ConvertToSystemPriority(_prio, minPrio, maxPrio); result = pthread_setschedparam(_thread, policy, ¶m); if (result == EINVAL) { - return false; + WEBRTC_TRACE(kTraceError, kTraceUtility, -1, + "unable to set thread priority"); } return true; } @@ -344,7 +370,7 @@ void ThreadPosix::Run() #ifdef WEBRTC_LINUX prctl(PR_SET_NAME, (unsigned long)_name, 0, 0, 0); #endif - WEBRTC_TRACE(kTraceStateInfo, kTraceUtility,-1, + WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, -1, "Thread with name:%s started ", _name); } else { @@ -382,13 +408,13 @@ void ThreadPosix::Run() // coupling the thread and the trace class like this. if (strcmp(_name, "Trace")) { - WEBRTC_TRACE(kTraceStateInfo, kTraceUtility,-1, + WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, -1, "Thread with name:%s stopped", _name); } } else { - WEBRTC_TRACE(kTraceStateInfo, kTraceUtility,-1, + WEBRTC_TRACE(kTraceStateInfo, kTraceUtility, -1, "Thread without name stopped"); } { diff --git a/src/system_wrappers/source/thread_posix.h b/src/system_wrappers/source/thread_posix.h index 45489a8622..8fcdf10ced 100644 --- a/src/system_wrappers/source/thread_posix.h +++ b/src/system_wrappers/source/thread_posix.h @@ -19,6 +19,8 @@ namespace webrtc { class CriticalSectionWrapper; class EventWrapper; +int ConvertToSystemPriority(ThreadPriority priority, int minPrio, int maxPrio); + class ThreadPosix : public ThreadWrapper { public: diff --git a/src/system_wrappers/source/thread_posix_unittest.cc b/src/system_wrappers/source/thread_posix_unittest.cc new file mode 100644 index 0000000000..8d6237147b --- /dev/null +++ b/src/system_wrappers/source/thread_posix_unittest.cc @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2012 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "system_wrappers/source/thread_posix.h" + +#include "gtest/gtest.h" + +TEST(ThreadTestPosix, PrioritySettings) { + // API assumes that maxPrio - minPrio > 2. Test the extreme case. + const int kMinPrio = -1; + const int kMaxPrio = 2; + + int last_priority = kMinPrio; + for (int priority = webrtc::kLowPriority; + priority <= webrtc::kRealtimePriority; ++priority) { + int system_priority = webrtc::ConvertToSystemPriority( + static_cast(priority), kMinPrio, kMaxPrio); + EXPECT_GT(system_priority, kMinPrio); + EXPECT_LT(system_priority, kMaxPrio); + EXPECT_GE(system_priority, last_priority); + last_priority = system_priority; + } +} diff --git a/src/system_wrappers/source/thread_unittest.cc b/src/system_wrappers/source/thread_unittest.cc index 361424a7fb..6bbe136937 100644 --- a/src/system_wrappers/source/thread_unittest.cc +++ b/src/system_wrappers/source/thread_unittest.cc @@ -11,6 +11,7 @@ #include "system_wrappers/interface/thread_wrapper.h" #include "gtest/gtest.h" +#include "system_wrappers/interface/scoped_ptr.h" #include "system_wrappers/interface/trace.h" namespace webrtc { @@ -60,6 +61,13 @@ class ThreadTest : public ::testing::Test { TestTraceCallback trace_; }; +TEST_F(ThreadTest, NullFunctionPointer) { + webrtc::scoped_ptr thread( + webrtc::ThreadWrapper::CreateThread()); + unsigned int id = 42; + EXPECT_FALSE(thread->Start(id)); +} + // Function that does nothing, and reports success. bool NullRunFunction(void* /* obj */) { return true; diff --git a/src/system_wrappers/source/thread_win.cc b/src/system_wrappers/source/thread_win.cc index 07c586a87c..f458e0e9c1 100644 --- a/src/system_wrappers/source/thread_win.cc +++ b/src/system_wrappers/source/thread_win.cc @@ -75,6 +75,9 @@ unsigned int WINAPI ThreadWindows::StartThread(LPVOID lpParameter) bool ThreadWindows::Start(unsigned int& threadID) { + if (!_runFunction) { + return false; + } _doNotCloseHandle = false; // Set stack size to 1M