From 93ec12f236d082147553ceae600be5a570df8d29 Mon Sep 17 00:00:00 2001 From: Lukas Zeller Date: Tue, 29 Aug 2023 17:57:44 +0200 Subject: [PATCH] SystemLayerImplSelect: libev: avoid timers firing early (#28434) (#28740) * SystemLayerImplSelect: libev: avoid timers firing slightly too early (#28434) This fixes a ReportEngine problem that was caused by libev based timers firing slightly (in the range of 20mS) too early, because libev by default uses the time when events started processing as the "now" reference for relative timers for efficiency reasons. To ensure timers cannot fire early, timer setup must compensate for any difference between ev_now() which is the time events started processing and ev_time(), which is the actual current time (but is a bit less efficient to obtain). # Conflicts: # src/system/SystemLayerImplSelect.cpp * SystemLayerImplSelect: libev: eliminate I/O wakeup thread libev based regular builds do not need a separate I/O wakeup thread, so the wakeup thread can be eliminated in CHIP_SYSTEM_CONFIG_USE_LIBEV case. In normal operation, libev based builds also never call Signal(), so if it is still called it now emits a error log message, to indicate something might be wrong in the setup. However we keep Signal() and related LayerSocketsLoop methods, following the Darwin dispatch implementation, as fallback to select-based event handling seems to be needed for some I/O tests. As noted in a comment to the original libev PR [1], eventually, the select() based mainloop and external mainloop based solutions like Darwin Dispatch and libev should be detangled and extracted into separate classes, adapting all the tests that somehow rely on the select() fallback. As a single self-funded developer however, I cannot possibly be expected to solve this for Apple ;-) [1] https://github.com/project-chip/connectedhomeip/pull/24232#pullrequestreview-1236220931 * SystemLayerImplSelect: reword comment: early firing timers can happen - the fix prevents them in normal libev case - however the caller MUST NOT rely on timers *never* firing a bit early --- src/system/SystemLayerImplSelect.cpp | 20 +++++++++++++++++++- src/system/SystemLayerImplSelect.h | 2 ++ src/system/WakeEvent.cpp | 4 ++-- src/system/WakeEvent.h | 4 ++-- 4 files changed, 25 insertions(+), 5 deletions(-) diff --git a/src/system/SystemLayerImplSelect.cpp b/src/system/SystemLayerImplSelect.cpp index 9eb0b2ece2864b..7ae25a0a59d134 100644 --- a/src/system/SystemLayerImplSelect.cpp +++ b/src/system/SystemLayerImplSelect.cpp @@ -66,8 +66,10 @@ CHIP_ERROR LayerImplSelect::Init() mHandleSelectThread = PTHREAD_NULL; #endif // CHIP_SYSTEM_CONFIG_POSIX_LOCKING +#if !CHIP_SYSTEM_CONFIG_USE_LIBEV // Create an event to allow an arbitrary thread to wake the thread in the select loop. ReturnErrorOnFailure(mWakeEvent.Open(*this)); +#endif // !CHIP_SYSTEM_CONFIG_USE_LIBEV VerifyOrReturnError(mLayerState.SetInitialized(), CHIP_ERROR_INCORRECT_STATE); return CHIP_NO_ERROR; @@ -113,13 +115,18 @@ void LayerImplSelect::Shutdown() mTimerPool.ReleaseAll(); #endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV +#if !CHIP_SYSTEM_CONFIG_USE_LIBEV mWakeEvent.Close(*this); +#endif // !CHIP_SYSTEM_CONFIG_USE_LIBEV mLayerState.ResetFromShuttingDown(); // Return to uninitialized state to permit re-initialization. } void LayerImplSelect::Signal() { +#if CHIP_SYSTEM_CONFIG_USE_LIBEV + ChipLogError(DeviceLayer, "Signal() should not be called in CHIP_SYSTEM_CONFIG_USE_LIBEV builds (might be ok in tests)"); +#else /* * Wake up the I/O thread by writing a single byte to the wake pipe. * @@ -143,6 +150,7 @@ void LayerImplSelect::Signal() ChipLogError(chipSystemLayer, "System wake event notify failed: %" CHIP_ERROR_FORMAT, status.Format()); } +#endif // !CHIP_SYSTEM_CONFIG_USE_LIBEV } CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallback onComplete, void * appState) @@ -184,7 +192,14 @@ CHIP_ERROR LayerImplSelect::StartTimer(Clock::Timeout delay, TimerCompleteCallba ev_timer_init(&timer->mLibEvTimer, &LayerImplSelect::HandleLibEvTimer, 1, 0); timer->mLibEvTimer.data = timer; auto t = Clock::Milliseconds64(delay).count(); - ev_timer_set(&timer->mLibEvTimer, static_cast(t) / 1E3, 0.); + // Note: libev uses the time when events started processing as the "now" reference for relative timers, + // for efficiency reasons. This point in time is represented by ev_now(). + // The real time is represented by ev_time(). + // Without correction, this leads to timers firing a bit too early relative to the time StartTimer() + // is called. So the relative value passed to ev_timer_set() is adjusted (increased) here. + // Note: Still, slightly early (and of course, late) firing timers are something the caller MUST be prepared for, + // because edge cases like system clock adjustments may cause them even with the correction applied here. + ev_timer_set(&timer->mLibEvTimer, (static_cast(t) / 1E3) + ev_time() - ev_now(mLibEvLoopP), 0.); (void) mTimerList.Add(timer); ev_timer_start(mLibEvLoopP, &timer->mLibEvTimer); return CHIP_NO_ERROR; @@ -269,7 +284,10 @@ void LayerImplSelect::CancelTimer(TimerCompleteCallback onComplete, void * appSt #endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV mTimerPool.Release(timer); +#if !CHIP_SYSTEM_CONFIG_USE_LIBEV + // LIBEV has no I/O wakeup thread, so must not call Signal() Signal(); +#endif } CHIP_ERROR LayerImplSelect::ScheduleWork(TimerCompleteCallback onComplete, void * appState) diff --git a/src/system/SystemLayerImplSelect.h b/src/system/SystemLayerImplSelect.h index c835b8a76720c5..361feef991460f 100644 --- a/src/system/SystemLayerImplSelect.h +++ b/src/system/SystemLayerImplSelect.h @@ -146,7 +146,9 @@ class LayerImplSelect : public LayerSocketsLoop int mSelectResult; ObjectLifeCycle mLayerState; +#if !CHIP_SYSTEM_CONFIG_USE_LIBEV WakeEvent mWakeEvent; +#endif #if CHIP_SYSTEM_CONFIG_POSIX_LOCKING std::atomic mHandleSelectThread; diff --git a/src/system/WakeEvent.cpp b/src/system/WakeEvent.cpp index 4b5d7c2a9ad00f..12ed9231e4a2de 100644 --- a/src/system/WakeEvent.cpp +++ b/src/system/WakeEvent.cpp @@ -23,7 +23,7 @@ #include -#if CHIP_SYSTEM_CONFIG_USE_SOCKETS +#if CHIP_SYSTEM_CONFIG_USE_SOCKETS && !CHIP_SYSTEM_CONFIG_USE_LIBEV #include @@ -207,4 +207,4 @@ CHIP_ERROR WakeEvent::Notify() const } // namespace System } // namespace chip -#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS +#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS && !CHIP_SYSTEM_CONFIG_USE_LIBEV diff --git a/src/system/WakeEvent.h b/src/system/WakeEvent.h index b979c67ab17752..d0c603952c21b7 100644 --- a/src/system/WakeEvent.h +++ b/src/system/WakeEvent.h @@ -25,7 +25,7 @@ // Include configuration headers #include -#if CHIP_SYSTEM_CONFIG_USE_SOCKETS +#if CHIP_SYSTEM_CONFIG_USE_SOCKETS && !CHIP_SYSTEM_CONFIG_USE_LIBEV #include #include @@ -67,4 +67,4 @@ class WakeEvent } // namespace System } // namespace chip -#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS +#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS && !CHIP_SYSTEM_CONFIG_USE_LIBEV