sys: timeutil: add utility functions for struct timespec #90060

cfriedt · 2025-05-16T11:48:18Z

Add a number of utility functions for manipulating struct timespec.

timespec_add()
timespec_compare()
timespec_equal()
timespec_is_valid()
timespec_negate()
timespec_normalize()
timespec_sub()
timespec_from_timeout()
timespec_to_timeout()

If the __builtin_add_overflow() function is available, then these functions use are mostly branchless, which should provide decent performance on systems with caches and branch prediction (gated by CONFIG_SPEED_OPTIMIZATIONS). Otherwise, manually written alternatives exist that are also perhaps more readable.

The two functions at the end convert time durations between representation as struct timespec and k_timeout_t.

Fixes #88115

Doc Preview

Testing Done:

twister -i -T tests/lib/timespec_util
INFO    - Total complete:   45/  45  100%  built (not run):    0, filtered:  280, failed:    0, error:    0
INFO    - 7 test scenarios (322 configurations) selected, 280 configurations filtered (277 by static filter, 3 at runtime).
INFO    - 42 of 42 executed test configurations passed (100.00%), 0 built (not run), 0 failed, 0 errored, with no warnings in 49.20 seconds.
INFO    - 378 of 378 executed test cases passed (100.00%) on 9 out of total 1058 platforms (0.85%).
INFO    - 42 test configurations executed on platforms, 0 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.json
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

tests/lib/timespec_utils/testcase.yaml

cfriedt · 2025-05-16T11:52:35Z

cc @trembel

bjarki-andreasen

Since timespec is specific to posix, should it be moved to include/zephyr/posix/ where

struct` timespec {
	time_t tv_sec;
	long tv_nsec;
};

is also defined? include/zephyr/sys I believe is primarily for zephyr native libraries, though there obviously are a few overlaps there like fdtable. Not sure how or where we draw such lines :)

bjarki-andreasen · 2025-05-16T14:12:09Z

include/zephyr/sys/timespec_util.h

+ *
+ * @return `true` if the operation would result in integer overflow, otherwise `false`.
+ */
+static inline bool timespec_add(struct timespec *a, const struct timespec *b)


if timespec itself is not nameclashing, the header should probably just be named timespec.h given the namespace timespec_util is not used for any function.

I just merged it directly into timeutil.h

#90060 (comment)

cfriedt · 2025-05-16T14:29:30Z

include/zephyr/sys/timespec_util.h

Based on the structure of the documentation, it might be better just to add the contents of this file directly to include/zephyr/sys/timeutil.h and adjust doxygen groups accordingly.

npitre

A few comments:

First, the bool return value. It feels to me like it should be the other
way around i.e. true=OK and false=problem. Currently you have things like:

    return timespec_negate(&neg) || timespec_add(a, &neg);

This just looks semantically wrong. It should be like: if we can negate
and add then return OK e.g.:

    return timespec_negate(&neg) && timespec_add(a, &neg);

Another example:

    return __builtin_add_overflow(a->tv_sec, b->tv_sec, &a->tv_sec) ||
           __builtin_add_overflow(a->tv_nsec, b->tv_nsec, &a->tv_nsec) ||
           timespec_normalize(a);

IMHO this would be more logically readable as:

    return !__builtin_add_overflow(a->tv_sec, b->tv_sec, &a->tv_sec) &&
           !__builtin_add_overflow(a->tv_nsec, b->tv_nsec, &a->tv_nsec) &&
           timespec_normalize(a);

Translation: if no overflow on the first add and no overflow on the second
add and normalization worked then return OK (true). Yes,
__builtin_add_overflow() returns true on error but that's in the name.
Here we have timespec_add() and the name doesn't imply an error.
Users would have more readable code if:

    if (timespec_add(a, b) {
        /* true: the addition was possible */
    } else {
        /* false: addition was _not_ possible */
    }

Next, I'm a little concerned by the optimization. This:

    int64_t sec = (ts->tv_nsec >= NSEC_PER_SEC) * (ts->tv_nsec / NSEC_PER_SEC) +
+                 (ts->tv_nsec < 0) * DIV_ROUND_UP(-ts->tv_nsec, NSEC_PER_SEC);

That's potentially two divisions just for the purpose of avoiding a branch.
On many architectures (if not most) a branch is still cheaper than a
division, even more so if the division ends up in a library call given
a 64-bit dividend.

And finally, you should add more values to your test case. In particular,
tests with tv_nsec being -NSEC_PER_SEC - 1, -2*NSEC_PER_SEC + 1,
-2*NSEC_PER_SEC - 1, etc. would provide more coverage.

cfriedt · 2025-05-16T17:20:44Z

Since timespec is specific to posix, should it be moved to include/zephyr/posix/

@bjarki-andreasen - mentioned in these docs as well as the C standard, while struct timespec originated with POSIX, it has been a part of ISO C since C11.

See also
https://en.cppreference.com/w/c/chrono/timespec

bjarki-andreasen · 2025-05-16T17:32:32Z

Since timespec is specific to posix, should it be moved to include/zephyr/posix/

@bjarki-andreasen - mentioned in these docs as well as the C standard, while struct timespec originated with POSIX, it has been a part of ISO C since C11.

See also
https://en.cppreference.com/w/c/chrono/timespec

Fair enough :) I have no strong opinion, just noticed that we do define the timespec struct in the posix folder. Anyway PR looks good, definate improvement!

cfriedt · 2025-05-16T19:04:56Z

IMHO this would be more logically readable as:

    return !__builtin_add_overflow(a->tv_sec, b->tv_sec, &a->tv_sec) &&
           !__builtin_add_overflow(a->tv_nsec, b->tv_nsec, &a->tv_nsec) &&
           timespec_normalize(a);

From my perspective, that is worse - there are 5 logical operations instead of 3.

Yes, __builtin_add_overflow() returns true on error but that's in the name.

I'm sort of thinking I would prefer to just append _overflow. To me, it's less useful to add multiple extra logical operations.

Here we have timespec_add().

Yeah, I could definitely rename that to timespec_add_overflow(). That would be in-line with all of the math-extras.h functions.

https://github.com/zephyrproject-rtos/zephyr/tree/main/include/zephyr/sys/math_extras.h

Next, I'm a little concerned by the optimization. This:
    int64_t sec = (ts->tv_nsec >= NSEC_PER_SEC) * (ts->tv_nsec / NSEC_PER_SEC) +
+                 (ts->tv_nsec < 0) * DIV_ROUND_UP(-ts->tv_nsec, NSEC_PER_SEC);
That's potentially two divisions just for the purpose of avoiding a branch. On many architectures (if not most) a branch is still cheaper than a division, even more so if the division ends up in a library call given a 64-bit dividend.

Agreed.

What I initially had is only cheap on processors that have e.g. icache, dcache, HW divide, and only cheap compared to potential cache misses.

I would be happy to use #ifdef CONFIG_SPEED_OPTIMIZATIONS here, because you are right in pointing out that the usual optimization for Zephyr should be for size.

And finally, you should add more values to your test case. In particular, tests with tv_nsec being -NSEC_PER_SEC - 1, -2*NSEC_PER_SEC + 1, -2*NSEC_PER_SEC - 1, etc. would provide more coverage.

Absolutely - thanks for the feedback @npitre.

cfriedt · 2025-05-16T19:32:56Z

Fair enough :) I have no strong opinion, just noticed that we do define the timespec struct in the posix folder. Anyway PR looks good, definate improvement!

Yeah, that's true - we should probably eliminate duplicate definitions especially with #30105

One kind of pervasive thing in POSIX (not sure if it's like this in the c spec as much) is that types can be originally defined in several header files. E.g. <sys/types.h> or <unistd.h> or <time.h> as the case may be. It's a weird overlap. If we use official-ish guard macros, it might work.

npitre

IMHO this would be more logically readable as:
    return !__builtin_add_overflow(a->tv_sec, b->tv_sec, &a->tv_sec) &&
           !__builtin_add_overflow(a->tv_nsec, b->tv_nsec, &a->tv_nsec) &&
           timespec_normalize(a);
From my perspective, that is worse - there are 5 logical operations
instead of 3.

Absolutely not. There are no more operations. The compiler is happy
to flip the logic around as it see fit and it won't care one way or the
other.. OTOH this is better semantics for humans reading the code.

I'm sort of thinking I would prefer to just append _overflow. To me,
it's less useful to add multiple extra logical operations.

Please don't! This is an abomination!

Adding _overflow everywhere instead of ! in a few places is a really
a bad tradeoff. Again, the compiler doesn't care either ways (the code won't
be any bigger or any less efficient).

cfriedt · 2025-05-16T21:05:46Z

Please don't! This is an abomination!

Hehe.. sure, if you insist. I feel less strongly

ycsin

LGTM

cfriedt · 2025-05-17T13:04:44Z

removed _overflow() from fn signatures, invert return values, adjust docs
cast NSEC_PER_SEC to long when compared with tv_nsec to ensure correct behaviour on 32-bit architectures

cfriedt · 2025-05-17T13:07:34Z

I might take a crack at fixing #90029 here as well. It would be nice to be able to put the tests in the tests/unit/timeutil testsuite when they are initially committed instead of moving them later.

npitre

You are doing:

     .tv_sec = (timeout.ticks == K_TICKS_FOREVER) * INT64_MAX +
               (timeout.ticks != K_TICKS_FOREVER) * (ns / NSEC_PER_SEC),
     .tv_nsec = (timeout.ticks == K_TICKS_FOREVER) * (NSEC_PER_SEC - 1) +
                (timeout.ticks != K_TICKS_FOREVER) *
                        (ns - (ns / NSEC_PER_SEC) * NSEC_PER_SEC),

Instead of (ns - (ns / NSEC_PER_SEC) * NSEC_PER_SEC) you could simply do
(ns % NSEC_PER_SEC). When computing (ns / NSEC_PER_SEC) earlier, the
modulus often comes for free as a byproduct of the division and the compiler
would just use it given the opportunity. Granted, a smart compiler will
conclude that the above code can be substituted by a modulus already,
meaning that in the end it doesn't make much difference to the resulting
binary, therefore it is best to just use the simpler code.

No need to respin the PR just for this hence my +1, but if you need to
re-push then this change should be considered.

Add a number of utility functions for manipulating struct timespec. * timespec_add() * timespec_compare() * timespec_equal() * timespec_is_valid() * timespec_negate() * timespec_normalize() * timespec_sub() * timespec_from_timeout() * timespec_to_timeout() If the `__builtin_add_overflow()` function is available, then the API is mostly branchless, which should provide decent performance on systems with an instruction cache and branch prediction. Otherwise, manually written alternatives exist that are also perhaps more readable. The two functions at the end convert time durations between representation as `struct timespec` and `k_timeout_t`. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

cfriedt · 2025-05-20T19:54:07Z

added cast to time_t from INT64_MAX (should resolve issue here)
used modulo operator instead of multiply / divide

Additional Testing Done:

twister -i -p native_sim/native/64 -s buildsystem.cmake.board.extend_two
INFO    - Using 'zephyr' toolchain.
INFO    - Building initial testsuite list...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/testplan.json
INFO    - JOBS: 32
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
INFO    - Total complete:    1/   1  100%  built (not run):    0, filtered:    0, failed:    0, error:    0
INFO    - 1 test scenarios (1 configurations) selected, 0 configurations filtered (0 by static filter, 0 at runtime).
INFO    - 1 of 1 executed test configurations passed (100.00%), 0 built (not run), 0 failed, 0 errored, with no warnings in 10.56 seconds.
INFO    - 1 of 1 executed test cases passed (100.00%) on 1 out of total 1058 platforms (0.09%).
INFO    - 1 test configurations executed on platforms, 0 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing JSON report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.json
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister.xml...
INFO    - Writing xunit report /home/cfriedt/zephyrproject/zephyr/twister-out/twister_report.xml...
INFO    - Run completed

npitre

Still missing one modulo in case you repush.

Add a timespec util testsuite. This should have reasonably high enough coverage to be useful. I would have preferred to add this as an architecture-independent unit test (for the unit_testing platform) under tests/unit/timeutil but there is an inconsistency about the size of time_t on the unit_testing and native_sim/native platforms. On every other platform supported by Zephyr, time_t is 64-bits. However, on those platforms, time_t is only 32-bits. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

Add documentation in the timeutil group for recently added functions for validating, comparing, and manipulating `struct timespec` objects as well as functions for converting between representation of time durations as either `struct timespec` or `k_timeout_t`. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

Use the newly added timespec util functions to manipulate and compare timespec structures with overflow detection. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

cfriedt · 2025-05-20T20:35:23Z

@npitre - jeez - this is like Where's Waldo 😆 Can't sneak anything past you (not that I was trying, it was just hard for me to see).

sonarqubecloud · 2025-05-20T21:03:20Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Add copyright for changes that were included with zephyrproject-rtos#90060. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

cfriedt · 2025-05-22T16:00:16Z

@andyross, @peter-mitsis - please (re-)review when you have a moment. One of you must approve as assignee.

kartben · 2025-05-22T22:44:49Z

@cfriedt clang is unhappy, could you please have a look?
https://github.com/zephyrproject-rtos/zephyr/actions/runs/15197606622

kartben · 2025-05-22T22:51:54Z

It's actually also failing with gcc https://github.com/zephyrproject-rtos/zephyr/actions/runs/15197606643

Add copyright for changes that were included with zephyrproject-rtos#90060. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

Add copyright for changes that were included with #90060. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

jeremybettis · 2025-05-27T16:59:09Z

include/zephyr/sys/timeutil.h

+	return (k_timeout_t){
+		.ticks = ((ts->tv_sec == INT64_MAX) && (ts->tv_nsec == NSEC_PER_SEC - 1)) *
+				 K_TICKS_FOREVER +
+			 ((ts->tv_sec != INT64_MAX) && (ts->tv_sec >= 0)) *


I'm seeing compile errors on this code.

/mnt/host/source/src/third_party/zephyr/main/include/zephyr/sys/timeutil.h:680:18: error: result of comparison of constant 9223372036854775807 with expression of type 'const __time_t' (aka 'const long') is always true [-Werror,-Wtautological-constant-out-of-range-compare] 680 | ((ts->tv_sec != INT64_MAX) && (ts->tv_sec >= 0)) * | ~~~~~~~~~~ ^ ~~~~~~~~~ /mnt/host/source/src/third_party/zephyr/main/include/zephyr/sys/timeutil.h:678:25: error: result of comparison of constant 9223372036854775807 with expression of type 'const __time_t' (aka 'const long') is always false [-Werror,-Wtautological-constant-out-of-range-compare] 678 | .ticks = ((ts->tv_sec == INT64_MAX) && (ts->tv_nsec == NSEC_PER_SEC - 1)) * | ~~~~~~~~~~ ^ ~~~~~~~~~ 2 errors generated.

west twister -p native_sim/native -s rollback/rollback.secure_clear_os with clang

I'm seeing compile errors on this code.

The code / line numbers do not match what is in main currently.

There was a fix submitted. Can you please ensure you are testing a commit that includes d61a7a8 and d3ec916?

Add copyright for changes that were included with zephyrproject-rtos#90060. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

cfriedt commented May 16, 2025

View reviewed changes

tests/lib/timespec_utils/testcase.yaml Outdated Show resolved Hide resolved

cfriedt requested review from npitre, jukkar, ycsin and bjarki-andreasen May 16, 2025 11:50

cfriedt force-pushed the timespec-utils branch from f5bcfe6 to cd39ed4 Compare May 16, 2025 14:05

bjarki-andreasen reviewed May 16, 2025

View reviewed changes

cfriedt commented May 16, 2025

View reviewed changes

cfriedt force-pushed the timespec-utils branch from cd39ed4 to c228df1 Compare May 16, 2025 15:43

cfriedt changed the title ~~sys: util: timespec_util: add collection of inlines for timespec~~ sys: util: timeutil: add utils for manipulating struct timespec May 16, 2025

npitre reviewed May 16, 2025

View reviewed changes

cfriedt force-pushed the timespec-utils branch 2 times, most recently from e72e78a to 7aeb687 Compare May 16, 2025 20:27

npitre reviewed May 16, 2025

View reviewed changes

cfriedt force-pushed the timespec-utils branch 3 times, most recently from 8816044 to 2118238 Compare May 16, 2025 20:53

ycsin reviewed May 17, 2025

View reviewed changes

cfriedt force-pushed the timespec-utils branch from 2118238 to 4085dfb Compare May 17, 2025 13:01

cfriedt changed the title ~~sys: util: timeutil: add utils for manipulating struct timespec~~ sys: timeutil: add utility functions for struct timespec May 17, 2025

cfriedt force-pushed the timespec-utils branch 2 times, most recently from da9e55d to 7a7a195 Compare May 17, 2025 17:37

cfriedt mentioned this pull request May 20, 2025

posix: options: add custom Zephyr POSIX subprofile #88547

Open

5 tasks

npitre previously approved these changes May 20, 2025

View reviewed changes

cfriedt dismissed npitre’s stale review via 17ffa63 May 20, 2025 19:51

cfriedt force-pushed the timespec-utils branch from b17ebd4 to 17ffa63 Compare May 20, 2025 19:51

npitre previously approved these changes May 20, 2025

View reviewed changes

cfriedt added 3 commits May 20, 2025 16:33

posix: timers: use newly added timespec util functions

538d96f

Use the newly added timespec util functions to manipulate and compare timespec structures with overflow detection. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

cfriedt dismissed npitre’s stale review via 538d96f May 20, 2025 20:33

cfriedt force-pushed the timespec-utils branch from 17ffa63 to 538d96f Compare May 20, 2025 20:33

cfriedt requested a review from npitre May 21, 2025 02:36

npitre approved these changes May 21, 2025

View reviewed changes

jukkar approved these changes May 21, 2025

View reviewed changes

ycsin approved these changes May 21, 2025

View reviewed changes

bjarki-andreasen approved these changes May 21, 2025

View reviewed changes

cfriedt added a commit to cfriedt/zephyr that referenced this pull request May 22, 2025

sys: timeutil: add missing copyright for recent changes

6cee684

Add copyright for changes that were included with zephyrproject-rtos#90060. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

peter-mitsis approved these changes May 22, 2025

View reviewed changes

kartben merged commit 8a5c744 into zephyrproject-rtos:main May 22, 2025
27 checks passed

cfriedt deleted the timespec-utils branch May 23, 2025 01:34

cfriedt added a commit to cfriedt/zephyr that referenced this pull request May 23, 2025

sys: timeutil: add missing copyright for recent changes

2bcf28a

Add copyright for changes that were included with zephyrproject-rtos#90060. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

kartben pushed a commit that referenced this pull request May 27, 2025

sys: timeutil: add missing copyright for recent changes

13e827f

Add copyright for changes that were included with #90060. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>

jeremybettis reviewed May 27, 2025

View reviewed changes

sys: timeutil: add utility functions for struct timespec #90060

sys: timeutil: add utility functions for struct timespec #90060

Uh oh!

Conversation

cfriedt commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cfriedt commented May 16, 2025

Uh oh!

bjarki-andreasen left a comment

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen May 16, 2025

Choose a reason for hiding this comment

Uh oh!

cfriedt May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfriedt May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

npitre left a comment

Choose a reason for hiding this comment

Uh oh!

cfriedt commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjarki-andreasen commented May 16, 2025

Uh oh!

cfriedt commented May 16, 2025

Uh oh!

cfriedt commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npitre left a comment

Choose a reason for hiding this comment

Uh oh!

cfriedt commented May 16, 2025

Uh oh!

ycsin left a comment

Choose a reason for hiding this comment

Uh oh!

cfriedt commented May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfriedt commented May 17, 2025

Uh oh!

npitre left a comment

Choose a reason for hiding this comment

Uh oh!

cfriedt commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npitre left a comment

Choose a reason for hiding this comment

Uh oh!

cfriedt commented May 20, 2025

Uh oh!

sonarqubecloud bot commented May 20, 2025

Quality Gate passed

Uh oh!

cfriedt commented May 22, 2025

Uh oh!

Uh oh!

kartben commented May 22, 2025

Uh oh!

kartben commented May 22, 2025

Uh oh!

jeremybettis May 27, 2025

Choose a reason for hiding this comment

Uh oh!

jeremybettis May 27, 2025

Choose a reason for hiding this comment

Uh oh!

cfriedt May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

cfriedt commented May 16, 2025 •

edited

Loading

cfriedt May 16, 2025 •

edited

Loading

cfriedt May 16, 2025 •

edited

Loading

cfriedt commented May 16, 2025 •

edited

Loading

cfriedt commented May 16, 2025 •

edited

Loading

cfriedt commented May 17, 2025 •

edited

Loading

cfriedt commented May 20, 2025 •

edited

Loading

cfriedt May 27, 2025 •

edited

Loading