Skip to content
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.

Sign up for GitHub

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

lib: posix: nanosleep: fix nanosleep for sub microsecond durations #28542

Merged
merged 2 commits into from
Oct 2, 2020
Merged

lib: posix: nanosleep: fix nanosleep for sub microsecond durations #28542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f9a8d5
lib: posix: nanosleep: round up to the nearest microsecond
cfriedt Sep 20, 2020
80b8b6e
tests: posix: nanosleep: round up to the nearest microsecond
cfriedt Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions lib/posix/nanosleep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <errno.h>
/* required for struct timespec */
#include <posix/time.h>
#include <sys/util.h>
#include <sys_clock.h>

/**
Expand All @@ -20,6 +21,7 @@
int nanosleep(const struct timespec *rqtp, struct timespec *rmtp)
{
uint64_t ns;
uint64_t us;
const bool update_rmtp = rmtp != NULL;

if (rqtp == NULL) {
Expand All @@ -41,14 +43,17 @@ int nanosleep(const struct timespec *rqtp, struct timespec *rmtp)
/* If a user passes this in, we could be here a while, but
* at least it's technically correct-ish
*/
ns = rqtp->tv_nsec + NSEC_PER_SEC;
k_sleep(K_SECONDS(rqtp->tv_sec - 1));
ns = rqtp->tv_nsec + NSEC_PER_SEC
+ k_sleep(K_SECONDS(rqtp->tv_sec - 1)) * NSEC_PER_MSEC;
} else {
ns = rqtp->tv_sec * NSEC_PER_SEC + rqtp->tv_nsec;
}

/* currently we have no mechanism to achieve greater resolution */
k_busy_wait(ns / NSEC_PER_USEC);
/* TODO: improve upper bound when hr timers are available */
us = ceiling_fraction(ns, NSEC_PER_USEC);
do {
us = k_usleep(us);
} while (us != 0);

do_rmtp_update:
if (update_rmtp) {
Expand Down
12 changes: 9 additions & 3 deletions tests/posix/common/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,13 @@ extern void test_nanosleep_req_is_rem(void);
extern void test_nanosleep_n1_0(void);
extern void test_nanosleep_0_n1(void);
extern void test_nanosleep_n1_n1(void);
extern void test_nanosleep_0_1000000000(void);
extern void test_nanosleep_0_1(void);
extern void test_nanosleep_0_1001(void);
extern void test_nanosleep_0_1000000000(void);
extern void test_nanosleep_0_500000000(void);
extern void test_nanosleep_1_0(void);
extern void test_nanosleep_1_1(void);
extern void test_nanosleep_1_1001(void);

void test_main(void)
{
Expand All @@ -55,9 +58,12 @@ void test_main(void)
ztest_unit_test(test_nanosleep_n1_0),
ztest_unit_test(test_nanosleep_0_n1),
ztest_unit_test(test_nanosleep_n1_n1),
ztest_unit_test(test_nanosleep_0_1000000000),
ztest_unit_test(test_nanosleep_0_1),
ztest_unit_test(test_nanosleep_0_1001),
ztest_unit_test(test_nanosleep_0_500000000),
ztest_unit_test(test_nanosleep_1_0)
ztest_unit_test(test_nanosleep_1_0),
ztest_unit_test(test_nanosleep_1_1),
ztest_unit_test(test_nanosleep_1_1001)
);
ztest_run_test_suite(posix_apis);
}
64 changes: 31 additions & 33 deletions tests/posix/common/src/nanosleep.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,6 @@
#include <stdint.h>
#include <sys_clock.h>

/* TODO: Upper bounds check when hr timers are available */
#define NSEC_PER_TICK \
(NSEC_PER_SEC / CONFIG_SYS_CLOCK_TICKS_PER_SEC)
#define NSEC_PER_CYCLE \
(NSEC_PER_SEC / CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC)

/* Specify accepted tolerance. On some Zephyr platforms (e.g. nRF5x) the busy
* wait loop and the system timer are based on different mechanisms and may not
* align perfectly. 1 percent base intolerance is to cover CPU processing in the
* test.
*/
#if CONFIG_NRF_RTC_TIMER
/* High frequency clock used for k_busy_wait may have up to 8% tolerance.
* Additionally, if RC is used for low frequency clock then it has 5% tolerance.
*/
#define TOLERANCE_PPC \
(1 + 8 + (IS_ENABLED(CONFIG_CLOCK_CONTROL_NRF_K32SRC_RC) ? 5 : 0))
#else
#define TOLERANCE_PPC 1
#endif

/** req and rem are both NULL */
void test_nanosleep_NULL_NULL(void)
{
Expand Down Expand Up @@ -199,23 +178,30 @@ static void common(const uint32_t s, uint32_t ns)
zassert_equal(rem.tv_nsec, 0, "actual: %d expected: %d",
rem.tv_nsec, 0);

uint64_t actual_ns = k_cyc_to_ns_ceil64((now - then));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still approving, but this is where it's going to break: when we have a system where the cycle clock is slower than the CPU clock then we'll have to add a fudge factor for lower bounds that aren't met. That'll generally be board-specific; I think one of my reel_board's has exhibited that behavior in the past.

Copy link
Member Author

@cfriedt cfriedt Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, "break" is debatable. From the spec:

The suspension time may be longer than requested because the argument value is rounded up to an integer multiple of the sleep resolution or because of the scheduling of other activity by the system.

Which clearly applies here because the scheduler is triggered at least once.

The board I'm currently working on is also in the 32k club, so it's also nowhere near accurate.

I considered adding a negative value in rmtp to reflect the overrun w.r.t. rqtp but the Linux & macOS do not do that, and it seems that rmtp is only modified when nanosleep(2) is interrupted by a signal.

Ideally, a solution to #6498 would be rolled-out soon-ish and that should help to fix this issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, "break" is debatable. From the spec:

The suspension time may be longer than requested because the argument value is rounded up to an integer multiple of the sleep resolution or because of the scheduling of other activity by the system.

Let me try again:

Assume the CPU clock that's used for the nanosleep busy-wait runs at 10 MHz and is precise: there are 10^7 ticks in one second.

There is no access to this as a clock, so a different clock is used for the system clock that counts cycles. Assume it runs at 10 kHz and has a -500 ppm error: it ticks 9995 times in one second.

Then the duration measured for a 1 s sleep taking 10^7 CPU cycles will be 999500 us, which will appear to violate the spec because it's shorter than expected. The test will fail, even though the code does exactly what it's supposed to do. That's what I mean by "break".

This mostly shows up in Nordic because other platforms have synchronous CPU and cycle clocks, but in theory it can apply to any platform. The frequency tolerance for Nordic LFRC is 5%, and for HFRC is 8% so a 13% error between the CPU and cycle clocks can be observed. It's pretty likely at least one board in some test environment somewhere will display a negative ppm error and fail the test.

Copy link
Member Author

@cfriedt cfriedt Sep 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly shows up in Nordic because other platforms have synchronous CPU and cycle clocks, but in theory it can apply to any platform. The frequency tolerance for Nordic LFRC is 5%, and for HFRC is 8% so a 13% error between the CPU and cycle clocks can be observed. It's pretty likely at least one board in some test environment somewhere will display a negative ppm error and fail the test.

Thanks for clarifying. I think that is a definite concern when using one clock domain to try and synchronize a separate clock domain. That is not what is happening here. In the implementation, I took out the busy wait and opted to use k_usleep() instead, which synchronises in the same (albeit slow as heck) clock domain.

Assumption 1: k_usleep() does not use the cycle counter that k_busy_wait() uses.

Assumption 1 seems to be valid but please correct me if I'm wrong.

Assumption 2: calling k_usleep() for any non-zero duration would incur at least 1 expiration of the cycle clock

My assumption was, that by doing so, there would be a guarantee that the cycle clock must expire at least once (it is likely more than that).

It would appear that Assumption 2 also holds.

So I am skeptical that (now - then) will ever be a negative value in this test.

I realize that nanosleep is incredibly inaccurate this way, but it is both compliant with the spec and the tests do not break.

Given that Zephyr is not relying on nanosleep anywhere yet, I feel that the inaccuracy is a lesser evil than requiring all platforms to try and tune their clock-tolerances and to try and do something hackish in the tests. I like keeping things simple-ish.

I think the issue here, is that Nordic put a lot of work into trying to tune nanosleep based on clock tolerances, but it was difficult to get tests to pass consistently. Tests also do not pass consistently in qemu as well, or on other platforms I was using. Using clock tolerances is a bit of a slippery slope.

The posixly-correct way to fix this would be to provide access to a collection of clocks and their resolutions via clock_getres(), then call clock_nanosleep() using the clock with the best resolution.

Since we don't (yet) have clock_getres(), and I believe we do not have the ability to use an arbitrary clock to un-suspend a thread, and also since we do not have a calibrated per-cpu delay-loop, I'm ok with using k_usleep() until other more reliable and more accurate options are available.

I realize it's a lot to ask, but can you be ok with that as well for the next release?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd missed that the implementation had also changed away from busy-wait. I haven't seen anything here that makes me remove my existing approval, but we need @pfalcon to approve this.

For the other stuff, I just want to point to #19030 and its parent #19282. Clocks and time are very important to me.

uint64_t exp_ns = (uint64_t)s * NSEC_PER_SEC + ns;
uint64_t tolerance_ns = MAX(NSEC_PER_CYCLE,
(TOLERANCE_PPC * exp_ns) / 100U);
uint64_t tck_ns = k_cyc_to_ns_ceil64((now == then)
? 1U
: (now - then));
int64_t delta_ns = (int64_t)(exp_ns - tck_ns);

zassert_true((delta_ns < 0)
? ((uint64_t)-delta_ns <= tolerance_ns)
: ((uint64_t)delta_ns <= tolerance_ns),
"error %lld beyond tolerance %llu for %llu vs %llu",
delta_ns, tolerance_ns, exp_ns, tck_ns);
/* round up to the nearest microsecond for k_busy_wait() */
exp_ns = ceiling_fraction(exp_ns, NSEC_PER_USEC) * NSEC_PER_USEC;

/* lower bounds check */
zassert_true(actual_ns >= exp_ns,
"actual: %llu expected: %llu", actual_ns, exp_ns);

/* TODO: Upper bounds check when hr timers are available */
}

/** sleep for 1ns */
void test_nanosleep_0_1(void)
{
common(0, 1);
}

/** sleep for 1us + 1ns */
void test_nanosleep_0_1001(void)
{
common(0, 1001);
}

/** sleep for 500000000ns */
void test_nanosleep_0_500000000(void)
{
Expand All @@ -227,3 +213,15 @@ void test_nanosleep_1_0(void)
{
common(1, 0);
}

/** sleep for 1s + 1ns */
void test_nanosleep_1_1(void)
{
common(1, 1);
}

/** sleep for 1s + 1us + 1ns */
void test_nanosleep_1_1001(void)
{
common(1, 1001);
}