Skip to content

sys/ztimer: allow compensation of ztimer_sleep() overhead #14936

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
Merged
merged 3 commits into from
Sep 22, 2020
Merged

sys/ztimer: allow compensation of ztimer_sleep() overhead #14936

merged 3 commits into from
Sep 22, 2020

Conversation

kaspar030
Copy link
Contributor

Contribution description

ztimer_sleep() can take considerable overhead.

This PR introduces a per-clock field adjust_sleep that allows compensating this overhead.
It can be configured for ztimer_usec by setting CONFIG_ZTIMER_USEC_ADJUST_SLEEP.

For consistency reasons, the corresponding adjust value field for "ztimer_set()" has been renamed from just "adjust" to "adjust_set", the config option CONFIG_ZTIMER_USEC_ADJUST_SET.

The sleep value is used in addition to the set value.

Testing procedure

  • run tests/ztimer_overhead, not down "min" value, recompile with CFLAGS += -DCONFIG_ZTIMER_USEC_ADJUST_SET=<min>

  • run tests/xtimer_usleep compiled with CFLAGS += -DCONFIG_ZTIMER_USEC_ADJUST_SET=<min> and USEMODULE+=ztimer_xtimer_compat, note offset value

  • re-run tests/xtimer_usleep compiled with CFLAGS += -DCONFIG_ZTIMER_USEC_ADJUST_SET=<min> and USEMODULE+=ztimer_xtimer_compat, and CFLAGS += -DCONFIG_ZTIMER_USEC_ADJUST_SLEEP=<offset>

The offset should now be closer to zero.

Issues/PRs references

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: timers Area: timer subsystems labels Sep 3, 2020
@kaspar030 kaspar030 requested a review from bergzand as a code owner September 3, 2020 08:45
@kaspar030 kaspar030 mentioned this pull request Sep 3, 2020
Closed
@kaspar030
Copy link
Contributor Author

btw, this is a first step towards making this automatic in ztimer's auto_init().

fjmolinas
fjmolinas reviewed Sep 22, 2020
View reviewed changes
@fjmolinas
Copy link
Contributor

Some testing:

  • BOARD=iotlab-m3 make -C tests/ztimer_overhead/ flash test
main(): This is RIOT! (Version: 2020.10-devel-1222-g2ed879-pr-14936)
ZTIMER_USEC->adjust_set = 0
min=9 max=9 avg_diff=9
  • CFLAGS=-DCONFIG_ZTIMER_USEC_ADJUST_SET=9 BOARD=iotlab-m3 make -C tests/ztimer_overhead/ flash test
main(): This is RIOT! (Version: 2020.10-devel-1222-g2ed879-pr-14936)
ZTIMER_USEC->adjust_set = 9
min=0 max=0 avg_diff=0
  • USEMODULE=ztimer_xtimer_compat CFLAGS=-DCONFIG_ZTIMER_USEC_ADJUST_SET=9 BOARD=iotlab-m3 make -C tests/xtimer_usleep/ flash test
main(): This is RIOT! (Version: 2020.10-devel-1222-g2ed879-pr-14936)
Running test 5 times with 7 distinct sleep times
Slept for 10010 us (expected: 10000 us) Offset: 10 us
Slept for 50010 us (expected: 50000 us) Offset: 10 us
Slept for 10244 us (expected: 10234 us) Offset: 10 us
Slept for 56790 us (expected: 56780 us) Offset: 10 us
Slept for 12132 us (expected: 12122 us) Offset: 10 us
Slept for 98775 us (expected: 98765 us) Offset: 10 us
Slept for 75010 us (expected: 75000 us) Offset: 10 us
Slept for 10010 us (expected: 10000 us) Offset: 10 us
Slept for 50010 us (expected: 50000 us) Offset: 10 us
Slept for 10244 us (expected: 10234 us) Offset: 10 us
Slept for 56790 us (expected: 56780 us) Offset: 10 us
Slept for 12132 us (expected: 12122 us) Offset: 10 us
Slept for 98775 us (expected: 98765 us) Offset: 10 us
Slept for 75010 us (expected: 75000 us) Offset: 10 us
Slept for 10010 us (expected: 10000 us) Offset: 10 us
Slept for 50010 us (expected: 50000 us) Offset: 10 us
Slept for 10244 us (expected: 10234 us) Offset: 10 us
Slept for 56790 us (expected: 56780 us) Offset: 10 us
Slept for 12132 us (expected: 12122 us) Offset: 10 us
Slept for 98775 us (expected: 98765 us) Offset: 10 us
Slept for 75010 us (expected: 75000 us) Offset: 10 us
Slept for 10010 us (expected: 10000 us) Offset: 10 us
Slept for 50010 us (expected: 50000 us) Offset: 10 us
Slept for 10244 us (expected: 10234 us) Offset: 10 us
Slept for 56790 us (expected: 56780 us) Offset: 10 us
Slept for 12132 us (expected: 12122 us) Offset: 10 us
Slept for 98775 us (expected: 98765 us) Offset: 10 us
Slept for 75010 us (expected: 75000 us) Offset: 10 us
Slept for 10010 us (expected: 10000 us) Offset: 10 us
Slept for 50010 us (expected: 50000 us) Offset: 10 us
Slept for 10244 us (expected: 10234 us) Offset: 10 us
Slept for 56790 us (expected: 56780 us) Offset: 10 us
Slept for 12132 us (expected: 12122 us) Offset: 10 us
Slept for 98775 us (expected: 98765 us) Offset: 10 us
Slept for 75010 us (expected: 75000 us) Offset: 10 us
Test ran for 1606222 us
  • USEMODULE=ztimer_xtimer_compat CFLAGS="-DCONFIG_ZTIMER_USEC_ADJUST_SET=9 -DCONFIG_ZTIMER_USEC_ADJUST_SLEEP=10" BOARD=iotlab-m3 make -C tests/xtimer_usleep/ flash test
Slept for 10000 us (expected: 10000 us) Offset: 0 us
Slept for 50000 us (expected: 50000 us) Offset: 0 us
Slept for 10234 us (expected: 10234 us) Offset: 0 us
Slept for 56780 us (expected: 56780 us) Offset: 0 us
Slept for 12122 us (expected: 12122 us) Offset: 0 us
Slept for 98765 us (expected: 98765 us) Offset: 0 us
Slept for 75000 us (expected: 75000 us) Offset: 0 us
Slept for 10000 us (expected: 10000 us) Offset: 0 us
Slept for 50000 us (expected: 50000 us) Offset: 0 us
Slept for 10234 us (expected: 10234 us) Offset: 0 us
Slept for 56780 us (expected: 56780 us) Offset: 0 us
Slept for 12122 us (expected: 12122 us) Offset: 0 us
Slept for 98765 us (expected: 98765 us) Offset: 0 us
Slept for 75000 us (expected: 75000 us) Offset: 0 us
Slept for 10000 us (expected: 10000 us) Offset: 0 us
Slept for 50000 us (expected: 50000 us) Offset: 0 us
Slept for 10234 us (expected: 10234 us) Offset: 0 us
Slept for 56780 us (expected: 56780 us) Offset: 0 us
Slept for 12122 us (expected: 12122 us) Offset: 0 us
Slept for 98765 us (expected: 98765 us) Offset: 0 us
Slept for 75000 us (expected: 75000 us) Offset: 0 us
Slept for 10000 us (expected: 10000 us) Offset: 0 us
Slept for 50000 us (expected: 50000 us) Offset: 0 us
Slept for 10234 us (expected: 10234 us) Offset: 0 us
Slept for 56780 us (expected: 56780 us) Offset: 0 us
Slept for 12122 us (expected: 12122 us) Offset: 0 us
Slept for 98765 us (expected: 98765 us) Offset: 0 us
Slept for 75000 us (expected: 75000 us) Offset: 0 us
Slept for 10000 us (expected: 10000 us) Offset: 0 us
Slept for 50000 us (expected: 50000 us) Offset: 0 us
Slept for 10234 us (expected: 10234 us) Offset: 0 us
Slept for 56780 us (expected: 56780 us) Offset: 0 us
Slept for 12122 us (expected: 12122 us) Offset: 0 us
Slept for 98765 us (expected: 98765 us) Offset: 0 us
Slept for 75000 us (expected: 75000 us) Offset: 0 us
Test ran for 1604972 us

In the test I had to change :

diff --git a/tests/xtimer_usleep/tests/01-run.py b/tests/xtimer_usleep/tests/01-run.py
index f629096073..8bd57295c8 100755
--- a/tests/xtimer_usleep/tests/01-run.py
+++ b/tests/xtimer_usleep/tests/01-run.py
@@ -35,7 +35,7 @@ def testfunc(child):
                 sleep_time = int(child.match.group(1))
                 exp = int(child.match.group(2))
                 upper_bound = exp + (exp * INTERNAL_JITTER)
-                if not (exp < sleep_time < upper_bound):
+                if not (exp <= sleep_time < upper_bound):
                     delta = (upper_bound-exp)
                     error = min(upper_bound-sleep_time, sleep_time-exp)
                     raise InvalidTimeout("Invalid timeout %d, expected %d < timeout < %d"

@fjmolinas
Copy link
Contributor

@kaspar030 some nitpicks otherwise it seems to work as advertised, one comment though regarding the CONFIG_ZTIMER_USEC_ADJUST_SLEEP, is there a risk of it undersleeping? Should the value be set to the offset found with the test -1?

@fjmolinas fjmolinas self-assigned this Sep 22, 2020
@kaspar030
Copy link
Contributor Author

one comment though regarding the CONFIG_ZTIMER_USEC_ADJUST_SLEEP, is there a risk of it undersleeping? Should the value be set to the offset found with the test -1?

yes there is. it should be set so "min" is 0, to guarantee "at least X" semantics. well, I think we specified this to be +- one, so -1 in some rare cases might still be acceptable. not sure.

@kaspar030 kaspar030 requested a review from miri64 as a code owner September 22, 2020 09:39
@kaspar030
Copy link
Contributor Author

In the test I had to change :

fixed. interesting, the test previously failed on zero offset...

@fjmolinas
Copy link
Contributor

one comment though regarding the CONFIG_ZTIMER_USEC_ADJUST_SLEEP, is there a risk of it undersleeping? Should the value be set to the offset found with the test -1?

yes there is. it should be set so "min" is 0, to guarantee "at least X" semantics. well, I think we specified this to be +- one, so -1 in some rare cases might still be acceptable. not sure.

Would it be possible to add a short mention to the ztimer.h on how these adjust values can be found out and how to be conservative when selecting them (this +-1 policy for example.)

@kaspar030
Copy link
Contributor Author

Would it be possible to add a short mention to the ztimer.h on how these adjust values can be found out and how to be conservative when selecting them (this +-1 policy for example.)

I intend to do more docs on this in the follow-up PR, which will hopefully just figure out the values automagically on each boot.

@fjmolinas
Copy link
Contributor

I intend to do more docs on this in the follow-up PR, which will hopefully just figure out the values automagically on each boot.

OK, but I intend to quote you on this!

@fjmolinas
Copy link
Contributor

@kaspar030 please squash!

@kaspar030
Copy link
Contributor Author

all green!

fjmolinas
fjmolinas approved these changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK.

@fjmolinas fjmolinas merged commit f3a5ee6 into RIOT-OS:master Sep 22, 2020
@kaspar030 kaspar030 deleted the ztimer_sleep_adjust branch September 22, 2020 13:09
@kaspar030
Copy link
Contributor Author

thanks for the review!

This was referenced Jun 15, 2021
Merged
Merged
@fjmolinas fjmolinas mentioned this pull request Feb 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

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

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

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

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

@smlng smlng Awaiting requested review from smlng

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

Assignees

@fjmolinas fjmolinas

Labels
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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kaspar030 @fjmolinas