kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks() #9073

pizi-nordic · 2018-07-23T12:20:05Z

The value of sys_clock_ticks_per_sec is obtained using
simple integer division with rounding toward zero. As result
using this variable in _ms_to_ticks() introduces some error.

This PR eliminates sys_clock_ticks_per_sec from equation
used in _ms_to_ticks() removing introduced error.

Also, this PR fixes #8895.

Please note, that this PR is based on top of #9137.

codecov-io · 2018-07-23T12:48:57Z

Codecov Report

Merging #9073 into master will increase coverage by 0.58%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #9073      +/-   ##
==========================================
+ Coverage   52.35%   52.94%   +0.58%     
==========================================
  Files         201      201              
  Lines       25290    25303      +13     
  Branches     5286     5288       +2     
==========================================
+ Hits        13240    13396     +156     
+ Misses       9965     9786     -179     
- Partials     2085     2121      +36

Impacted Files	Coverage Δ
include/kernel.h	`98.43% <ø> (ø)`	⬆️
kernel/sys_clock.c	`95.23% <100%> (ø)`	⬆️
kernel/sched.c	`91.22% <100%> (+0.03%)`	⬆️
subsys/net/ip/route.c	`55.24% <0%> (-0.7%)`	⬇️
subsys/net/ip/utils.c	`79.82% <0%> (+0.28%)`	⬆️
subsys/net/ip/net_pkt.c	`64.37% <0%> (+0.38%)`	⬆️
include/net/net_ip.h	`70.39% <0%> (+1.31%)`	⬆️
subsys/net/ip/net_if.c	`64.32% <0%> (+1.32%)`	⬆️
include/net/net_pkt.h	`88.2% <0%> (+2.56%)`	⬆️
subsys/net/ip/icmpv6.c	`36.58% <0%> (+6.27%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f23d26...178bf48. Read the comment docs.

pizi-nordic · 2018-07-23T15:45:55Z

@andrewboie, @andyross: Some advice needed.

This PR introduces a correct a round up in _ms_to_ticks(), which fixes a shorter than expected sleep time when sleep over several ticks was requested. However this round-up introduced another errors which are visible here: https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/19362/5/tests

The tests fail because N * rounded_up_ms_to_ticks(T) is larger than rounded_up_ms_to_ticks(N * T) .
I do not know how to solve this problem in a correct way. IMHO we have the following options (short-term):

Accept the situation and update failing tests.
Round up sys_clock_hw_cycles_per_tick and use simple division in _ms_to_ticks(): In such case the k_sleep() and friends will always sleep longer than expected (but other side effects could appear).

What do you think?

andyross · 2018-07-23T16:03:41Z

Not having looked at the code: my preference would be to fix the test. Hyper-tight tolerances on timer behavior is always going to cause problems. The relevant standards (this is a POSIX test) don't general promise that anyway.

pizi-nordic · 2018-07-25T14:59:35Z

@andyross: I have one more question: At the moment the _ms_to_ticks() rounds up, while __ticks_to_ms() rounds down. As result, the __ticks_to_ms(_ms_to_ticks(n)) != n. Do you know what is the purpose of such design?

andyross · 2018-07-25T15:49:22Z

I don't. And I'd be terrified to change it lest we break something subtle that relies on positive-definite timer intervals or something.

Really the whole low level timer subsystem needs rework. I'm looking at it right not just in the context of spinlockification and realizing there's a bunch of odd cruft in there.

pizi-nordic · 2018-07-25T16:09:23Z

What I noted while working on #8896, is that we are often changing units. It works perfectly as long as there is no rounding during this conversion (or rounding is in the same direction, but this just hides the problem). IMHO a lot of issues popping up on nRF5x platform are related to loss of accuracy during these conversions.

Are there any plans to remove ticks from the Zephyr and leave tickess as only supported mode?
Also, could you please elaborate a bit more about current problems in tickless mode? (you mentioned some few weeks ago, but without details)

The _update_time_slice_before_swap() function directly compared _time_slice_duration (expressed in ms) with value returned by _get_remaining_program_time() which used ticks as a time unit. Moreover, the _time_slice_duration was also used as an argument for _set_time(), which expects time expressed in ticks. This commit ensures that the same unit (ticks) is used in comparsion and timer adjustments. Signed-off-by: Piotr Zięcik <[email protected]>

The time slicing settings was kept in milliseconds while all related operations was based on ticks. Continuous back and forth conversion between ticks and milliseconds introduced an accumulating error due to rounding in _ms_to_ticks() and __ticks_to_ms(). As result configured time slice duration was not achieved. This commit removes excessive ticks <-> ms conversion by using ticks as time unit for all operations related to time slicing. Also, it fixes zephyrproject-rtos#8896 as well as zephyrproject-rtos#8897. Signed-off-by: Piotr Zięcik <[email protected]>

The value of sys_clock_ticks_per_sec is obtained using simple integer division with rounding toward zero. As result using this variable in _ms_to_ticks() introduces some error. This commit eliminates sys_clock_ticks_per_sec from equation used in _ms_to_ticks() removing introduced error. Also, this commit fixes zephyrproject-rtos#8895. Signed-off-by: Piotr Zięcik <[email protected]>

This commit replaces exact time compassion by a range check, allowing the tests to pass on platforms which needs rounding in __ticks_to_ms() and _ms_to_ticks(). Signed-off-by: Piotr Zięcik <[email protected]>

ioannisg · 2018-08-13T06:38:01Z

@pizi-nordic I am just wondering; does this PR overlap with #9137 ?

pizi-nordic · 2018-08-13T11:46:44Z

@ioannisg: As stated in the description:

Please note, that this PR is based on top of #9137.

:)

nashif · 2018-08-14T14:23:45Z

hmm, something went wrong, I forgot it was on top of other patches that were already merged, now commits appear twice!

pizi-nordic requested review from andrewboie and andyross as code owners July 23, 2018 12:20

pizi-nordic requested a review from ioannisg July 23, 2018 12:20

pizi-nordic force-pushed the do_not_use_sys_clock_ticks_per_sec_in_ms_to_ticks branch from 823649f to c0e9d4b Compare July 23, 2018 12:20

pizi-nordic changed the title ~~kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks()~~ [WIP] kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks() Jul 23, 2018

andyross approved these changes Jul 23, 2018

View reviewed changes

pizi-nordic added 2 commits August 2, 2018 13:39

pizi-nordic force-pushed the do_not_use_sys_clock_ticks_per_sec_in_ms_to_ticks branch from c0e9d4b to 1132fd5 Compare August 2, 2018 11:39

pizi-nordic requested review from nniranjhana, pswarnak and spoorthik as code owners August 2, 2018 11:39

pizi-nordic force-pushed the do_not_use_sys_clock_ticks_per_sec_in_ms_to_ticks branch from 1132fd5 to 249fd21 Compare August 2, 2018 13:19

pizi-nordic added 2 commits August 2, 2018 15:23

tests: kernel: Do not use exact time in timing checks.

178bf48

This commit replaces exact time compassion by a range check, allowing the tests to pass on platforms which needs rounding in __ticks_to_ms() and _ms_to_ticks(). Signed-off-by: Piotr Zięcik <[email protected]>

pizi-nordic force-pushed the do_not_use_sys_clock_ticks_per_sec_in_ms_to_ticks branch 2 times, most recently from 178bf48 to 262daf1 Compare August 2, 2018 14:02

pizi-nordic changed the title ~~[WIP] kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks()~~ kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks() Aug 3, 2018

This was referenced Aug 6, 2018

drivers: timer: nrf: Optimize k_cycle_get_32(). #9275

Merged

nRF52_PCA10040: Failing test_slice_reset #9310

Closed

nashif merged commit 5b3a7ed into zephyrproject-rtos:master Aug 14, 2018

pizi-nordic deleted the do_not_use_sys_clock_ticks_per_sec_in_ms_to_ticks branch August 20, 2018 12:41

pizi-nordic mentioned this pull request Sep 11, 2018

System timer handling with low-frequency timers #9904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks() #9073

kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks() #9073

pizi-nordic commented Jul 23, 2018 •

edited

Loading

codecov-io commented Jul 23, 2018 •

edited

Loading

pizi-nordic commented Jul 23, 2018

andyross commented Jul 23, 2018

pizi-nordic commented Jul 25, 2018

andyross commented Jul 25, 2018

pizi-nordic commented Jul 25, 2018

ioannisg commented Aug 13, 2018

pizi-nordic commented Aug 13, 2018 •

edited

Loading

nashif commented Aug 14, 2018

kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks() #9073

kernel: Do not use sys_clock_ticks_per_sec in _ms_to_ticks() #9073

Conversation

pizi-nordic commented Jul 23, 2018 • edited Loading

codecov-io commented Jul 23, 2018 • edited Loading

Codecov Report

pizi-nordic commented Jul 23, 2018

andyross commented Jul 23, 2018

pizi-nordic commented Jul 25, 2018

andyross commented Jul 25, 2018

pizi-nordic commented Jul 25, 2018

ioannisg commented Aug 13, 2018

pizi-nordic commented Aug 13, 2018 • edited Loading

nashif commented Aug 14, 2018

pizi-nordic commented Jul 23, 2018 •

edited

Loading

codecov-io commented Jul 23, 2018 •

edited

Loading

pizi-nordic commented Aug 13, 2018 •

edited

Loading