net: timeout: refactor to fix multiple problems

The net_timeout structure is documented to exist because of behavior
that is no longer true, i.e. that `k_delayed_work_submit()` supports
only delays up to INT32_MAX milliseconds.  Nonetheless, use of 32-bit
timestamps within the work handlers mean the restriction is still
present.

This infrastructure is currently used for two timers with long
durations:
* address for IPv6 addresses
* prefix for IPv6 prefixes

The handling of rollover was subtly different between these: address
wraps reset the start time while prefix wraps did not.

The calculation of remaining time in ipv6_nbr was incorrect when the
original requested time in seconds was a multiple of
NET_TIMEOUT_MAX_VALUE: the remainder value would be zero while the
wrap counter was positive, causing the calculation to indicate no time
remained.

The maximum value was set to allow a 100 ms latency between elapse of
the deadline and assessment of a given timer, but detection of
rollover assumed that the captured time in the work handler was
precisely the expected deadline, which is unlikely to be true.  Use of
the shared system work queue also risks observed latency exceeding 100
ms.  These calculations could produce delays to next event that
exceeded the maximum delay, which introduced special cases.

Refactor so all operations that use this structure are encapsulated
into API that is documented and has a full-coverage unit test.  Switch
to the standard mechanism of detecting completed deadlines by
calculating the signed difference between the deadline and the current
time, which eliminates some special cases.

Uniformly rely on the scanning the set of timers to determine the next
deadline, rather than assuming that the most recent update is always
next.

Signed-off-by: Peter Bigot <[email protected]>

Author: pabigot

doc/reference/networking/net_timeout.rst

@@ -10,10 +10,50 @@ Network Timeout
 Overview
 ********
 
-The ``k_delayed_work`` API has a 1 ms accuracy for a timeout value,
-so the maximum timeout can be about 24 days. Some network timeouts
-are longer than this, so the net_timeout API provides a generic timeout
-mechanism that tracks such wraparounds and restarts the timeout as needed.
+Zephyr's network infrastructure mostly uses the millisecond-resolution uptime
+clock to track timeouts, with both deadlines and durations measured with
+32-bit unsigned values.  The 32-bit value rolls over at 49 days 17 hours 2 minutes
+47.296 seconds.
+
+Timeout processing is often affected by latency, so that the time at which the
+timeout is checked may be some time after it should have expired.  Handling
+this correctly without arbitrary expectations of maximum latency requires that
+the maximum delay that can be directly represented be a 31-bit non-negative
+number (``INT32_MAX``), which overflows at 24 days 20 hours 31 minutes 23.648
+seconds.
+
+Most network timeouts are shorter than the delay rollover, but a few protocols
+allow for delays that are represented as unsigned 32-bit values counting
+seconds, which corresponds to a 42-bit millisecond count.
+
+The net_timeout API provides a generic timeout mechanism to correctly track
+the remaining time for these extended-duration timeouts.
+
+Use
+***
+
+The simplest use of this API is:
+
+#. Configure a network timeout using :c:func:`net_timeout_set()`.
+#. Use :c:func:`net_timeout_evaluate()` to determine how long it is until the
+   timeout occurs.  Schedule a timeout to occur after this delay.
+#. When the timeout callback is invoked, use :c:func:`net_timeout_evaluate()`
+   again to determine whether the timeout has completed, or whether there is
+   additional time remaining.  If the latter, reschedule the callback.
+#. While the timeout is running, use :c:func:`net_timeout_remaining` to get
+   the number of seconds until the timeout expires.  This may be used to
+   explicitly update the timeout, which should be done by canceling any
+   pending callback and restarting from step 1 with the new timeout.
+
+The :c:struct:`net_timeout` contains a ``sys_snode_t`` that allows multiple
+timeout instances to be aggregated to share a single kernel timer element.
+The application must use :c:func:`net_timeout_evaluate()` on all instances to
+determine the next timeout event to occur.
+
+:c:func:`net_timeout_deadline()` may be used to reconstruct the full-precision
+deadline of the timeout.  This exists primarily for testing but may have use
+in some applications, as it does allow a millisecond-resolution calculation of
+remaining time.
 
 API Reference
 *************

include/net/net_timeout.h

@@ -6,6 +6,7 @@
 
 /*
  * Copyright (c) 2018 Intel Corporation
+ * Copyright (c) 2020 Nordic Semiconductor ASA
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -21,40 +22,137 @@
  */
 
 #include <string.h>
-#include <zephyr/types.h>
 #include <stdbool.h>
+#include <limits.h>
+#include <zephyr/types.h>
+#include <sys/slist.h>
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
-/** Let the max timeout be 100 ms lower because of
- * possible rounding in delayed work implementation.
+/** @brief Divisor used to support ms resolution timeouts.
+ *
+ * Because delays are processed in work queues which are not invoked
+ * synchronously with clock changes we need to be able to detect timeouts
+ * after they occur, which requires comparing "deadline" to "now" with enough
+ * "slop" to handle any observable latency due to "now" advancing past
+ * "deadline".
+ *
+ * The simplest solution is to use the native conversion of the well-defined
+ * 32-bit unsigned difference to a 32-bit signed difference, which caps the
+ * maximum delay at INT32_MAX.  This is compatible with the standard mechanism
+ * for detecting completion of deadlines that do not overflow their
+ * representation.
  */
-#define NET_TIMEOUT_MAX_VALUE ((uint32_t)(INT32_MAX - 100))
+#define NET_TIMEOUT_MAX_VALUE ((uint32_t)INT32_MAX)
 
-/** Generic struct for handling network timeouts */
+/** Generic struct for handling network timeouts.
+ *
+ * Except for the linking node, all access to state from these objects must go
+ * through the defined API.
+ */
 struct net_timeout {
-	/** Used to track timers */
+	/** Used to link multiple timeouts that share a common timer infrastructure.
+	 *
+	 * For examples a set of related timers may use a single delayed work
+	 * structure, which is always scheduled at the shortest time to a
+	 * timeout event.
+	 */
 	sys_snode_t node;
 
-	/** Address lifetime timer start time */
+	/* Time at which the timer was last set.
+	 *
+	 * This usually corresponds to the low 32 bits of k_uptime_get(). */
 	uint32_t timer_start;
 
-	/** Address lifetime timer timeout in milliseconds. Note that this
-	 * value is signed as k_delayed_work_submit() only supports signed
-	 * delay value.
+	/* Portion of remaining timeout that does not exceed
+	 * NET_TIMEOUT_MAX_VALUE.
+	 *
+	 * This value is updated in parallel with timer_start and wrap_counter
+	 * by net_timeout_evaluate().
 	 */
-	int32_t timer_timeout;
+	uint32_t timer_timeout;
 
-	/** Timer wrap count. Used if the timer timeout is larger than
-	 * about 24 days. The reason we need to track wrap arounds, is
-	 * that the timer timeout used in k_delayed_work_submit() is
-	 * 32-bit signed value and the resolution is 1ms.
+	/* Timer wrap count.
+	 *
+	 * This tracks multiples of NET_TIMEOUT_MAX_VALUE milliseconds that
+	 * have yet to pass.  It is also updated along with timer_start and
+	 * wrap_counter by net_timeout_evaluate().
 	 */
-	int32_t wrap_counter;
+	uint32_t wrap_counter;
 };
 
+/** @brief Configure a network timeout structure.
+ *
+ * @param timeout a pointer to the timeout state.
+ *
+ * @param lifetime the duration of the timeout in seconds.
+ *
+ * @param now the time at which the timeout started counting down, in
+ * milliseconds.  This is generally a captured value of k_uptime_get_32().
+ */
+void net_timeout_set(struct net_timeout *timeout,
+		     uint32_t lifetime,
+		     uint32_t now);
+
+/** @brief Return the 64-bit system time at which the timeout will complete.
+ *
+ * @note Correct behavior requires invocation of net_timeout_evaluate() at its
+ * specified intervals.
+ *
+ * @param timeout state a pointer to the timeout state, initialized by
+ * net_timeout_set() and maintained by net_timeout_evaluate().
+ *
+ * @param now the full-precision value of k_uptime_get() relative to which the
+ * deadline will be calculated.
+ *
+ * @return the value of k_uptime_get() at which the timeout will expire.
+ */
+int64_t net_timeout_deadline(const struct net_timeout *timeout,
+			     int64_t now);
+
+/** @brief Calculate the remaining time to the timeout in whole seconds.
+ *
+ * @note This function rounds the remaining time down, i.e. if the timeout
+ * will occur in 3500 milliseconds the value 3 will be returned.
+ *
+ * @note Correct behavior requires invocation of net_timeout_evaluate() at its
+ * specified intervals.
+ *
+ * @param timeout a pointer to the timeout state
+ *
+ * @param now the time relative to which the estimate of remaining time should
+ * be calculated.  This should be recently captured value from
+ * k_uptime_get_32().
+ *
+ * @retval 0 if the timeout has completed.
+ * @retval positive the remaining duration of the timeout, in seconds.
+ */
+uint32_t net_timeout_remaining(const struct net_timeout *timeout,
+			       uint32_t now);
+
+/** @brief Update state to reflect elapsed time and get new delay.
+ *
+ * This function must be invoked periodically to (1) apply the effect of
+ * elapsed time on what remains of a total delay that exceeded the maximum
+ * representable delay, and (2) determine that either the timeout has
+ * completed or that the infrastructure must wait a certain period before
+ * checking again for completion.
+ *
+ * @param timeout a pointer to the timeout state
+ *
+ * @param now the time relative to which the estimate of remaining time should
+ * be calculated.  This should be recently captured value from
+ * k_uptime_get_32().
+ *
+ * @retval 0 if the timeout has completed
+ * @retval positive the maximum delay until the state of this timeout should
+ * be re-evaluated, in milliseconds.
+ */
+uint32_t net_timeout_evaluate(struct net_timeout *timeout,
+			      uint32_t now);
+
 #ifdef __cplusplus
 }
 #endif

subsys/net/ip/CMakeLists.txt

@@ -9,6 +9,7 @@ zephyr_library_compile_definitions_ifdef(
 zephyr_library_sources(
   net_core.c
   net_if.c
+  net_timeout.c
   utils.c
   )
 

subsys/net/ip/ipv6_nbr.c

@@ -2122,26 +2122,9 @@ static inline void handle_prefix_onlink(struct net_pkt *pkt,
 
 #define TWO_HOURS (2 * 60 * 60)
 
-static uint32_t time_diff(uint32_t time1, uint32_t time2)
-{
-	return (uint32_t)abs((int32_t)time1 - (int32_t)time2);
-}
-
 static inline uint32_t remaining_lifetime(struct net_if_addr *ifaddr)
 {
-	uint64_t remaining;
-
-	if (ifaddr->lifetime.timer_timeout == 0) {
-		return 0;
-	}
-
-	remaining = (uint64_t)ifaddr->lifetime.timer_timeout +
-		(uint64_t)ifaddr->lifetime.wrap_counter *
-		(uint64_t)NET_TIMEOUT_MAX_VALUE -
-		(uint64_t)time_diff(k_uptime_get_32(),
-				 ifaddr->lifetime.timer_start);
-
-	return (uint32_t)(remaining / MSEC_PER_SEC);
+	return net_timeout_remaining(&ifaddr->lifetime, k_uptime_get_32());
 }
 
 static inline void handle_prefix_autonomous(struct net_pkt *pkt,