Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sys/ztimer: fix re-scheduling of timers [backport 2024.07] #20928

Closed
wants to merge 1 commit into from
Closed

sys/ztimer: fix re-scheduling of timers [backport 2024.07] #20928

wants to merge 1 commit into from

Conversation

maribu
Copy link
Member

@maribu maribu commented Oct 21, 2024

Backport of #20924

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?

@Enoch247 @maribu
sys/ztimer: fix re-scheduling of timers
4844633 
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().

(cherry picked from commit 45942f6)
@maribu maribu added Area: sys Area: System Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 21, 2024
@maribu maribu mentioned this pull request Oct 21, 2024
Merged
@riot-ci
Copy link

riot-ci commented Oct 21, 2024 •
edited
Loading

Murdock results

❌ FAILED

4844633 sys/ztimer: fix re-scheduling of timers

Success Failures Total Runtime
3153 0 9482 05m:30s

Artifacts

@maribu maribu enabled auto-merge October 21, 2024 10:43
@maribu
Copy link
Member Author

maribu commented Oct 21, 2024

This backport will not pass the CI until #20929 is merged.

@maribu
Copy link
Member Author

maribu commented Oct 21, 2024

@Teufelchen1 IMO this bug easily qualifies as backport material. Care to take a look?

@maribu maribu mentioned this pull request Oct 21, 2024
Closed
Teufelchen1
Teufelchen1 approved these changes Oct 23, 2024
View reviewed changes
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super deep into ztimer tbh but looks reasonable (and matches the off-github discussion)

@maribu
Copy link
Member Author

maribu commented Nov 12, 2024

Realisticly: Let's just rather get the new release out soon that will contain the fix.

@maribu maribu closed this Nov 12, 2024
auto-merge was automatically disabled November 12, 2024 15:00

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Teufelchen1 Teufelchen1 Teufelchen1 approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

Assignees
No one assigned
Labels
Area: sys Area: System Area: timers Area: timer subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maribu @riot-ci @Teufelchen1 @Enoch247