sys/ztimer: fix re-scheduling of timers

[Enoch247]

Contribution description

If the timer at the head of a ztimer clock's timer list is re-scheduled (ztimer_set() called on an already set timer) and the timer is no longer at the head after being re-scheduled, clock-ops->set() is never called from inside ztimer_set(), and the underlying timer is left with an ISR scheduled to expire at the timer's old time. The intended behavior is that the clock's lower level timer should always be set to expire at the time of the clocks head timer.

This patch changes ztimer_set() to call _ztimer_update(), which sets the lower level timer according to the current list of timers, rather than setting the timer directly inside of ztimer_set().

This is a fix we might consider back porting. As far as I can tell this bug has always existed in ztimer.

Testing procedure

• apply patch to create testbench
• run make -C examples/hello-world/ all term
• observe that the timers without this fix fire at the wrong time, and that with this fix they fire at the correct time

Testbench patch

diff --git a/examples/hello-world/Makefile b/examples/hello-world/Makefile
index 258d8e9baf..92c9137241 100644
--- a/examples/hello-world/Makefile
+++ b/examples/hello-world/Makefile
@@ -15,4 +15,6 @@ DEVELHELP ?= 1
 # Change this to 0 show compiler invocation lines by default:
 QUIET ?= 1
 
+USEMODULE += ztimer_usec
+
 include $(RIOTBASE)/Makefile.include
diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index 213128ac64..f17d23c8e4 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -20,6 +20,12 @@
  */
 
 #include <stdio.h>
+#include <ztimer.h>
+
+void cb(void *arg) {
+    uint32_t n = (uint32_t)arg;
+    printf("t%"PRIu32"@%"PRIu32"\n", n, ztimer_now(ZTIMER_USEC));
+}
 
 int main(void)
 {
@@ -28,5 +34,15 @@ int main(void)
     printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
     printf("This board features a(n) %s CPU.\n", RIOT_CPU);
 
+    ztimer_t t1 = {.callback = cb, .arg = (void*)1};
+    ztimer_t t2 = {.callback = cb, .arg = (void*)2};
+
+    cb((void*)0);
+    ztimer_set(ZTIMER_USEC, &t1, 1*1000000);
+    ztimer_set(ZTIMER_USEC, &t2, 2*1000000);
+    //ztimer_remove(ZTIMER_USEC, &t1); // If this line is commented out these timers do not fire at the correct time.
+    ztimer_set(ZTIMER_USEC, &t1, 3*1000000);
+
+    while(1);
     return 0;
 }

Sample of testbench's expected (good) behavior

2024-10-18 15:06:43,015 # RIOT native interrupts/signals initialized.
2024-10-18 15:06:43,016 # RIOT native board initialized.
2024-10-18 15:06:43,017 # RIOT native hardware initialization complete.
2024-10-18 15:06:43,018 # 
2024-10-18 15:06:43,018 # main(): This is RIOT! (Version: 2024.10-devel-323-g45942-fix-ztimer-timer-reschedule)
2024-10-18 15:06:43,018 # Hello World!
2024-10-18 15:06:43,018 # You are running RIOT on a(n) native board.
2024-10-18 15:06:43,019 # This board features a(n) native CPU.
2024-10-18 15:06:43,019 # t0@34
2024-10-18 15:06:44,013 # t2@2000830
2024-10-18 15:06:45,013 # t1@3000279

Sample of testbench's behavior without this fix

2024-10-18 14:46:21,082 # RIOT native interrupts/signals initialized.
2024-10-18 14:46:21,083 # RIOT native board initialized.
2024-10-18 14:46:21,083 # RIOT native hardware initialization complete.
2024-10-18 14:46:21,083 # 
2024-10-18 14:46:21,083 # main(): This is RIOT! (Version: 2024.07-devel-164-g99cef-add-stm32h7.wip+ohsheet.patched+u5-spi)
2024-10-18 14:46:21,084 # Hello World!
2024-10-18 14:46:21,084 # You are running RIOT on a(n) native board.
2024-10-18 14:46:21,084 # This board features a(n) native CPU.
2024-10-18 14:46:21,084 # t0@23
2024-10-18 14:46:21,085 # t2@1000230
2024-10-18 14:46:21,085 # t1@1000248
2024-10-18 14:46:39,207 # 
2024-10-18 14:46:39,207 # native: exiting
2024-10-18 14:46:39,207 # Exiting Pyterm

Issues/PRs references

This bug is possible the cause of the early timeout worked around in this #19965. Perhaps that workaround could be removed?

[riot-ci]

Murdock results

✔️ PASSED

45942f6 sys/ztimer: fix re-scheduling of timers

Success	Failures	Total	Runtime
10212	0	10214	13m:06s

Artifacts

• Documentation preview

[benpicco]

That fix looks pretty straightforward!

Please also add a test case that reproduced the bug.

[maribu]

Backport provided in #20928

[maribu]

This bug is possible the cause of the early timeout worked around in this #19965. Perhaps that workaround could be removed?

Mostly yes.

However, the timeout in there can also trigger early even when ztimer does not trigger early: The GNRC SOCK implementation uses an mbox to wait for a network message to be received. However, mbox has no timeout. Instead, ztimer will put a message to the mbox when it is triggered. If the mbox message related to a received network message is put into the mbox just before the timeout message, the next one to fetch something from the mbox will get the stale timeout message from the mbox directly.

[maribu]

I think with #21113 the work around in #19965 could indeed be dropped for good.