Skip to content

Commit 4b3c37e

Browse files
tcarmelveilleuxtennessee-googlerestyled-commits
authored
Add strong guarantee on monotonicity of system clock (#31885)
* Add strong guarantee on monotonicity of system clock - It was observed some platforms have decrementing values from `SystemClock::GetMonotonicMilliseconds64()` implementation which can violate the required invariant that they NEVER go back, since multiple deadlines are computed from this clock, which could then never hit. This was observed to break in the field in subtle ways after ~48 days (2^32 milliseconds). - This change introduces a VerifyOrDie to allow crash/restart if the invariant is broken, since that leaves the stack in a possibly wedged state. - The new invariant is constructed to be reentrant-safe to maintain the invariant, in a way that it will eventually be caught, even if there are many threads/cores calling `GetMonotonicTimestamp()`. This was done in a lock-free manner, and was added since the public API of SystemLayer is not guaranteed to only be called from Matter thread context. Issue #31875 Testing done: - All unit tests still pass. - All integration tests still pass. - When `mockClock.AdvanceMonotonic(200_ms);` is replaced with `mockClock.AdvanceMonotonic(0xffff'ffff'ffff'ffff_ms);` in `TestSystemTimer.cpp::CheckOrder()`, the test crashes as expected due to detection of wraparound. * Fix review comments and move to intrinsics * Restyled by clang-format * Fix build and add warning * Fix build * Restyled by clang-format * Address review comment, add new define instead of linux && darwin * Restyled by clang-format --------- Co-authored-by: tennessee.carmelveilleux@gmail.com <tennessee@google.com> Co-authored-by: Restyled.io <commits@restyled.io>
1 parent 4c4d8bf commit 4b3c37e

File tree

2 files changed

+64
-8
lines changed

2 files changed

+64
-8
lines changed

src/system/SystemClock.cpp

+33
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,39 @@ ClockBase * gClockBase = &gClockImpl;
6161

6262
} // namespace Internal
6363

64+
Timestamp ClockBase::GetMonotonicTimestamp()
65+
{
66+
// Below implementation uses `__atomic_*` API which has wider support than
67+
// <atomic> on embedded platforms, so that embedded platforms can use
68+
// it by widening the #ifdefs later.
69+
#if CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK
70+
uint64_t prevTimestamp = __atomic_load_n(&mLastTimestamp, __ATOMIC_SEQ_CST);
71+
static_assert(sizeof(prevTimestamp) == sizeof(Timestamp), "Must have scalar match between timestamp and uint64_t for atomics.");
72+
73+
// Force a reorder barrier to prevent GetMonotonicMilliseconds64() from being
74+
// optimizer-called before prevTimestamp loading, so that newTimestamp acquisition happens-after
75+
// the prevTimestamp load.
76+
__atomic_signal_fence(__ATOMIC_SEQ_CST);
77+
#else
78+
uint64_t prevTimestamp = mLastTimestamp;
79+
#endif // CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK
80+
81+
Timestamp newTimestamp = GetMonotonicMilliseconds64();
82+
83+
// Need to guarantee the invariant that monotonic clock never goes backwards, which would break multiple system
84+
// assumptions which use these clocks.
85+
VerifyOrDie(newTimestamp.count() >= prevTimestamp);
86+
87+
#if CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK
88+
// newTimestamp guaranteed to never be < the last timestamp.
89+
__atomic_store_n(&mLastTimestamp, newTimestamp.count(), __ATOMIC_SEQ_CST);
90+
#else
91+
mLastTimestamp = newTimestamp.count();
92+
#endif // CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK
93+
94+
return newTimestamp;
95+
}
96+
6497
#if !CHIP_SYSTEM_CONFIG_PLATFORM_PROVIDES_TIME
6598

6699
#if CHIP_SYSTEM_CONFIG_USE_POSIX_TIME_FUNCTS

src/system/SystemClock.h

+31-8
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,14 @@
4343
#include <chrono>
4444
#include <stdint.h>
4545

46+
#if CHIP_DEVICE_LAYER_TARGET_DARWIN || CHIP_DEVICE_LAYER_TARGET_LINUX
47+
#define CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK 1
48+
#endif // CHIP_DEVICE_LAYER_TARGET_DARWIN || CHIP_DEVICE_LAYER_TARGET_LINUX
49+
50+
#ifndef CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK
51+
#define CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK 0
52+
#endif
53+
4654
namespace chip {
4755
namespace System {
4856

@@ -149,15 +157,20 @@ class ClockBase
149157
*
150158
* Although some platforms may choose to return a value that measures the time since boot for the
151159
* system, applications must *not* rely on this.
160+
*
161+
* WARNING: *** It is up to each platform to ensure that GetMonotonicTimestamp can be
162+
* called safely in a re-entrant way from multiple contexts if making use
163+
* of this method from the application, outside the Matter stack execution
164+
* serialization context. ***
152165
*/
153-
Timestamp GetMonotonicTimestamp() { return GetMonotonicMilliseconds64(); }
166+
virtual Timestamp GetMonotonicTimestamp();
154167

155168
/**
156-
* Returns a monotonic system time in units of microseconds.
169+
* Returns a monotonic system time in units of microseconds, from the platform.
157170
*
158171
* This function returns an elapsed time in microseconds since an arbitrary, platform-defined epoch.
159-
* The value returned is guaranteed to be ever-increasing (i.e. never wrapping or decreasing) between
160-
* reboots of the system. Additionally, the underlying time source is guaranteed to tick
172+
* The value returned MUST BE guaranteed to be ever-increasing (i.e. never wrapping or decreasing) until
173+
* reboot of the system. Additionally, the underlying time source is guaranteed to tick
161174
* continuously during any system sleep modes that do not entail a restart upon wake.
162175
*
163176
* Although some platforms may choose to return a value that measures the time since boot for the
@@ -176,11 +189,11 @@ class ClockBase
176189
virtual Microseconds64 GetMonotonicMicroseconds64() = 0;
177190

178191
/**
179-
* Returns a monotonic system time in units of milliseconds.
192+
* Returns a monotonic system time in units of microseconds, from the platform.
180193
*
181194
* This function returns an elapsed time in milliseconds since an arbitrary, platform-defined epoch.
182-
* The value returned is guaranteed to be ever-increasing (i.e. never wrapping or decreasing) between
183-
* reboots of the system. Additionally, the underlying time source is guaranteed to tick
195+
* The value returned MUST BE guaranteed to be ever-increasing (i.e. never wrapping or decreasing) until
196+
* reboot of the system. Additionally, the underlying time source is guaranteed to tick
184197
* continuously during any system sleep modes that do not entail a restart upon wake.
185198
*
186199
* Although some platforms may choose to return a value that measures the time since boot for the
@@ -288,6 +301,9 @@ class ClockBase
288301
* current time.
289302
*/
290303
virtual CHIP_ERROR SetClock_RealTime(Microseconds64 aNewCurTime) = 0;
304+
305+
protected:
306+
uint64_t mLastTimestamp = 0;
291307
};
292308

293309
// Currently we have a single implementation class, ClockImpl, whose members are implemented in build-specific files.
@@ -334,7 +350,14 @@ class MockClock : public ClockImpl
334350
return CHIP_NO_ERROR;
335351
}
336352

337-
void SetMonotonic(Milliseconds64 timestamp) { mSystemTime = timestamp; }
353+
void SetMonotonic(Milliseconds64 timestamp)
354+
{
355+
mSystemTime = timestamp;
356+
#if CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK
357+
__atomic_store_n(&mLastTimestamp, timestamp.count(), __ATOMIC_SEQ_CST);
358+
#endif // CHIP_DEVICE_LAYER_USE_ATOMICS_FOR_CLOCK
359+
}
360+
338361
void AdvanceMonotonic(Milliseconds64 increment) { mSystemTime += increment; }
339362
void AdvanceRealTime(Milliseconds64 increment) { mRealTime += increment; }
340363

0 commit comments

Comments
 (0)