Add clock_nanosleep()

[vangyzen]

Summary

Add a clock_nanosleep() syscall, as specified by POSIX.
Make nanosleep() a wrapper around it.

Attach the clock_nanosleep test from NetBSD. Adjust it for the
FreeBSD behavior of updating rmtp only when interrupted by a signal.
I believe this to be POSIX-compliant, since POSIX mentions the rmtp
parameter only in the paragraph about EINTR. This is also what
Linux does. (NetBSD updates rmtp unconditionally.)

Copy the whole nanosleep.2 man page from NetBSD because it is complete
and closely resembles the POSIX description. Edit, polish, and reword it
a bit, being sure to keep any relevant text from the FreeBSD page.

Test Plan

root@bruise:~ # kyua test -k /usr/tests/lib/libc/sys/Kyuafile nanosleep_test
nanosleep_test:nanosleep_basic  ->  passed  [0.006s]
nanosleep_test:nanosleep_err  ->  passed  [0.005s]
nanosleep_test:nanosleep_sig  ->  passed  [1.079s]

Results file id is usr_tests_lib_libc_sys.20170316-174007-527263
Results saved to /root/.kyua/store/results.usr_tests_lib_libc_sys.20170316-174007-527263.db

3/3 passed (0 failed)

root@bruise:~ # kyua test -k /usr/tests/lib/libc/sys/Kyuafile clock_nanosleep_test
clock_nanosleep_test:clock_nanosleep_remain  ->  passed  [0.007s]

Results file id is usr_tests_lib_libc_sys.20170316-174018-328065
Results saved to /root/.kyua/store/results.usr_tests_lib_libc_sys.20170316-174018-328065.db

1/1 passed (0 failed)

root@bruise:~ # kyua test -k /usr/tests/lib/libc/gen/Kyuafile sleep_test
sleep_test:kevent  ->  passed  [27.195s]
sleep_test:nanosleep  ->  expected_failure: Long reschedule latency due to PR kern/43997: Reschedule latency 9018190363211123789 exceeds allowable fuzz 40000000  [0.055s]
sleep_test:select  ->  passed  [37.531s]
sleep_test:sleep  ->  passed  [29.170s]

Results file id is usr_tests_lib_libc_gen.20170316-175553-841747
Results saved to /root/.kyua/store/results.usr_tests_lib_libc_gen.20170316-175553-841747.db

4/4 passed (0 failed)

[vangyzen]

• Git somehow got confused by the similarity of these two files and swapped their contents. Fix that.

• Also, give the copyright on clock_nanosleep.c to the FreeBSD Foundation, since the file is mostly the same as nanosleep.c.

[kib]

Please remove generated files from the diff in review.

lib/libc/sys/Symbol.map

515 ↗

(On Diff #26293)

Does _clock_nanosleep() export needed ?

lib/libc/sys/clock_nanosleep.c

2 ↗

(On Diff #26293)

I do not see why ff is the copyright owner for this file.

Also, could you create the file from some more appropriate .c in sys ?

lib/libc/sys/interposing_table.c

45 ↗

(On Diff #26293)

Interposing table should only grow, at the end. We do not allow mix of libc and libthr from different revisions, but keeping this discipline allows new libc and old libthr mostly work. This might be critical during system upgrades, esp. using make installworld method.

sys/kern/kern_time.c

593 ↗

(On Diff #26293)

Why kern_clock_nanosleep() cannot be used ? Please do not call sys_XXX() from other syscalls. If you want to save code duplication for copyin of timespecs, then another wrapper is much better then sys_XXX call.

[vangyzen]

lib/libc/sys/Symbol.map

515 ↗

(On Diff #26293)

There is no current need, but exporting it seemed reasonable. I can remove it, if you prefer.

lib/libc/sys/clock_nanosleep.c

2 ↗

(On Diff #26293)

I copied it from nanosleep.c, of course. This is just poor inference by Phabricator.

This is also the reason for the FF copyright: The file is not sufficiently different for me to claim copyright.

sys/kern/kern_time.c

593 ↗

(On Diff #26293)

As you suspected, I did this to reuse the copyin/useracc/copyout code. I wondered if someone would find it distasteful. I'll refactor a wrapper.

[vangyzen]

• Revert auto-generated files for code review.
• Grow the interposing table at the end, for compat/upgrades.
• Refactor copyin/out code into user_clock_nanosleep(), called from sys_FOO.

[vangyzen]

sys/compat/freebsd32/freebsd32_misc.c

2229 ↗

(On Diff #26324)

I'll refactor this, too.

[ngie]

The test and user space pieces seem good, modulo statements from kib@. I'll look at the kern pieces soon.

Can you please:

• demo the test output?
• add jilles to double check the whole clock_nanosleep behavior difference between POSIX, Linux, and netbsd?

Thanks :)!!!

[vangyzen]

• Fix whitespace
• Do not update remaining time for absolute timeouts
• Finish writing freebsd32_clock_nanosleep(), which was ignoring the clock_id and flags arguments. Oops.
• Create freebsd32_user_clock_nanosleep() to match user_clock_nanosleep().
• Fix the size argument to useracc(), which was wrong before I started.

[vangyzen]

lib/libc/sys/clock_nanosleep.c

2 ↗

(On Diff #26293)

When I move this to a Subversion tree, I'll be sure to copy from nanosleep.c.

[vangyzen]

@jilles: @ngie thought you would be interested in the behavior difference from NetBSD, as mentioned in the Summary. Of course, I welcome comments from anyone on anything.

[vangyzen]

I think I have addressed all comments so far, although some are still open for discussion. Let me know if I missed anything.

[kib]

lib/libc/sys/Symbol.map

515 ↗

(On Diff #26293)

I think it is better to not add unnecessary exports.

lib/libc/sys/clock_nanosleep.c

2 ↗

(On Diff #26293)

Still ff did nothing to this file, esp. in 2017. Please put it under your name. Whether to leave 2014 ff copyright, is up to you.

lib/libc/sys/nanosleep.2

44 ↗

(On Diff #26327)

.Fo/.Fa allow to avoid too long line.

sys/compat/freebsd32/freebsd32_misc.c

2240 ↗

(On Diff #26327)

Blank line is needed after '{'.

Wouldn't kern_posix_error() function more correct there ? Both ktrace and dtrace would benefit, it seems.

sys/kern/kern_time.c

607 ↗

(On Diff #26327)

Same question about use of kern_posix_error().

[vangyzen]

Code review feedback:

• Do not export _clock_nanosleep, since it is not needed.
• Assign copyright on a new file to myself.
• Use .Fo/.Fc to wrap long lines.
• Use kern_posix_error().

[vangyzen]

sys/compat/freebsd32/freebsd32_misc.c

2240 ↗

(On Diff #26327)

I had not noticed kern_posix_error(). Thanks.

[jilles]

contrib/netbsd-tests/lib/libc/sys/t_clock_nanosleep.c

49–53 ↗

(On Diff #26335)

Yes. rmtp should be written only in the case the function is interrupted by a signal.

include/time.h

171 ↗

(On Diff #26335)

Wrong section. The clock_nanosleep() function was added in Issue 7, i.e. #if __POSIX_VISIBLE >= 200112.

lib/libc/sys/nanosleep.2

104–107 ↗

(On Diff #26335)

CLOCK_VIRTUAL and CLOCK_PROF are not accepted here but are accepted by clock_gettime() and listed in its man page.

CPU time clocks (CLOCK_PROCESS_CPUTIME_ID, CLOCK_THREAD_CPUTIME_ID and IDs returned by clock_getcpuclockid() and pthread_getcpuclockid()) are also accepted by clock_gettime() but not listed in its man page.

I think it is better to describe the supported clocks for this function here.

113–116 ↗

(On Diff #26335)

with a
.Fa clock_id
argument of
.Dv CLOCK_REALTIME
and a
.Fa flags
argument of 0.

126 ↗

(On Diff #26335)

The value indicating the interruption is always EINTR.

sys/compat/freebsd32/freebsd32_misc.c

2266 ↗

(On Diff #26335)

Writing to *ua_rmtp if the error is not EINTR is not POSIX-compliant but may be considered part of the nanosleep ABI.

sys/kern/kern_time.c

530 ↗

(On Diff #26335)

What POSIX seems to intend is EINVAL for clock IDs that are truly invalid and the CPU clock of the calling thread and ENOTSUP for clock IDs that are known but not supported by this function.

The CPU clock of the calling thread matches both criteria and therefore both errors are permitted.

This is actually a bit nasty since it requires knowing valid clock IDs just for an error number. I don't think it is very important.

627 ↗

(On Diff #26335)

Writing to *ua_rmtp if the error is not EINTR is not POSIX-compliant but may be considered part of the nanosleep ABI.

[vangyzen]

• Move clock_nanosleep declaration from POSIX 1993 to 2001
• Various man page updates
• Fix POSIX compliance of updating rmtp

[vangyzen]

I think I have addressed all outstanding requests.

[vangyzen]

sys/kern/kern_time.c

627 ↗

(On Diff #26335)

On second thought, I'm going to revert the change that added TIMER_NANOSLEEP to address this comment. In head, the only error on which nanosleep() non-compliantly updates rmtp is EINVAL. In that case, kern_nanosleep() has not updated rmt, so sys_nanosleep() updates the user-space rmtp by copying garbage from its stack frame. This behavior is obviously useless, so instead of preserving it, I'm going to make nanosleep() compliant.

[vangyzen]

• Revert "Fix POSIX compliance of updating rmtp"
• Update rmtp only on EINTR

[jilles]

Hmm, I thought of something else. It would help future extensions and making applications correct if an invalid flags argument (that is, not 0 and not TIMER_ABSTIME) returns error EINVAL instead of behaving like one of the two.

It looks good otherwise and I look forward to having this call available.

[vangyzen]

• Return EINVAL for invalid flags.

[vangyzen]

Good idea. Thanks for all the feedback.

[vangyzen]

• Update man page for invalid flags == EINVAL; try to clarify ENOTSUP

[ngie]

sys/compat/freebsd32/freebsd32_misc.c

2266 ↗

(On Diff #26375)

style(9): ua_rmtp != NULL?

sys/kern/kern_time.c

536 ↗

(On Diff #26375)

style(9): (flags & TIMER_ABSTIME) != 0?

537–539 ↗

(On Diff #26375)

I wonder if this should be in the conditional... it's a bit redundant with the if (flags & TIMER_ABSTIME) { check.

625 ↗

(On Diff #26375)

style(9): ua_rmtp != NULL?

629 ↗

(On Diff #26375)

style(9): ua_rmtp != NULL?

[jilles]

Since you have not committed this yet, another request here: since you made clock_nanosleep() a cancellation point (correctly), please add it to the list in share/man/man3/pthread_testcancel.3.

[vangyzen]

• merge from head
• style(9) fixes suggested by ngie@
• mention clock_nanosleep in pthread_testcancel.3

[vangyzen]

The previous diff was against a stale master branch,
so it included extraneous changes.

sys/kern/kern_time.c

537–539 ↗

(On Diff #26375)

It's half redundant, but the other half is necessary. Here, we already know the timeout is absolute, so I'm testing is_abs_real to see if it's based on the real-time clock. If so, I set rd_rtcgen so the thread will be woken if the RTC is adjusted. Since I already have is_abs_real for line 561, I just use it here, instead of adding something like bool is_realtime.