From 5d48fdd605d483de20f61c942bc3ea463ad1f6b9 Mon Sep 17 00:00:00 2001 From: Gerard Marull-Paretas Date: Tue, 21 Dec 2021 10:32:02 +0100 Subject: [PATCH 1/5] sys: time_units: add missing include The header can't be fully used in standalone mode: toolchain.h has to be included first, otherwise the ALWAYS_INLINE attribute is not defined. Headers that can be directly included and are not self-contained should be considered a bad practice. Signed-off-by: Gerard Marull-Paretas --- include/sys/time_units.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/sys/time_units.h b/include/sys/time_units.h index 0963a6ba4275b..d1983b391f803 100644 --- a/include/sys/time_units.h +++ b/include/sys/time_units.h @@ -7,6 +7,9 @@ #ifndef ZEPHYR_INCLUDE_TIME_UNITS_H_ #define ZEPHYR_INCLUDE_TIME_UNITS_H_ +#include +#include + #ifdef __cplusplus extern "C" { #endif From ca2bdbfadaf42f27c2f23988656f3e9e5cbaf4a9 Mon Sep 17 00:00:00 2001 From: Krzysztof Chruscinski Date: Fri, 14 Jan 2022 09:25:37 +0100 Subject: [PATCH 2/5] lib: posix: clock: Prevent early overflows Algorithm was converting uptime to nanoseconds which can easily lead to overflows. Changed algorithm to use milliseconds and nanoseconds for remainder only. Signed-off-by: Krzysztof Chruscinski --- lib/posix/clock.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/posix/clock.c b/lib/posix/clock.c index c36f2401cc4c5..2bc8d052c1f5a 100644 --- a/lib/posix/clock.c +++ b/lib/posix/clock.c @@ -27,7 +27,6 @@ static struct k_spinlock rt_clock_base_lock; */ int z_impl_clock_gettime(clockid_t clock_id, struct timespec *ts) { - uint64_t elapsed_nsecs; struct timespec base; k_spinlock_key_t key; @@ -48,9 +47,13 @@ int z_impl_clock_gettime(clockid_t clock_id, struct timespec *ts) return -1; } - elapsed_nsecs = k_ticks_to_ns_floor64(k_uptime_ticks()); - ts->tv_sec = (int32_t) (elapsed_nsecs / NSEC_PER_SEC); - ts->tv_nsec = (int32_t) (elapsed_nsecs % NSEC_PER_SEC); + uint64_t ticks = k_uptime_ticks(); + uint64_t elapsed_secs = k_ticks_to_ms_floor64(ticks) / MSEC_PER_SEC; + uint64_t nremainder = ticks - k_ms_to_ticks_floor64(MSEC_PER_SEC * elapsed_secs); + + ts->tv_sec = (time_t) elapsed_secs; + /* For ns 32 bit conversion can be used since its smaller than 1sec. */ + ts->tv_nsec = (int32_t) k_ticks_to_ns_floor32(nremainder); ts->tv_sec += base.tv_sec; ts->tv_nsec += base.tv_nsec; From 856cefe2a32c579ea070f968e0ce11309e821e5a Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Fri, 9 Dec 2022 07:12:29 -0500 Subject: [PATCH 3/5] tests: time_units: check for overflow in z_tmcvt intermediate Prior to #41602, due to the ordering of operations (first mul, then div), an intermediate value would overflow, resulting in a time non-linearity. This test ensures that time rolls-over properly. Signed-off-by: Chris Friedt (cherry picked from commit 74c9c0e7a3dc1dc8dfbc7a16f989fc5389a5fd4f) --- tests/unit/time_units/CMakeLists.txt | 6 +++ tests/unit/time_units/main.c | 15 +++++++ tests/unit/time_units/overflow.c | 59 ++++++++++++++++++++++++++++ tests/unit/time_units/prj.conf | 1 + tests/unit/time_units/testcase.yaml | 5 +++ 5 files changed, 86 insertions(+) create mode 100644 tests/unit/time_units/CMakeLists.txt create mode 100644 tests/unit/time_units/main.c create mode 100644 tests/unit/time_units/overflow.c create mode 100644 tests/unit/time_units/prj.conf create mode 100644 tests/unit/time_units/testcase.yaml diff --git a/tests/unit/time_units/CMakeLists.txt b/tests/unit/time_units/CMakeLists.txt new file mode 100644 index 0000000000000..34534ebca59fe --- /dev/null +++ b/tests/unit/time_units/CMakeLists.txt @@ -0,0 +1,6 @@ +# Copyright 2022 Meta +# SPDX-License-Identifier: Apache-2.0 + +project(time_units) +set(SOURCES main.c overflow.c) +find_package(ZephyrUnittest REQUIRED HINTS $ENV{ZEPHYR_BASE}) diff --git a/tests/unit/time_units/main.c b/tests/unit/time_units/main.c new file mode 100644 index 0000000000000..51ea1f620c6c8 --- /dev/null +++ b/tests/unit/time_units/main.c @@ -0,0 +1,15 @@ +/* + * Copyright 2022 Meta + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +extern void test_z_tmcvt_for_overflow(void); + +void test_main(void) +{ + ztest_test_suite(test_time_units, ztest_unit_test(test_z_tmcvt_for_overflow)); + ztest_run_test_suite(test_time_units); +} diff --git a/tests/unit/time_units/overflow.c b/tests/unit/time_units/overflow.c new file mode 100644 index 0000000000000..ad58db7ca0947 --- /dev/null +++ b/tests/unit/time_units/overflow.c @@ -0,0 +1,59 @@ +/* + * Copyright 2022 Meta + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include +#include +#include +#include + +#include +#include + +/** + * @brief Test @ref z_tmcvt for robustness against intermediate value overflow. + * + * With input + * ``` + * [t0, t1, t2] = [ + * UINT64_MAX / to_hz - 1, + * UINT64_MAX / to_hz, + * UINT64_MAX / to_hz + 1, + * ] + * ``` + * + * passed through @ref z_tmcvt, we expect a linear sequence: + * ``` + * [ + * 562949953369140, + * 562949953399658, + * 562949953430175, + * ] + * ``` + * + * If an overflow occurs, we see something like the following: + * ``` + * [ + * 562949953369140, + * 562949953399658, + * 8863, + * ] + * ``` + */ +void test_z_tmcvt_for_overflow(void) +{ + const uint32_t from_hz = 32768UL; + const uint32_t to_hz = 1000000000UL; + + zassert_equal(562949953369140ULL, + z_tmcvt(UINT64_MAX / to_hz - 1, from_hz, to_hz, true, false, false, false), + NULL); + zassert_equal(562949953399658ULL, + z_tmcvt(UINT64_MAX / to_hz, from_hz, to_hz, true, false, false, false), + NULL); + zassert_equal(562949953430175ULL, + z_tmcvt(UINT64_MAX / to_hz + 1, from_hz, to_hz, true, false, false, false), + NULL); +} diff --git a/tests/unit/time_units/prj.conf b/tests/unit/time_units/prj.conf new file mode 100644 index 0000000000000..9467c2926896d --- /dev/null +++ b/tests/unit/time_units/prj.conf @@ -0,0 +1 @@ +CONFIG_ZTEST=y diff --git a/tests/unit/time_units/testcase.yaml b/tests/unit/time_units/testcase.yaml new file mode 100644 index 0000000000000..4624ce7143b8c --- /dev/null +++ b/tests/unit/time_units/testcase.yaml @@ -0,0 +1,5 @@ +common: + tags: time_units + type: unit +tests: + utilities.time_units.z_tmcvt: {} From 8cd40149e91f25a22c513953c13ebfcc1b0a0bb6 Mon Sep 17 00:00:00 2001 From: Krzysztof Chruscinski Date: Wed, 12 Jan 2022 10:35:38 +0100 Subject: [PATCH 4/5] sys: time_units: Add Kconfig option for algorithm selection Add maximum timeout used for conversion to Kconfig. Option is used to determine which conversion algorithm to use: faster but overflowing earlier or slower without early overflow. Signed-off-by: Krzysztof Chruscinski (cherry picked from commit 50c7c7b1e4027b08a594f108df5d62bce1fc229f) --- include/sys/time_units.h | 27 +++++++++++++++++++++++++- kernel/Kconfig | 11 +++++++++++ subsys/testsuite/ztest/include/ztest.h | 1 + 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/include/sys/time_units.h b/include/sys/time_units.h index d1983b391f803..bbea29c1359b2 100644 --- a/include/sys/time_units.h +++ b/include/sys/time_units.h @@ -59,6 +59,21 @@ static TIME_CONSTEXPR inline int sys_clock_hw_cycles_per_sec(void) #endif } +/** @internal + * Macro determines if fast conversion algorithm can be used. It checks if + * maximum timeout represented in source frequency domain and multiplied by + * target frequency fits in 64 bits. + * + * @param from_hz Source frequency. + * @param to_hz Target frequency. + * + * @retval true Use faster algorithm. + * @retval false Use algorithm preventing overflow of intermediate value. + */ +#define Z_TMCVT_USE_FAST_ALGO(from_hz, to_hz) \ + ((ceiling_fraction(CONFIG_SYS_CLOCK_MAX_TIMEOUT_DAYS * 24ULL * 3600ULL * from_hz, \ + UINT32_MAX) * to_hz) <= UINT32_MAX) + /* Time converter generator gadget. Selects from one of three * conversion algorithms: ones that take advantage when the * frequencies are an integer ratio (in either direction), or a full @@ -126,8 +141,18 @@ static TIME_CONSTEXPR ALWAYS_INLINE uint64_t z_tmcvt(uint64_t t, uint32_t from_h } else { if (result32) { return (uint32_t)((t * to_hz + off) / from_hz); + } else if (const_hz && Z_TMCVT_USE_FAST_ALGO(from_hz, to_hz)) { + /* Faster algorithm but source is first multiplied by target frequency + * and it can overflow even though final result would not overflow. + * Kconfig option shall prevent use of this algorithm when there is a + * risk of overflow. + */ + return ((t * to_hz + off) / from_hz); } else { - return (t * to_hz + off) / from_hz; + /* Slower algorithm but input is first divided before being multiplied + * which prevents overflow of intermediate value. + */ + return (t / from_hz) * to_hz + ((t % from_hz) * to_hz + off) / from_hz; } } } diff --git a/kernel/Kconfig b/kernel/Kconfig index c2575c77a50eb..8e5b2a308b4cd 100644 --- a/kernel/Kconfig +++ b/kernel/Kconfig @@ -613,6 +613,17 @@ config TIMEOUT_64BIT availability of absolute timeout values (which require the extra precision). +config SYS_CLOCK_MAX_TIMEOUT_DAYS + int "Max timeout (in days) used in conversions" + default 365 + help + Value is used in the time conversion static inline function to determine + at compile time which algorithm to use. One algorithm is faster, takes + less code but may overflow if multiplication of source and target + frequency exceeds 64 bits. Second algorithm prevents that. Faster + algorithm is selected for conversion if maximum timeout represented in + source frequency domain multiplied by target frequency fits in 64 bits. + config XIP bool "Execute in place" help diff --git a/subsys/testsuite/ztest/include/ztest.h b/subsys/testsuite/ztest/include/ztest.h index c0b5fcc92cfee..6aa4306948b2e 100644 --- a/subsys/testsuite/ztest/include/ztest.h +++ b/subsys/testsuite/ztest/include/ztest.h @@ -38,6 +38,7 @@ #define CONFIG_MP_NUM_CPUS 1 #define CONFIG_SYS_CLOCK_TICKS_PER_SEC 100 #define CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC 10000000 +#define CONFIG_SYS_CLOCK_MAX_TIMEOUT_DAYS 365 #define ARCH_STACK_PTR_ALIGN 8 /* FIXME: Properly integrate with Zephyr's arch specific code */ #define CONFIG_X86 1 From b952a07a3eb6012fff77983a41f802b10473a91c Mon Sep 17 00:00:00 2001 From: Chris Friedt Date: Mon, 12 Dec 2022 00:03:08 -0500 Subject: [PATCH 5/5] posix: clock: fix seconds calculation The previous method used to calculate seconds in `clock_gettime()` seemed to have an inaccuracy that grew with time causing the seconds to be off by an order of magnitude when ticks would roll over. This change fixes the method used to calculate seconds. Signed-off-by: Chris Friedt --- lib/posix/clock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/posix/clock.c b/lib/posix/clock.c index 2bc8d052c1f5a..eea05d8b2c3e7 100644 --- a/lib/posix/clock.c +++ b/lib/posix/clock.c @@ -48,8 +48,8 @@ int z_impl_clock_gettime(clockid_t clock_id, struct timespec *ts) } uint64_t ticks = k_uptime_ticks(); - uint64_t elapsed_secs = k_ticks_to_ms_floor64(ticks) / MSEC_PER_SEC; - uint64_t nremainder = ticks - k_ms_to_ticks_floor64(MSEC_PER_SEC * elapsed_secs); + uint64_t elapsed_secs = ticks / CONFIG_SYS_CLOCK_TICKS_PER_SEC; + uint64_t nremainder = ticks - elapsed_secs * CONFIG_SYS_CLOCK_TICKS_PER_SEC; ts->tv_sec = (time_t) elapsed_secs; /* For ns 32 bit conversion can be used since its smaller than 1sec. */