From 52e2f831851e8de8aa29e00938a0ef8f6ae8e8c7 Mon Sep 17 00:00:00 2001 From: Nicolas Pitre Date: Thu, 6 Jul 2023 13:56:01 -0400 Subject: [PATCH] kernel/timeout: introduce the timepoint API This is meant as a substitute for sys_clock_timeout_end_calc() Current sys_clock_timeout_end_calc() usage opens up many bug possibilities due to the actual timeout evaluation's open-coded nature. Issue ##50611 is one example. - Some users store the returned value in a signed variable, others in an unsigned one, making the comparison with UINT64_MAX (corresponding to K_FOREVER) wrong in the signed case. - Some users compute the difference and store that in a signed variable to compare against 0 which still doesn't work with K_FOREVER. And when this difference is used as a timeout argument then the K_FOREVER nature of the timeout is lost. - Some users complexify their code by special-casing K_NO_WAIT and K_FOREVER inline which is bad for both code readability and binary size. Let's introduce a better abstraction to deal with absolute timepoints with an opaque type to be used with a well-defined API. The word "timeout" was avoided in the naming on purpose as the timeout namespace is quite crowded already and it is preferable to make a distinction between relative time periods (timeouts) and absolute time values (timepoints). A few stacks are also adjusted as they were too tight on X86. Signed-off-by: Nicolas Pitre --- doc/kernel/services/timing/clocks.rst | 36 +++++++------ include/zephyr/sys_clock.h | 73 ++++++++++++++++++++++++++- kernel/timeout.c | 39 +++++++++----- scripts/checkpatch/typedefsfile | 1 + subsys/net/ip/Kconfig | 1 + subsys/net/ip/Kconfig.mgmt | 1 + subsys/testsuite/ztest/Kconfig | 1 + 7 files changed, 122 insertions(+), 30 deletions(-) diff --git a/doc/kernel/services/timing/clocks.rst b/doc/kernel/services/timing/clocks.rst index 3a016b3f35..0517d601f2 100644 --- a/doc/kernel/services/timing/clocks.rst +++ b/doc/kernel/services/timing/clocks.rst @@ -324,39 +324,43 @@ code. For example, consider this design: } This code requires that the timeout value be inspected, which is no -longer possible. For situations like this, the new API provides an -internal :c:func:`sys_clock_timeout_end_calc` routine that converts an -arbitrary timeout to the uptime value in ticks at which it will -expire. So such a loop might look like: +longer possible. For situations like this, the new API provides the +internal :c:func:`sys_timepoint_calc` and :c:func:`sys_timepoint_timeout` routines +that converts an arbitrary timeout to and from a timepoint value based on +an uptime tick at which it will expire. So such a loop might look like: .. code-block:: c - void my_wait_for_event(struct my_subsys *obj, k_timeout_t timeout_in_ms) + void my_wait_for_event(struct my_subsys *obj, k_timeout_t timeout) { /* Compute the end time from the timeout */ - uint64_t end = sys_clock_timeout_end_calc(timeout_in_ms); + k_timepoint_t end = sys_timepoint_calc(timeout); - while (end > k_uptime_ticks()) { + do { if (is_event_complete(obj)) { return; } + /* Update timeout with remaining time */ + timeout = sys_timepoint_timeout(end); + /* Wait for notification of state change */ - k_sem_take(obj->sem, timeout_in_ms); - } + k_sem_take(obj->sem, timeout); + } while (!K_TIMEOUT_EQ(timeout, K_NO_WAIT)); } -Note that :c:func:`sys_clock_timeout_end_calc` returns values in units of -ticks, to prevent conversion aliasing, is always presented at 64 bit -uptime precision to prevent rollover bugs, handles special -:c:macro:`K_FOREVER` naturally (as ``UINT64_MAX``), and works -identically for absolute timeouts as well as conventional ones. +Note that :c:func:`sys_timepoint_calc` accepts special values :c:macro:`K_FOREVER` +and :c:macro:`K_NO_WAIT`, and works identically for absolute timeouts as well +as conventional ones. Conversely, :c:func:`sys_timepoint_timeout` may return +:c:macro:`K_FOREVER` or :c:macro:`K_NO_WAIT` if those were used to create +the timepoint, the later also being returned if the timepoint is now in the +past. For simple cases, :c:func:`sys_timepoint_expired` can be used as well. -But some care is still required for subsystems that use it. Note that +But some care is still required for subsystems that use those. Note that delta timeouts need to be interpreted relative to a "current time", and obviously that time is the time of the call to -:c:func:`sys_clock_timeout_end_calc`. But the user expects that the time is +:c:func:`sys_timepoint_calc`. But the user expects that the time is the time they passed the timeout to you. Care must be taken to call this function just once, as synchronously as possible to the timeout creation in user code. It should not be used on a "stored" timeout diff --git a/include/zephyr/sys_clock.h b/include/zephyr/sys_clock.h index 52c88b170e..66c767a24e 100644 --- a/include/zephyr/sys_clock.h +++ b/include/zephyr/sys_clock.h @@ -190,7 +190,78 @@ int64_t sys_clock_tick_get(void); #define sys_clock_tick_get_32() (0) #endif -uint64_t sys_clock_timeout_end_calc(k_timeout_t timeout); +/** + * @brief Kernel timepoint type + * + * Absolute timepoints are stored in this opaque type. + * It is best not to inspect its content directly. + * + * @see sys_timepoint_calc() + * @see sys_timepoint_timeout() + * @see sys_timepoint_expired() + */ +typedef struct { uint64_t tick; } k_timepoint_t; + +/** + * @brief Calculate a timepoint value + * + * Returns a timepoint corresponding to the expiration (relative to an + * unlocked "now"!) of a timeout object. When used correctly, this should + * be called once, synchronously with the user passing a new timeout value. + * It should not be used iteratively to adjust a timeout (see + * `sys_timepoint_timeout()` for that purpose). + * + * @param timeout Timeout value relative to current time (may also be + * `K_FOREVER` or `K_NO_WAIT`). + * @retval Timepoint value corresponding to given timeout + * + * @see sys_timepoint_timeout() + * @see sys_timepoint_expired() + */ +k_timepoint_t sys_timepoint_calc(k_timeout_t timeout); + +/** + * @brief Remaining time to given timepoint + * + * Returns the timeout interval between current time and provided timepoint. + * If the timepoint is now in the past or if it was created with `K_NO_WAIT` + * then `K_NO_WAIT` is returned. If it was created with `K_FOREVER` then + * `K_FOREVER` is returned. + * + * @param timepoint Timepoint for which a timeout value is wanted. + * @retval Corresponding timeout value. + * + * @see sys_timepoint_calc() + */ +k_timeout_t sys_timepoint_timeout(k_timepoint_t timepoint); + +/** + * @brief Indicates if timepoint is expired + * + * @param timepoint Timepoint to evaluate + * @retval true if the timepoint is in the past, false otherwise + * + * @see sys_timepoint_calc() + */ +static inline bool sys_timepoint_expired(k_timepoint_t timepoint) +{ + return K_TIMEOUT_EQ(sys_timepoint_timeout(timepoint), Z_TIMEOUT_NO_WAIT); +} + +/** + * @brief Provided for backward compatibility. + * + * This is deprecated. Consider `sys_timepoint_calc()` instead. + * + * @see sys_timepoint_calc() + */ +__deprecated +static inline uint64_t sys_clock_timeout_end_calc(k_timeout_t timeout) +{ + k_timepoint_t tp = sys_timepoint_calc(timeout); + + return tp.tick; +} #ifdef __cplusplus } diff --git a/kernel/timeout.c b/kernel/timeout.c index df8c2aaede..cfd2b48ac8 100644 --- a/kernel/timeout.c +++ b/kernel/timeout.c @@ -289,28 +289,41 @@ static inline int64_t z_vrfy_k_uptime_ticks(void) #include #endif -/* Returns the uptime expiration (relative to an unlocked "now"!) of a - * timeout object. When used correctly, this should be called once, - * synchronously with the user passing a new timeout value. It should - * not be used iteratively to adjust a timeout. - */ -uint64_t sys_clock_timeout_end_calc(k_timeout_t timeout) +k_timepoint_t sys_timepoint_calc(k_timeout_t timeout) { - k_ticks_t dt; + k_timepoint_t timepoint; if (K_TIMEOUT_EQ(timeout, K_FOREVER)) { - return UINT64_MAX; + timepoint.tick = UINT64_MAX; } else if (K_TIMEOUT_EQ(timeout, K_NO_WAIT)) { - return sys_clock_tick_get(); + timepoint.tick = 0; } else { - - dt = timeout.ticks; + k_ticks_t dt = timeout.ticks; if (IS_ENABLED(CONFIG_TIMEOUT_64BIT) && Z_TICK_ABS(dt) >= 0) { - return Z_TICK_ABS(dt); + timepoint.tick = Z_TICK_ABS(dt); + } else { + timepoint.tick = sys_clock_tick_get() + MAX(1, dt); } - return sys_clock_tick_get() + MAX(1, dt); } + + return timepoint; +} + +k_timeout_t sys_timepoint_timeout(k_timepoint_t timepoint) +{ + uint64_t now, remaining; + + if (timepoint.tick == UINT64_MAX) { + return K_FOREVER; + } + if (timepoint.tick == 0) { + return K_NO_WAIT; + } + + now = sys_clock_tick_get(); + remaining = (timepoint.tick > now) ? (timepoint.tick - now) : 0; + return K_TICKS(remaining); } #ifdef CONFIG_ZTEST diff --git a/scripts/checkpatch/typedefsfile b/scripts/checkpatch/typedefsfile index ff7ab3cb93..22cb81ef72 100644 --- a/scripts/checkpatch/typedefsfile +++ b/scripts/checkpatch/typedefsfile @@ -1,5 +1,6 @@ _cpu_arch_t k_mem_partition_attr_t +k_timepoint_t mbedtls_pk_context z_arch_esf_t pinctrl_soc_pin_t diff --git a/subsys/net/ip/Kconfig b/subsys/net/ip/Kconfig index e5f096e594..3b9947eb50 100644 --- a/subsys/net/ip/Kconfig +++ b/subsys/net/ip/Kconfig @@ -508,6 +508,7 @@ config NET_TCP_PKT_ALLOC_TIMEOUT config NET_TCP_WORKQ_STACK_SIZE int "TCP work queue thread stack size" + default 1200 if X86 default 1024 depends on NET_TCP help diff --git a/subsys/net/ip/Kconfig.mgmt b/subsys/net/ip/Kconfig.mgmt index bb5c9ccc18..2ecfc9d028 100644 --- a/subsys/net/ip/Kconfig.mgmt +++ b/subsys/net/ip/Kconfig.mgmt @@ -22,6 +22,7 @@ if NET_MGMT_EVENT config NET_MGMT_EVENT_STACK_SIZE int "Stack size for the inner thread handling event callbacks" default 2048 if COVERAGE_GCOV + default 840 if X86 default 800 if THREAD_LOCAL_STORAGE default 768 help diff --git a/subsys/testsuite/ztest/Kconfig b/subsys/testsuite/ztest/Kconfig index 7c5803147a..d48961cc84 100644 --- a/subsys/testsuite/ztest/Kconfig +++ b/subsys/testsuite/ztest/Kconfig @@ -19,6 +19,7 @@ config ZTEST_NEW_API config ZTEST_STACK_SIZE int "Test function thread stack size" default 2048 if COVERAGE_GCOV + default 2048 if X86 default 1024 config ZTEST_TEST_DELAY_MS