Big timer API refactoring #10160
Big timer API refactoring #10160
Conversation
andyross
requested review from
nashif,
ceolin,
cvinayak,
carlescufi,
pizi-nordic,
ramakrishnapallala and
dcpleung
andyross
requested review from
aescolar,
anangl,
andrewboie,
galak,
jukkar,
kgugala,
MaureenHelm,
nniranjhana,
pfalcon,
pgielda,
pswarnak,
ruuddw,
spoorthik and
tbursztyka
as code owners
andyross
force-pushed
the
timer-refactor
branch
from
ca62355 to
dde6a1c
Compare
Codecov Report
@@ Coverage Diff @@
## master #10160 +/- ##
==========================================
- Coverage 52.58% 52.52% -0.06%
==========================================
Files 213 216 +3
Lines 26148 26164 +16
Branches 5636 5636
==========================================
- Hits 13749 13743 -6
- Misses 10145 10168 +23
+ Partials 2254 2253 -1
Continue to review full report at Codecov.
|
This header doesn't get included in assembly context, nor does it provide any asm-usable macros. Signed-off-by: Andy Ross <[email protected]>
The kernel.h file had a bunch of internal APIs for timeout/clock handling mixed in. Move these to sys_clock.h, which it always included (in a weird location, so move THAT to kernel_includes.h with everything else). Signed-off-by: Andy Ross <[email protected]>
The existing API defined sys_clock_{hw_cycles,ticks}_per_sec as simple
"variables" to be shared, except that they were only real storage in
certain modes (the HPET driver, basically) and everywhere else they
were a build constant.
Properly, these should be an API defined by the timer driver (who
controls those rates) and consumed by the clock subsystem. So give
them function syntax as a stepping stone to get there.
Note that this also removes the deprecated variable
_sys_clock_us_per_tick rather than give it the same treatment.
Signed-off-by: Andy Ross <[email protected]>
This was only used in a few places just to indirect the already perfectly valid SYS_CLOCK_TICKS_PER_SEC value. There's no reason for these to ever have been kconfig units, and in fact the distinction appears to have introduced a hidden/untested bug in the power subsystem (the two variables were used interchangably, but they were defined in reciprocal units!). Just use "ticks" as our time unit pervasively, and clarify the docs to explain that. Signed-off-by: Andy Ross <[email protected]>
This just got turned into a function from a "variable" API, but post-the-most-recent-patch it turns out to be degenerate anyway. Everyone everywhere should always have been using the kconfig variable directly, and it was only a weirdness in the tickless API that made it confusing. Fix. Signed-off-by: Andy Ross <[email protected]>
This was another "global variable" API. Give it function syntax too. Also add a warning, because on nRF devices (at least) the cycle clock runs in kHz and is too slow to give a precise answer here. Signed-off-by: Andy Ross <[email protected]>
This flag is an indication to the timer driver that the OS doesn't
care about rollover conditions of the tick count while idling, so the
system doesn't need to wake up once per counter flip[1]. Obviously in
that circumstance values returned from k_uptime_get_32() are going to
be wrong, so the implementation had an assert to check for misuse.
But no one understood that from the docs, so the only place these APIs
were used in practice were as "guards" around code that needed to call
k_uptime_get_32(), even though that's 100% wrong per docs!
Clarify the docs. Remove the incorrect guards. Change the flag to
initialize to true so that uptime isn't broken-by-default in tickless
mode. Also move the implemenations of the functions out of the
header, as there's no good reason for these to need to be inlined.
[1] Which can be significant. A 100MHz ARM using the 24 bit SysTick
counter rolls over at about 6 Hz, and if it had to come out of
idle at that rate it would be a significant power issue that would
swamp the gains from tickless. Obviously systems with slow
counters like nRF or 64 bit ones like RISC-V or x86's TSC aren't
as affected.
Signed-off-by: Andy Ross <[email protected]>
The system tick count is a 64 bit quantity that gets updated from
interrupt context, meaning that it's dangerously non-atomic and has to
be locked. The core kernel clock code did this right.
But the value was also exposed to the rest of the universe as a global
variable, and virtually nothing else was doing this correctly. Even
in the timer ISRs themselves, the interrupts may be themselves
preempted (most of our architectures support nested interrupts) by
code that wants to set timeouts and inspect system uptime.
Define a z_tick_{get,set}() API, eliminate the old variable, and make
sure everyone uses the right mechanism.
Signed-off-by: Andy Ross <[email protected]>
This header isn't actually needed in the one assembly context where it's included. Signed-off-by: Andy Ross <[email protected]>
There were three separate "announce ticks" entry points exposed for use by drivers. Unify them to just a single z_clock_announce() function, making the "final" tick announcement the business of the driver only, not the kernel. Note the oddness with "_sys_idle_elapsed_ticks": this was a global variable exposed by the kernel. But it was never actually used by the kernel. It was updated and inspected only within the timer drivers, and only so that it could be passed back to the kernel as the default (actually hidden) argument to the announce function. Break this false dependency by putting this variable into each timer driver individually. Signed-off-by: Andy Ross <[email protected]>
The existing API had two almost identical functions: _set_time() and _timer_idle_enter(). Both simply instruct the timer driver to set the next timer interrupt expiration appropriately so that the call to z_clock_announce() will be made at the requested number of ticks. On most/all hardware, these should be implementable identically. Unfortunately because they are specified differently, existing drivers have implemented them in parallel. Specify a new, unified, z_clock_set_timeout(). Document it clearly for implementors. And provide a shim layer for legacy drivers that will continue to use the old functions. Note that this patch fixes an existing bug found by inspection: the old call to _set_time() out of z_clock_announce() failed to test for the "wait forever" case in the situation where clock_always_on is true, meaning that a system that reached this point and then never set another timeout would freeze its uptime clock incorrectly. Signed-off-by: Andy Ross <[email protected]>
Rename timer driver API functions to be consistent. ADD DOCS TO THE HEADER so implementations understand what the requirements are. Remove some unused functions that don't need declarations here. Also removes the per-platform #if's around the power control callback in favor of a weak-linked noop function in the driver initialization (adds a few bytes of code to default platforms -- we'll live, I think). Signed-off-by: Andy Ross <[email protected]>
The tickless driver had a bunch of "hairy" APIs which forced the timer drivers to do needless low-level accounting for the benefit of the kernel, all of which then proceeded to implement them via cut and paste. Specifically the "program_time" calls forced the driver to expose to the kernel exactly when the next interrupt was due and how much time had elapsed, in a parallel API to the existing "what time is it" and "announce a tick" interrupts that carry the same information. Remove these from the kernel, replacing them with synthesized logic written in terms of the simpler APIs. In some cases there will be a performance impact due to the use of the 64 bit uptime call, but that will go away soon. Signed-off-by: Andy Ross <[email protected]>
andyross
force-pushed
the
timer-refactor
branch
from
dde6a1c to
84ff95f
Compare
| @@ -15,6 +15,8 @@ | |||
|
|
|||
| static u32_t accumulated_cycle_count; | |||
|
|
|||
| static s32_t _sys_idle_elapsed_ticks = 1; | |||
There was a problem hiding this comment.
Could we rename this variable in the drivers?
static _sys_.... located in timer driver does not sound good for me.
There was a problem hiding this comment.
The legacy drivers are likely going away and being replaced with much simpler ones written to the new API. I wanted to avoid needless churn there. But sure, to the extent we don't want to rewrite timer drivers doing a cleanup pass after all this mess wouldn't be a bad idea.
| @@ -341,3 +341,24 @@ void _nano_sys_clock_tick_announce(s32_t ticks) | |||
| } | |||
| #endif | |||
| } | |||
|
|
|||
| int k_enable_sys_clock_always_on(void) | |||
There was a problem hiding this comment.
IMHO if we are not in tickless mode we we should return:
- 1 from
k_enable_sys_clock_always_on() - -ENOTSUP from
k_disable_sys_clock_always_on()(requires API change).
Now the API tells that you can always disable this feature, but re-enabling might be not supported.
There was a problem hiding this comment.
Agreed, though a little out of scope for this particular pull request. As mentioned in the commit note, this API is sort of broken by design. Personally I'd argue we want to remove it in general and just replace it with a kconfig where the app can tell the kernel whether it wants to allow the clock to lie after system idle or not.
There was a problem hiding this comment.
+1 for Kconfig option.
| * @return the current system tick count | ||
| * | ||
| */ | ||
| u32_t z_tick_get_32(void); |
There was a problem hiding this comment.
It's used in some tests. More broadly, the tick count is 64 bit and requires locking, so being able to grab just the low word is a performance win. Not a terrible API IMHO.
drivers/timer/legacy_api.h
Outdated
| void _timer_idle_exit(void); | ||
| #endif | ||
|
|
||
| extern void z_clock_set_timeout(s32_t ticks, bool idle) |
There was a problem hiding this comment.
Why not static inline?
| * | ||
| * Informs the system clock driver that the next needed call to | ||
| * z_clock_announce() will not be until the specified number of ticks | ||
| * from the the current time have elapsed. Note that spurious calls |
There was a problem hiding this comment.
From the current time or from the last tick?
If we use current time as a reference with fast clock + short tick, we might introduce error.
There was a problem hiding this comment.
Current time expressed in ticks. I'll clean up these docs before the final series. Remember per disccussion earlier that nothing in the system is allowed to "round" ticks. If you express current time here you're still talking in the same lockstep units.
There was a problem hiding this comment.
I am afraid about slipping over tick(s) when new timeout is set. Please consider the nRF51 device: For maximum precision, some would like to have 32768 ticks per second (1 tick = 1 hw cycle). In such setup one tick is equals to ~31 us. The CPU in this SoC runs at 16MHz, so 1 tick is roughly 500 CPU cycles.
The execution path from z_clock_announce() to z_clock_set_timeout() requires some time, and could be preempted by high priority interrupts (for example from BLE stack). So now() (in ticks) at the time of z_clock_set_timeout() might be different than now() announced by z_clock_announce(), which is used by kernel to adjust timeouts list and calculate next event time.
In my opinion we either have to eliminate "now()" from the deltas calculation or make timeout list absolute.
There was a problem hiding this comment.
FWIW: check out the logic in z_clock_announce() that I just pushed in #10303. The current time is adjusted while executing callbacks such that it should be safe against that kind of skew. A timeout added is always atomic relative to the most recently announced tick.
But needless to say, we don't have a test case for that (I guess we could write one with some abusive irq_locking...)
kernel/sys_clock.c
Outdated
| @@ -36,6 +36,8 @@ int z_clock_hw_cycles_per_sec; | |||
| */ | |||
| static volatile u64_t tick_count; | |||
|
|
|||
| u64_t z_last_tick_announced; | |||
There was a problem hiding this comment.
Shouldn't we protect access to this variable?
There was a problem hiding this comment.
This one is internal to the kernel only, so maybe not. But it's going away anyway -- it exists as a temporary shim while the APIs switch around.
|
@aescolar please provide a review ASAP, blocking a PR without a review is not nice and should be avoided. We will not merge this without additional review for sure,, given the weight of the change, so be assured it will get enough reviews. |
| extern u32_t _get_elapsed_program_time(void); | ||
| #endif | ||
|
|
||
| extern void z_clock_set_timeout(s32_t ticks, bool idle) |
There was a problem hiding this comment.
I guess this extern is a (harmless) typo?
| /* Stub implementation of z_clock_set_timeout() in terms of the | ||
| * original APIs. Used by older timer drivers. Should be replaced. | ||
| * | ||
| * Yes, this "header" includes a function definition and must be |
There was a problem hiding this comment.
Maybe you want to declare the function static to save this note?
There was a problem hiding this comment.
It's a public function. Basically the legacy_api.h "header" is a stub of C code that the driver code can include to implement the new API in terms of the old one. It's grown another wart or two in the next pull request too.
And yes: it's ugly. The intent is that this is the shim we use to get the new timer backend working, then we go back and rewrite the timer drivers.
There was a problem hiding this comment.
Basically the legacy_api.h "header"
Let me piggyback on this discussion and say that "legacy_api.h" name and stuff like #include "legacy_api.h" looks very confusing to me. Oftentimes, people won't resolve include paths to keep in mind that there's (or maybe not) "legacy_api.h" refers to timer/ directory (or maybe not). Can this be renamed to timer_legacy.h or similar please. (Yes, it will be .../timer/timer_*.h. That's ok.)
There was a problem hiding this comment.
Sorry, missed this one when I closed the bug. FWIW: the double quote inclusion in standard C always refers to the local source directory. Uses like this (i.e. headers that don't define public APIs for inclusion elsewhere) are sort of what it's for. I can look at renaming it though.
But again: the hope is that this header is short-lived. It exists so as to not break Zephyr when used with the old-style timer drivers. Newer ones are much smaller (seriously, like 20-25% the size of the earlier drivers). Take a look at my timerhacks branch for some work getting started on this
| * Note that ticks can also be passed the special value K_FOREVER, | ||
| * indicating that no future timer interrupts are expected or required | ||
| * and that the system is permitted to enter an indefinite sleep even | ||
| * if this could cause rolloever of the internal counter (i.e. the |
| /** | ||
| * @brief Timer idle exit notification | ||
| * | ||
| * This notifies the timer driver that the system is exiting the idle |
There was a problem hiding this comment.
the idle => from the idle state(?)
aescolar
dismissed
their stale review
I did not review it thoroughly enough for a +2, but I did not find anything bad
|
For those who have been watching this, note that a further evolved series (with real changes, not just refactoring) is up at #10303 |
|
The extended PR in #10303 has come together faster than anticipated. I'm going to tag this one DNM but leave it open to finish existing discussions. Please do further review on that one. |
|
Closing this one. Seems like all conversation has moved to the more current one now. |
[Note: this PR has been superceded by #10303, which contains all of it and much more]
Big refactoring of the timer subsystem APIs, with a primary goal of
shrinking the surface of the timer driver interface.
Basically: look at sys_clock.h and system_timer.h for the core
meaning: the interfaces are smaller and more orthogonal now,
permitting (much) simpler drivers to be written and allowing for
significant work to the timer backend to clean up the "TICKLESS
#ifdefery" madness. And the driver interface is now documented in
place, so we know what promises are being made and don't have to guess
at the intended meaning of e.g. "_get_elapsed_program_time()".
In particular, note that "tickless" can now be viewed as the default
mode for a driver. If it wants to be simple, it can just deliver one
z_clock_announce() call per tick and ignore the "set_timeout" call as
a hint. Or it can take that hint and arrange to deliver interrupts at
a lower frequency. The kernel doesn't need to care about the
difference!
Tests seem not to have broken with this series, so I'd be comfortable
committing it. But again, this is just refactoring and not a new
feature, so it doesn't have to be rushed.
(And full disclosure: the very fact that nothing seems to have
broken with the tickless changes speaks more to the weakness of our
test coverage in tickless mode more than it does to the code quality
here! I really would have expected to have to chase down at least one
hard bug, and I didn't see anything break.).