Make kernel timeout record (struct _timeout) a public API
#91146
Conversation
The kernel timeout record, currenly a private feature, is very useful for subsystems which require a very low overhead timeout with callback. Currently subsystems are forced to use the k_timer for any and all timeouts, which is not efficient, or do some tricks to include and use the kernel timeout record anyway, like #include <../kernel/include/timeout_q.h> like its done for RTIO. This commit exposes the kernel timeout in a zero overhead and backwards compatible manner, with added (and needed) documentation to help users use the kernel timeout record appropriately. It will hopefully encourage subsystems to use the kernel timeout record in place of k_timer where appropriate. In the future, we may even deprecate struct _timeout in favor of k_timeout_record. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
Add test of timeout record to the common kernel tests included if CONFIG_SYS_CLOCK_EXISTS. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
Use newly exposed k_timeout_record instead of "hacking" a bit to use the internal _timeout. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
bjarki-andreasen
added
the
Architecture Review
github-actions
bot
requested review from
andyross,
ceolin,
cfriedt,
dcpleung,
nashif,
npitre,
peter-mitsis,
TaiJuWu,
teburd and
yperess
|
There was a problem hiding this comment.
No complaints, really. I agree this is more generically useful than a k_timer (though k_timer is really a pretty lightweight wrapper) and has a simple API to specify.
But at the same time this is the internal thing, and putting a public face on it is going to freeze the kernel if it wants to do something different in the future (e.g. there is a "min-heap" PR in flight somewhere else where I suggested that it would make a good backend for the timeout list) it might tie our hands if it requires an API change.
Maybe we could mark this "experimental" for the first few releases out of conservatism?
There was a problem hiding this comment.
I also support this PR and I wished it was available when RTIO delay was implemented. Thanks, @bjarki-andreasen!
not really, it is already there zephyr/include/zephyr/kernel_structs.h Line 295 in d8b3775 So, what happens to _timeout? Do we need to keep it? We now will end up with three timeout structs, isnt this going o be confusing? |
I mention in the commit we could decide to deprecate _timeout in favor of the one in this PR, but as noted by @andyross maybe we should give the "new API" some time before comitting to it :) |
I say we need to deprecate and just have one way for doing this and remove the duplicate structs and private APIs/structures. |
This PR promotes the internal kernel timeout record
struct _timeoutto a public struct and API, exposing it with a friendly namestruct k_timeout_recordand documenting it thoroughly. The kernel timeout record is really efficient and useful for subsystems which need a "timer" but can benefit from the lower overhead compared to thek_timer. The nametimeout_recordwas inspired by @nashif :)This PR is backwards compatible, it keeps the internal struct untouched, it just adds a wrapper around it, with a tiny test in
kernel/commonjust to make sure the wrapper functions as expected.