Skip to content

Conversation

@nordic-krch
Copy link
Contributor

Algorithm was converting uptime to nanoseconds which can easily
lead to overflows. Changed algorithm to use milliseconds and
nanoseconds for remainder only.

Issue is related to #41111 where it has been seen that it can easily overflow due to limitations of conversion algorithm. Even with algorithm fixed in #41602 (with the cost of performance degradation) there were chances that it will overflow.

Signed-off-by: Krzysztof Chruscinski [email protected]

Algorithm was converting uptime to nanoseconds which can easily
lead to overflows. Changed algorithm to use milliseconds and
nanoseconds for remainder only.

Signed-off-by: Krzysztof Chruscinski <[email protected]>
@nordic-krch
Copy link
Contributor Author

As an example: if we have sys clock frequency 32768Hz (nordic case) then uptime ticks converted to nanoseconds will overflow after 156 days.

@carlescufi carlescufi requested a review from cfriedt January 14, 2022 11:36
@AndreyDodonov-EH
Copy link
Contributor

As an example: if we have sys clock frequency 32768Hz (nordic case) then uptime ticks converted to nanoseconds will overflow after 156 days.

@nordic-krch The overflow after 156 would happen only in case of the "fast" algorithm, correct? (Z_TMCVT_USE_FAST_ALGO)
In case of slower algorithm, there is no multiplication of ticks and to_hz without prior division or mod.

@AndreyDodonov-EH
Copy link
Contributor

@carlescufi @nordic-krch Shouldn't then the same change be applied to clock_settime?

@cfriedt
Copy link
Member

cfriedt commented Dec 7, 2022

Next time, we should try to include a test or two in the PR. I'll put one together now.

@nordic-krch
Copy link
Contributor Author

Shouldn't then the same change be applied to clock_settime?

You are right. Setting clock has the same weakness. I'm a bit overloaded now so won't be able to provide it.

@AndreyDodonov-EH
Copy link
Contributor

Shouldn't then the same change be applied to clock_settime?

You are right. Setting clock has the same weakness. I'm a bit overloaded now so won't be able to provide it.

No problem, apparently @cfriedt stepped up to do that.
I also already have a snippet in our companies fork.

@cfriedt May be, it will be of use to you:

clock_settime.patch.txt

As for the test, I used back-and-forth conversion, e.g.:

    #define TEST_PERIOD_SECONDS 600000
    #define TEST_PERIOS_STEP 50
    for (uint64_t seconds=0; seconds<TEST_PERIOD_SECONDS ; seconds+=TEST_PERIOS_STEP )
    {
        uint64_t ticks = seconds * CONFIG_SYS_CLOCK_TICKS_PER_SEC;
        uint64_t ns = k_ticks_to_ns_floor64(ticks);
        uint64_t calced_seconds = ns / NSEC_PER_SEC;
        if (seconds != calced_seconds) {
            LOG_INF("seconds: %llu, calced_seconds: %llu \n", seconds, calced_seconds);
            break;
        }
    }

It was breaking before introduction of precision formula in #41602

However, using this precision formula introduced some problems with semaphores with timeouts on our NRF91,
but I still need to write something to reproduce it in a laconic way before I open another issue.

@cfriedt
Copy link
Member

cfriedt commented Dec 8, 2022

@AndreyDodonov-EH - you could always submit a PR.

I was actually more concerned with having tests for the changes that have gone in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: POSIX POSIX API Library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants