From 8f676d716d9e8b808a17ecff6df2847a59ed015a Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Wed, 10 Apr 2024 22:20:25 +1200 Subject: [PATCH] Darwin: PlatformManagerImpl improvements (#32904) * Small improvements to TestPlatformMgr.cpp Use NL_TEST_ASSERT_SUCCESS when checking errors Add a few more assertions * Add AtomicGlobal as a thread-safe variant of Global It is simply an alias for the same type if CHIP_CONFIG_GLOBALS_LAZY_INIT is not enabled, as the eager implementation of Global is thread-safe anyway. Use "friend" instead of "friend class" where the type might be an alias. * Darwin: PlatformManagerImpl improvements Make PlatformMgr[Impl]() thread-safe by using an AtomicGlobal Make GetWorkQueue() thread-safe by creating the queue in the constructor Make {Start,Stop}EventLoopTask thread-safe using an atomic for state Signal the sempahore only if non-null, no matter where stop is called from Other minor tweaks * Don't reference std::call_once unless CHIP_CONFIG_GLOBALS_LAZY_INIT=1 --- src/lib/core/Global.h | 86 +++++++++++++--- src/lib/dnssd/Discovery_ImplPlatform.h | 2 +- src/platform/Darwin/DnssdImpl.h | 2 +- src/platform/Darwin/PlatformManagerImpl.cpp | 108 +++++++------------- src/platform/Darwin/PlatformManagerImpl.h | 32 +++--- src/platform/tests/TestPlatformMgr.cpp | 34 +++--- 6 files changed, 147 insertions(+), 117 deletions(-) diff --git a/src/lib/core/Global.h b/src/lib/core/Global.h index 26de6b58381ccd..ddecf2bbeddb5b 100644 --- a/src/lib/core/Global.h +++ b/src/lib/core/Global.h @@ -19,9 +19,48 @@ #include +#include #include +#if CHIP_SYSTEM_CONFIG_USE_DISPATCH +#include +#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH + namespace chip { +namespace detail { + +#if CHIP_CONFIG_GLOBALS_LAZY_INIT + +struct NonAtomicOnce +{ + bool mInitialized = false; + void call(void (*func)(void *), void * context) + { + if (!mInitialized) + { + mInitialized = true; + func(context); + } + } +}; + +struct AtomicOnce +{ +// dispatch_once (if available) is more efficient than std::call_once because +// it takes advantage of the additional assumption that the dispatch_once_t +// is allocated within a static / global. +#if CHIP_SYSTEM_CONFIG_USE_DISPATCH + dispatch_once_t mOnce = 0; + void call(void (*func)(void *), void * context) { dispatch_once_f(&mOnce, context, func); } +#else // CHIP_SYSTEM_CONFIG_USE_DISPATCH + std::once_flag mOnce; + void call(void (*func)(void *), void * context) { std::call_once(mOnce, func, context); } +#endif +}; + +#endif // CHIP_CONFIG_GLOBALS_LAZY_INIT + +} // namespace detail /** * A wrapper for global object that enables initialization and destruction to @@ -29,9 +68,16 @@ namespace chip { * * The contained object of type T is default constructed, possibly lazily. * - * This class is generally NOT thread-safe; external synchronization is required. + * Values of this type MUST be globals or static class members. + * + * This class is not thread-safe; external synchronization is required. + * @see AtomicGlobal for a thread-safe variant. */ +#if CHIP_CONFIG_GLOBALS_LAZY_INIT +template +#else // CHIP_CONFIG_GLOBALS_LAZY_INIT template +#endif // CHIP_CONFIG_GLOBALS_LAZY_INIT class Global { public: @@ -40,6 +86,11 @@ class Global T & get() { return _get(); } T * operator->() { return &_get(); } + // Globals are not copyable or movable + Global(const Global &) = delete; + Global(const Global &&) = delete; + Global & operator=(const Global &) = delete; + #if CHIP_CONFIG_GLOBALS_LAZY_INIT public: constexpr Global() = default; @@ -49,24 +100,22 @@ class Global // Zero-initialize everything. We should technically leave mStorage uninitialized, // but that can sometimes cause clang to be unable to constant-initialize the object. alignas(T) unsigned char mStorage[sizeof(T)] = {}; - bool mInitialized = false; - - T & _value() { return *reinterpret_cast(mStorage); } + OnceStrategy mOnce; T & _get() { - if (!mInitialized) - { - new (mStorage) T(); - mInitialized = true; + T * value = reinterpret_cast(mStorage); + mOnce.call(&create, value); + return *value; + } + static void create(void * value) + { + new (value) T(); #if !CHIP_CONFIG_GLOBALS_NO_DESTRUCT - CHIP_CXA_ATEXIT(&destroy, this); + CHIP_CXA_ATEXIT(&destroy, value); #endif // CHIP_CONFIG_GLOBALS_NO_DESTRUCT - } - return _value(); } - - static void destroy(void * context) { static_cast *>(context)->_value().~T(); } + static void destroy(void * value) { static_cast(value)->~T(); } #else // CHIP_CONFIG_GLOBALS_LAZY_INIT public: @@ -100,4 +149,15 @@ class Global #endif // CHIP_CONFIG_GLOBALS_LAZY_INIT }; +/** + * A variant of Global that is thread-safe. + */ +template +using AtomicGlobal = +#if CHIP_CONFIG_GLOBALS_LAZY_INIT + Global; +#else // CHIP_CONFIG_GLOBALS_LAZY_INIT + Global; // eager globals are already thread-safe +#endif // CHIP_CONFIG_GLOBALS_LAZY_INIT + } // namespace chip diff --git a/src/lib/dnssd/Discovery_ImplPlatform.h b/src/lib/dnssd/Discovery_ImplPlatform.h index c6ba929801ba7b..34fedf831b5ecf 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.h +++ b/src/lib/dnssd/Discovery_ImplPlatform.h @@ -95,7 +95,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver uint8_t mCommissionableInstanceName[sizeof(uint64_t)]; OperationalResolveDelegate * mOperationalDelegate = nullptr; - friend class Global; + friend Global; static Global sManager; }; diff --git a/src/platform/Darwin/DnssdImpl.h b/src/platform/Darwin/DnssdImpl.h index de5419b0bf7907..3cf48db33545a5 100644 --- a/src/platform/Darwin/DnssdImpl.h +++ b/src/platform/Darwin/DnssdImpl.h @@ -131,7 +131,7 @@ class MdnsContexts private: MdnsContexts() = default; - friend class Global; + friend Global; static Global sInstance; std::vector mContexts; diff --git a/src/platform/Darwin/PlatformManagerImpl.cpp b/src/platform/Darwin/PlatformManagerImpl.cpp index d0289f7b7e0a18..b7dc8b681fa8e8 100644 --- a/src/platform/Darwin/PlatformManagerImpl.cpp +++ b/src/platform/Darwin/PlatformManagerImpl.cpp @@ -31,6 +31,7 @@ #endif #include +#include #include // Include the non-inline definitions for the GenericPlatformManagerImpl<> template, @@ -39,22 +40,28 @@ #include #include -#import "PlatformMetricKeys.h" using namespace chip::Tracing::DarwinPlatform; namespace chip { namespace DeviceLayer { -Global PlatformManagerImpl::sInstance; +AtomicGlobal PlatformManagerImpl::sInstance; -CHIP_ERROR PlatformManagerImpl::_InitChipStack() +PlatformManagerImpl::PlatformManagerImpl() : + mWorkQueue(dispatch_queue_create("org.csa-iot.matter.workqueue", + dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL, + QOS_CLASS_USER_INITIATED, QOS_MIN_RELATIVE_PRIORITY))) { - CHIP_ERROR err; + // Tag our queue for IsWorkQueueCurrentQueue() + dispatch_queue_set_specific(mWorkQueue, this, this, nullptr); + dispatch_suspend(mWorkQueue); +} +CHIP_ERROR PlatformManagerImpl::_InitChipStack() +{ // Initialize the configuration system. #if !CHIP_DISABLE_PLATFORM_KVS - err = Internal::PosixConfig::Init(); - SuccessOrExit(err); + ReturnErrorOnFailure(Internal::PosixConfig::Init()); #endif // CHIP_DISABLE_PLATFORM_KVS #if !CHIP_SYSTEM_CONFIG_USE_LIBEV @@ -64,8 +71,7 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack() // Call _InitChipStack() on the generic implementation base class // to finish the initialization process. - err = Internal::GenericPlatformManagerImpl::_InitChipStack(); - SuccessOrExit(err); + ReturnErrorOnFailure(Internal::GenericPlatformManagerImpl::_InitChipStack()); #if !CHIP_DISABLE_PLATFORM_KVS // Now set up our device instance info provider. We couldn't do that @@ -74,53 +80,36 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack() #endif // CHIP_DISABLE_PLATFORM_KVS mStartTime = System::SystemClock().GetMonotonicTimestamp(); - -exit: - return err; + return CHIP_NO_ERROR; } CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask() { - if (mIsWorkQueueSuspended) - { - mIsWorkQueueSuspended = false; - dispatch_resume(mWorkQueue); - } - + auto expected = WorkQueueState::kSuspended; + VerifyOrReturnError(mWorkQueueState.compare_exchange_strong(expected, WorkQueueState::kRunning), CHIP_ERROR_INCORRECT_STATE); + dispatch_resume(mWorkQueue); return CHIP_NO_ERROR; }; CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask() { - if (!mIsWorkQueueSuspended && !mIsWorkQueueSuspensionPending) - { - mIsWorkQueueSuspensionPending = true; - if (!IsWorkQueueCurrentQueue()) - { - // dispatch_sync is used in order to guarantee serialization of the caller with - // respect to any tasks that might already be on the queue, or running. - dispatch_sync(mWorkQueue, ^{ - dispatch_suspend(mWorkQueue); - }); - - mIsWorkQueueSuspended = true; - mIsWorkQueueSuspensionPending = false; - } - else + auto expected = WorkQueueState::kRunning; + VerifyOrReturnError(mWorkQueueState.compare_exchange_strong(expected, WorkQueueState::kSuspensionPending), + CHIP_ERROR_INCORRECT_STATE); + + // We need to dispatch to the work queue to ensure any currently queued jobs + // finish executing. When called from outside the work queue we also need to + // wait for them to complete before returning to the caller, so we use + // dispatch_sync in that case. + (IsWorkQueueCurrentQueue() ? dispatch_async : dispatch_sync)(mWorkQueue, ^{ + dispatch_suspend(mWorkQueue); + mWorkQueueState.store(WorkQueueState::kSuspended); + auto * semaphore = mRunLoopSem; + if (semaphore != nullptr) { - // We are called from a task running on our work queue. Dispatch async, - // so we don't deadlock ourselves. Note that we do have to dispatch to - // guarantee that we don't signal the semaphore until we have ensured - // that no more tasks will run on the queue. - dispatch_async(mWorkQueue, ^{ - dispatch_suspend(mWorkQueue); - mIsWorkQueueSuspended = true; - mIsWorkQueueSuspensionPending = false; - dispatch_semaphore_signal(mRunLoopSem); - }); + dispatch_semaphore_signal(semaphore); } - } - + }); return CHIP_NO_ERROR; } @@ -147,14 +136,9 @@ void PlatformManagerImpl::_Shutdown() CHIP_ERROR PlatformManagerImpl::_PostEvent(const ChipDeviceEvent * event) { - if (mWorkQueue == nullptr) - { - return CHIP_ERROR_INCORRECT_STATE; - } - const ChipDeviceEvent eventCopy = *event; dispatch_async(mWorkQueue, ^{ - Impl()->DispatchEvent(&eventCopy); + DispatchEvent(&eventCopy); }); return CHIP_NO_ERROR; } @@ -162,32 +146,14 @@ CHIP_ERROR PlatformManagerImpl::_PostEvent(const ChipDeviceEvent * event) #if CHIP_STACK_LOCK_TRACKING_ENABLED bool PlatformManagerImpl::_IsChipStackLockedByCurrentThread() const { - // If we have no work queue, or it's suspended, then we assume our caller - // knows what they are doing in terms of their own concurrency. - return !mWorkQueue || mIsWorkQueueSuspended || IsWorkQueueCurrentQueue(); + // Assume our caller knows what they are doing in terms of concurrency if the work queue is suspended. + return IsWorkQueueCurrentQueue() || mWorkQueueState.load() == WorkQueueState::kSuspended; }; #endif -static int sPlatformManagerKey; // We use pointer to this as key. - -dispatch_queue_t PlatformManagerImpl::GetWorkQueue() -{ - if (mWorkQueue == nullptr) - { - mWorkQueue = - dispatch_queue_create(CHIP_CONTROLLER_QUEUE, - dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL, - QOS_CLASS_USER_INITIATED, QOS_MIN_RELATIVE_PRIORITY)); - dispatch_suspend(mWorkQueue); - dispatch_queue_set_specific(mWorkQueue, &sPlatformManagerKey, this, nullptr); - mIsWorkQueueSuspended = true; - } - return mWorkQueue; -} - bool PlatformManagerImpl::IsWorkQueueCurrentQueue() const { - return dispatch_get_specific(&sPlatformManagerKey) == this; + return dispatch_get_specific(this) == this; } CHIP_ERROR PlatformManagerImpl::StartBleScan(BleScannerDelegate * delegate) diff --git a/src/platform/Darwin/PlatformManagerImpl.h b/src/platform/Darwin/PlatformManagerImpl.h index 7f3aba0b1a7b8e..12b515ae83d42f 100644 --- a/src/platform/Darwin/PlatformManagerImpl.h +++ b/src/platform/Darwin/PlatformManagerImpl.h @@ -26,10 +26,9 @@ #include #include +#include #include -static constexpr const char * const CHIP_CONTROLLER_QUEUE = "org.csa-iot.matter.framework.controller.workqueue"; - namespace chip { namespace DeviceLayer { @@ -47,7 +46,7 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener public: // ===== Platform-specific members that may be accessed directly by the application. - dispatch_queue_t GetWorkQueue(); + dispatch_queue_t GetWorkQueue() { return mWorkQueue; } bool IsWorkQueueCurrentQueue() const; CHIP_ERROR StartBleScan(BleScannerDelegate * delegate = nullptr); @@ -80,21 +79,26 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener friend PlatformManager & PlatformMgr(void); friend PlatformManagerImpl & PlatformMgrImpl(void); - static Global sInstance; + friend AtomicGlobal; + static AtomicGlobal sInstance; + + PlatformManagerImpl(); System::Clock::Timestamp mStartTime = System::Clock::kZero; - dispatch_queue_t mWorkQueue = nullptr; - // Semaphore used to implement blocking behavior in _RunEventLoop. - dispatch_semaphore_t mRunLoopSem; + dispatch_queue_t mWorkQueue; - bool mIsWorkQueueSuspended = false; - // TODO: mIsWorkQueueSuspensionPending might need to be an atomic and use - // atomic ops, if we're worried about calls to StopEventLoopTask() from - // multiple threads racing somehow... - bool mIsWorkQueueSuspensionPending = false; + enum class WorkQueueState + { + kSuspended, + kRunning, + kSuspensionPending, + }; - inline ImplClass * Impl() { return static_cast(this); } + std::atomic mWorkQueueState = WorkQueueState::kSuspended; + + // Semaphore used to implement blocking behavior in _RunEventLoop. + dispatch_semaphore_t mRunLoopSem; }; /** @@ -112,7 +116,7 @@ inline PlatformManager & PlatformMgr(void) * Returns the platform-specific implementation of the PlatformManager singleton object. * * chip applications can use this to gain access to features of the PlatformManager - * that are specific to the ESP32 platform. + * that are specific to the platform. */ inline PlatformManagerImpl & PlatformMgrImpl(void) { diff --git a/src/platform/tests/TestPlatformMgr.cpp b/src/platform/tests/TestPlatformMgr.cpp index 89bad3b4c535f0..87c7913ebb7c3b 100644 --- a/src/platform/tests/TestPlatformMgr.cpp +++ b/src/platform/tests/TestPlatformMgr.cpp @@ -32,6 +32,7 @@ #include #include +#include #include #include #include @@ -60,14 +61,12 @@ static void TestPlatformMgr_BasicEventLoopTask(nlTestSuite * inSuite, void * inC { std::atomic counterRun{ 0 }; - CHIP_ERROR err = PlatformMgr().InitChipStack(); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT_SUCCESS(inSuite, PlatformMgr().InitChipStack()); // Start/stop the event loop task a few times. for (size_t i = 0; i < 3; i++) { - err = PlatformMgr().StartEventLoopTask(); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT_SUCCESS(inSuite, PlatformMgr().StartEventLoopTask()); std::atomic counterSync{ 2 }; @@ -99,8 +98,7 @@ static void TestPlatformMgr_BasicEventLoopTask(nlTestSuite * inSuite, void * inC for (size_t t = 0; counterSync != 0 && t < 1000; t++) chip::test_utils::SleepMillis(1); - err = PlatformMgr().StopEventLoopTask(); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT_SUCCESS(inSuite, PlatformMgr().StopEventLoopTask()); // Sleep for a short time to allow the event loop to stop. // Note, in some platform implementations the event loop thread @@ -115,26 +113,28 @@ static void TestPlatformMgr_BasicEventLoopTask(nlTestSuite * inSuite, void * inC } static bool stopRan; +static CHIP_ERROR stopResult = CHIP_NO_ERROR; static void StopTheLoop(intptr_t) { // Testing the return value here would involve multi-threaded access to the // nlTestSuite, and it's not clear whether that's OK. - stopRan = true; - PlatformMgr().StopEventLoopTask(); + stopRan = true; + stopResult = PlatformMgr().StopEventLoopTask(); } static void TestPlatformMgr_BasicRunEventLoop(nlTestSuite * inSuite, void * inContext) { stopRan = false; - CHIP_ERROR err = PlatformMgr().InitChipStack(); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT_SUCCESS(inSuite, PlatformMgr().InitChipStack()); PlatformMgr().ScheduleWork(StopTheLoop); + NL_TEST_ASSERT(inSuite, !stopRan); PlatformMgr().RunEventLoop(); NL_TEST_ASSERT(inSuite, stopRan); + NL_TEST_ASSERT_SUCCESS(inSuite, stopResult); PlatformMgr().Shutdown(); } @@ -152,12 +152,13 @@ static void TestPlatformMgr_RunEventLoopTwoTasks(nlTestSuite * inSuite, void * i stopRan = false; sleepRan = false; - CHIP_ERROR err = PlatformMgr().InitChipStack(); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT_SUCCESS(inSuite, PlatformMgr().InitChipStack()); PlatformMgr().ScheduleWork(SleepSome); PlatformMgr().ScheduleWork(StopTheLoop); + NL_TEST_ASSERT(inSuite, !stopRan); + NL_TEST_ASSERT(inSuite, !sleepRan); PlatformMgr().RunEventLoop(); NL_TEST_ASSERT(inSuite, stopRan); NL_TEST_ASSERT(inSuite, sleepRan); @@ -177,11 +178,12 @@ static void TestPlatformMgr_RunEventLoopStopBeforeSleep(nlTestSuite * inSuite, v stopRan = false; sleepRan = false; - CHIP_ERROR err = PlatformMgr().InitChipStack(); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT_SUCCESS(inSuite, PlatformMgr().InitChipStack()); PlatformMgr().ScheduleWork(StopAndSleep); + NL_TEST_ASSERT(inSuite, !stopRan); + NL_TEST_ASSERT(inSuite, !sleepRan); PlatformMgr().RunEventLoop(); NL_TEST_ASSERT(inSuite, stopRan); NL_TEST_ASSERT(inSuite, sleepRan); @@ -206,10 +208,8 @@ void DeviceEventHandler(const ChipDeviceEvent * event, intptr_t arg) static void TestPlatformMgr_AddEventHandler(nlTestSuite * inSuite, void * inContext) { - CHIP_ERROR error; sEventRecieved = 0; - error = PlatformMgr().AddEventHandler(DeviceEventHandler, 12345); - NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR); + NL_TEST_ASSERT_SUCCESS(inSuite, PlatformMgr().AddEventHandler(DeviceEventHandler, 12345)); #if 0 while (sEventRecieved == 0)