-
Notifications
You must be signed in to change notification settings - Fork 7.6k
lib: os: timespec_util: reimplement and test with broad inputs #92709
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.
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
base: main
Are you sure you want to change the base?
Conversation
fa5790b
to
09cba91
Compare
09cba91
to
54f04c4
Compare
54f04c4
to
4cba546
Compare
I would strongly recommend merging this before |
4cba546
to
4aa56b8
Compare
a79627d
to
e7fb518
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the code correctly there is some overflow issues.
Note also a few suggestions so we would be able to support other time_t sizes.
(I have not reviewed the test itself.)
I added more rounding test-cases above 1000 ticks, so that we could be sure the conversion is accurate (with rounding) in the slightly higher end of |
@cfriedt in the previous to the last commit there has been both quite significant changes to the PR content and a rebase in 1. |
8cfbfc5
to
385daba
Compare
|
@andyross - would be great if you could re-approve. @danieldegrasse @dkalowsk - it would be really quite good to get this I've made quite a few of the recommended changes by Alberto. Coverage and bisectability tests show that it's safe. I think it's ultimately up to you as release managers. |
include/zephyr/sys/clock.h
Outdated
#define K_TICK_MAX ((k_ticks_t)(IS_ENABLED(CONFIG_TIMEOUT_64BIT) ? INT64_MAX : UINT32_MAX - 1)) | ||
|
||
/* The semantic equivalent of K_NO_WAIT but expressed as a timespec object*/ | ||
#define K_TS_NO_WAIT K_TICKS_TO_TIMESPEC(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess TS is for TIMESPEC? Can we please use the same convention everywhere, i.e. K_TIMESPEC_NO_WAIT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure - I can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
The changes to support any time_t size are trivial, and you got them in #92709 (comment) How I see it, you have two options: a) you apply those changes, this code supports and time_t size, this PR can be merged and we move on. |
@@ -166,6 +166,49 @@ typedef struct { | |||
/* added tick needed to account for tick in progress */ | |||
#define _TICK_ALIGN 1 | |||
|
|||
/* Maximum and minimum value TIME_T can hold (accounting for 32-bit time_t in glibc.h) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* Maximum and minimum value TIME_T can hold (accounting for 32-bit time_t in glibc.h) */ | |
/* Maximum and minimum value TIME_T can hold */ |
This supports any time_t size
Define additional constants and macros for converting between timespec objects and k_timeout_t. There are four semantically important durations identified with new constants identified via '. 1. K_NO_WAIT / K_TIMESPEC_NO_WAIT' * duration of zero 2. K_TICK_MIN' / K_TIMESPEC_MIN' * minimum number of ticks strictly greater than K_NO_WAIT 3. K_TICK_MAX' / K_TIMESPEC_MAX' * maximum number of ticks strictly and semantically less than K_FOREVER 4. K_FOREVER / K_TIMESPEC_FOREVER' * the familiar non-expiring number of ticks (i.e. infinity) With these definitions, there are 3 meaningful regions for conversion between timespec and k_timeout_t representation: 1. < K_TIMESPEC_NO_WAIT (i.e. a negative timespec) * should be rounded up to K_TIMESPEC_NO_WAIT * < K_NO_WAIT in ticks is an absolute time point in 64-bit representation should no be permitted in the affected conversion routines, as they are explicitly for converting time durations rather than absolute time points. 2. K_TIMESPEC_NO_WAIT < x < K_TIMESPEC_MAX * should be rounded-up to the next tick boundary * must be represented as the minimal non-zero duration in ticks 3. K_TIMESPEC_MAX <= x < K_TIMESPEC_FOREVER * must be represented as the maximum expiring duration in ticks * should be rounded-down to K_TIMESPEC_MAX Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Originally, the timespec_to_timeout() and timespec_from_timeout() tests assumed that tv_sec and tv_nsec values divided evenly with the number of ticks per second (CONFIG_SYS_CLOCK_TICKS_PER_SEC). However, those assumptions broke down when testing with 32768 ticks/s . Upon further investigation, there were additional corner cases discovered that were not handled in an ideal or safe way. Part of this fix involved identifying several "domains" and special values that have semantic meaning that must be handled particularly carefully. Additional hidden-API constants were made to simplify the solution. K_TICK_MIN (1) K_TICK_MAX (UINT32_MAX-1 in 32-bit, INT64_MAX in 64-bit) K_TIMESPEC_NO_WAIT (like K_NO_WAIT) K_TIMESPEC_FOREVER (like K_FOREVER) 1. Converting a negative k_timeout_t These only exist in 64-bit representation and actually encode absolute timepoints via ticks. Since the stated purpose of the conversion functions is to convert between durations, we must reject absolute time points in timespec_from_timeout(). 2. Converting a negative timespec We assume that this duration means a timeout has already expired, and round these up to K_NO_WAIT in timespec_to_timeout(). 3. Due to the larger numeric space in the timespec representation, the reverse mapping of timespec to k_timeout_t is "fuzzy". However, K_NO_WAIT (K_TIMESPEC_NO_WAIT) and K_FOREVER (K_TIMESPEC_FOREVER) must remain semantically equivalent. The previous implementation also held this to be true, but the test cases are a bit clearer about it now. 4. Also, due to the larger numeric space in timespec representation, there was a special requirement to round up to the nearest tick boundary for any timespec in the strictly exclusive range (K_TIMESPEC_NO_WAIT, K_TIMESPEC_MAX). We must round up, since a) rounding down to K_NO_WAIT is most certainly not representative of a non-zero finite duration delay b) the kernel operates on tick boundaries 5. Above the K_TIMESPEC_MAX boundary, which is the highest possible timespec that can be represented by a tick, and specifically in the 64-bit timeout representation, there is a domain that cannot be rounded up to K_TIMESPEC_FOREVER and should always be rounded down to K_TIMESPEC_MAX. This is to ensure that finite durations remain finite (even if they are beyond the heat death of the universe). Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Elaborate on testing for functions that convert between timespec and k_timeout_t. We explicitly add tests to verify functionality in the regions of interest, verify the semantic equivalence of specific time points. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Since it's possible that rounding up might not always be the right thing to do in every situation, in order to allow the application to make more informed decisions, we created a modified timespec_to_timeout() that also returns the remainder (or difference) between the requested time to convert and resulting k_timeout_t. The difference is expressed as a timespec object. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
This reverts commit 8f09262. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
385daba
to
a2cb683
Compare
@aescolar - thanks. I appreciate your technical input on this topic, and your reviews were very helpful especially in terms of the modulo arithmetic.
There is a third option. Where we merge this PR as a fix for the bugs that it's fixing, keep the scope of it narrow, and move on. The idealogical differences that we have about whether Zephyr should explicitly support 32-bit However, I did make use of your suggestion to incorporate |
|
Aside from fixing 4 bugs, another reason to consider merging this before tagging v4.2.0 is to avoid forcing the deprecation process for This PR adds the If this is merged within the release cycle, then If this is not merged within this release cycle, then we will need to rename In other words, @danieldegrasse and @dkalowsk - I would be happy to bring it up with the TSC if necessary. @andyross has already approved once and changed requested by @nashif have been made. |
We can just backport this change to 4.2. About this code today requiring a 64bit time_t, it seems you are misunderstanding my motivation to ask for any time_t to be suported. The size of time_t is decided by the C library given the platform. Several of our users and contributors (including TSC members) have their own out of tree architectures, toolchains and C libraries, and/or have clients who do. For processors focused on audio processing 24/48bit architectures are very typical. Many users will not want to care at all about time_t/timespec either because they do not have a need for that kind type, or because they are all together avoiding it due to the trouble around time_t and the functions that use it (see your own MISRA reference as an example). This code (lib/os/clock.c and timeutil) is built always, and so are these new (for a couple of weeks) functions. Even for users do not have any interest in using time_t or timespec. These users will probably be a bit confused about what part of the zephyr code they may use could be using these or not. Quite many users are using the native_simulator based targets for host testing, and prefer to build with their host libc (glibc typically) so they can interact with the host easier. A lot of this "host interaction" happens by using other host prebuilt libraries. In general many of Zephyr developers just want to be able to test their code with any libC and don't want unnecessary constraints set on them. You seem to think that the Zephyr project can easily force time_t to be 64bits. But it can't, because its size is defined by the libC. All the Zephyr project can do is make it more difficult to work with other time_t sizes and take time and frustrate our users/developers. What I'm requesting is that this code would support any time_t size. Be it 32, 48, 64 or whatever the libC and user decides. If you do this change, the code supports any libc, so you can remove the filter on the library on this code, and stop selecting pico (if you want). And any other developer whose code uses this code can also avoid having any constraint about the libC choice or time_t size in their testing. I just don't see any drawback in supporting any time_t size, but quite a lot of drawbacks of not supporting it. |
2025-07-15: release meeting
|
Originally, the
timespec_to_timeout()
andtimespec_from_timeout()
tests assumed thattv_sec
andtv_nsec
values divided evenly with the number of ticks per second (CONFIG_SYS_CLOCK_TICKS_PER_SEC
). However, those assumptions broke down when testing with 32768 ticks/s .Upon further investigation, there were additional corner cases discovered that were not handled in an ideal or safe way.
Part of this fix involved identifying several "domains" and special values that have semantic meaning that must be handled particularly carefully.
Fixes #92158
Fixes #92908
Fixes #92912
Fixes #90479 (Coverity CID 524770)
Some context for #92908 and #92912 is provided below.
Added Constants
Additional hidden-API constants were made to simplify the solution. These are "hidden" via an existing Doxygen
@cond
block.K_TICK_MIN
: 1K_TICK_MAX
:UINT32_MAX
-1 in 32-bit,INT64_MAX
in 64-bitK_TIMESPEC_MIN
: the equivalent ofK_TICK_MIN
as atimespec
objectK_TIMESPEC_MAX
: the equivalent ofK_TICK_MAX
as atimespec
objectK_TIMESPEC_NO_WAIT
: the equivalent ofK_NO_WAIT
as atimespec
objectK_TIMESPEC_FOREVER
: the equivalent ofK_FOREVER
as atimespec
objectCoverage Details
k_timeout_t
These only exist in 64-bit representation and actually encode absolute timepoints via ticks. Since the stated purpose of the conversion functions is to convert between durations, we must reject absolute time points in timespec_from_timeout(). The previous implementation did this as well.
timespec
We assume that this duration means a timeout has already expired, and round these up to
K_NO_WAIT
intimespec_to_timeout()
. The previous implementation also considered this case but it has been simplified withtimespec_compare()
.Due to the larger numeric space in the timespec representation, the reverse mapping of timespec to
k_timeout_t
is "fuzzy". However,K_NO_WAIT
(K_TIMESPEC_NO_WAIT
) andK_FOREVER
(K_TIMESPEC_FOREVER
) must remain semantically equivalent. The previous implementation also held this to be true, but the test cases are a bit clearer about it now.Also, due to the larger numeric space in timespec representation, there was a special requirement to round up to the nearest tick boundary for any timespec in the strictly exclusive range (
K_TIMESPEC_NO_WAIT
,K_TIMESPEC_MAX
). We must round up, sinceK_NO_WAIT
is most certainly not representative of a non-zero finite duration delayHowever, since it's possible that rounding up might not always be the right thing to do, in order to allow the application to make more informed decisions, we created a modified
timespec_to_timeout_rem()
that also returns the remainder (or difference) between the requested time to convert and resultingk_timeout_t
. The difference is expressed as atimespec
object. The previous implementation did not provide this capability in a convenient manner.K_TIMESPEC_MAX
boundary, which is the highest possible timespec that can be represented by a tick, and specifically in the 64-bit timeout representation, there is a domain that cannot be rounded up toK_TIMESPEC_FOREVER
and should always be rounded down toK_TIMESPEC_MAX
.This is to ensure that finite durations remain finite (even if they are beyond the heat death of the universe).
Added API
The function above accounts for the fact that converting from
timespec
(which has a much larger numerical range) tok_timeout_t
is a "fuzzy" conversion. The entire numerical range oftimespec
must be mapped to either a semantick_timeout_t
or must be aligned to an appropriate tick boundary.Fortunately, it is not a "lossy" conversion.
Inline Functions changed
timespec_from_timeout()
was changed from an inline to a regular functiontimespec_to_timeout()
was changed to a wrapper aroundtimespec_to_timeout_rem()
withrem = NULL
Testing Done
Additional test cases were defined to exercise permutations of: