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 <npitre@baylibre.com>

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
+longer possible.  For situations like this, the new API provides the
-internal :c:func:`sys_clock_timeout_end_calc` routine that converts an
+internal :c:func:`sys_timepoint_calc` and :c:func:`sys_timepoint_timeout` routines
-arbitrary timeout to the uptime value in ticks at which it will
+that converts an arbitrary timeout to and from a timepoint value based on
-expire.  So such a loop might look like:
+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
+Note that :c:func:`sys_timepoint_calc` accepts special values :c:macro:`K_FOREVER`
-ticks, to prevent conversion aliasing, is always presented at 64 bit
+and :c:macro:`K_NO_WAIT`, and works identically for absolute timeouts as well
-uptime precision to prevent rollover bugs, handles special
+as conventional ones. Conversely, :c:func:`sys_timepoint_timeout` may return
-:c:macro:`K_FOREVER` naturally (as ``UINT64_MAX``), and works
+:c:macro:`K_FOREVER` or :c:macro:`K_NO_WAIT` if those were used to create
-identically for absolute timeouts as well as conventional ones.
+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

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
 }

kernel/timeout.c

@@ -289,28 +289,41 @@ static inline int64_t z_vrfy_k_uptime_ticks(void)
 #include <syscalls/k_uptime_ticks_mrsh.c>
 #endif
 
-/* Returns the uptime expiration (relative to an unlocked "now"!) of a
+k_timepoint_t sys_timepoint_calc(k_timeout_t timeout)
- * 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_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 {
-
+		k_ticks_t dt = timeout.ticks;
-		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

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

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

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

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