Details

Reviewers

kib
jilles
ngie

Commits

rS315526: Add clock_nanosleep()

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)

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8096
Build 8299: arc lint + arc unit

Event Timeline

vangyzen created this revision.Mar 15 2017, 9:49 PM

Herald added a reviewer: ngie. · View Herald TranscriptMar 15 2017, 9:49 PM

Herald added a subscriber: badger. · View Herald Transcript

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.

vangyzen added a subscriber: ngie.Mar 15 2017, 10:42 PM

Please remove generated files from the diff in review.

lib/libc/sys/Symbol.map
515	Does _clock_nanosleep() export needed ?
lib/libc/sys/clock_nanosleep.c
3	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	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
597	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.

dchagin added a subscriber: dchagin.Mar 16 2017, 7:01 AM

vangyzen marked an inline comment as done.Mar 16 2017, 2:44 PM

vangyzen added inline comments.

lib/libc/sys/Symbol.map
515	There is no current need, but exporting it seemed reasonable. I can remove it, if you prefer.
lib/libc/sys/clock_nanosleep.c
3	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
597	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.

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.

Harbormaster completed remote builds in B8088: Diff 26324.Mar 16 2017, 4:55 PM

vangyzen added inline comments.Mar 16 2017, 4:57 PM

sys/compat/freebsd32/freebsd32_misc.c
2231	I'll refactor this, too.

vangyzen marked 2 inline comments as done.Mar 16 2017, 5:03 PM

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 :)!!!

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 marked an inline comment as done.Mar 16 2017, 5:38 PM

vangyzen added a subscriber: jilles.

vangyzen added inline comments.

lib/libc/sys/clock_nanosleep.c
3	When I move this to a Subversion tree, I'll be sure to copy from nanosleep.c.

vangyzen edited the test plan for this revision. (Show Details)Mar 16 2017, 5:40 PM

vangyzen edited the test plan for this revision. (Show Details)

@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.

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

vangyzen edited the test plan for this revision. (Show Details)Mar 16 2017, 6:00 PM

kib added inline comments.Mar 16 2017, 7:26 PM

lib/libc/sys/Symbol.map
515	I think it is better to not add unnecessary exports.
lib/libc/sys/clock_nanosleep.c
3	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	.Fo/.Fa allow to avoid too long line.
sys/compat/freebsd32/freebsd32_misc.c
2240	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	Same question about use of kern_posix_error().

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().

Harbormaster completed remote builds in B8096: Diff 26335.Mar 16 2017, 8:12 PM

vangyzen added inline comments.Mar 16 2017, 8:12 PM

sys/compat/freebsd32/freebsd32_misc.c
2240	I had not noticed kern_posix_error(). Thanks.

kib accepted this revision.Mar 16 2017, 8:47 PM

This revision is now accepted and ready to land.Mar 16 2017, 8:47 PM

jilles added inline comments.Mar 16 2017, 11:11 PM

contrib/netbsd-tests/lib/libc/sys/t_clock_nanosleep.c
49–53	Yes. `rmtp` should be written only in the case the function is interrupted by a signal.
include/time.h
171	Wrong section. The `clock_nanosleep()` function was added in Issue 7, i.e. `#if __POSIX_VISIBLE >= 200112`.
lib/libc/sys/nanosleep.2
104–107	`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	with a .Fa clock_id argument of .Dv CLOCK_REALTIME and a .Fa flags argument of 0.
126	The value indicating the interruption is always `EINTR`.
sys/compat/freebsd32/freebsd32_misc.c
2266	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	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	Writing to `*ua_rmtp` if the error is not `EINTR` is not POSIX-compliant but may be considered part of the `nanosleep` ABI.

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

This revision now requires review to proceed.Mar 17 2017, 6:09 PM

Harbormaster completed remote builds in B8106: Diff 26364.Mar 17 2017, 6:09 PM

I think I have addressed all outstanding requests.

vangyzen added inline comments.Mar 17 2017, 6:45 PM

sys/kern/kern_time.c
627	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.

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

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.

Return EINVAL for invalid flags.

Harbormaster completed remote builds in B8110: Diff 26374.Mar 17 2017, 11:04 PM

Good idea. Thanks for all the feedback.

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

jilles accepted this revision.Mar 17 2017, 11:29 PM

This revision is now accepted and ready to land.Mar 17 2017, 11:29 PM

ngie added inline comments.Mar 18 2017, 2:02 AM

sys/compat/freebsd32/freebsd32_misc.c
2266	style(9): `ua_rmtp != NULL`?
sys/kern/kern_time.c
536	style(9): `(flags & TIMER_ABSTIME) != 0`?
537–539	I wonder if this should be in the conditional... it's a bit redundant with the `if (flags & TIMER_ABSTIME) {` check.
623–627	style(9): `ua_rmtp != NULL`?
627	style(9): `ua_rmtp != NULL`?

vangyzen mentioned this in D10044: nanosleep: plug a kernel memory disclosure.Mar 18 2017, 1:48 PM

vangyzen mentioned this in rS315510: nanosleep: plug a kernel memory disclosure.Mar 18 2017, 8:16 PM

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.

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

This revision now requires review to proceed.Mar 19 2017, 12:39 AM

Herald added subscribers: ae, emaste. · View Herald TranscriptMar 19 2017, 12:39 AM

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

sys/kern/kern_time.c
537–539	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`.

Closed by commit rS315526: Add clock_nanosleep() (authored by vangyzen). · Explain WhyMar 19 2017, 12:51 AM

This revision was automatically updated to reflect the committed changes.

Add clock_nanosleep()
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 26335

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

include/time.h

lib/libc/include/libc_private.h

lib/libc/include/namespace.h

lib/libc/include/un-namespace.h

lib/libc/sys/Makefile.inc

lib/libc/sys/Symbol.map

lib/libc/sys/clock_nanosleep.c

lib/libc/sys/interposing_table.c

lib/libc/sys/nanosleep.2

lib/libc/tests/sys/Makefile

lib/libthr/thread/thr_private.h

lib/libthr/thread/thr_syscalls.c

sys/compat/freebsd32/freebsd32_misc.c

sys/compat/freebsd32/syscalls.master

sys/kern/kern_time.c

sys/kern/syscalls.master

sys/sys/syscallsubr.h

Add clock_nanosleep()ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 26335

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

include/time.h

lib/libc/include/libc_private.h

lib/libc/include/namespace.h

lib/libc/include/un-namespace.h

lib/libc/sys/Makefile.inc

lib/libc/sys/Symbol.map

lib/libc/sys/clock_nanosleep.c

lib/libc/sys/interposing_table.c

lib/libc/sys/nanosleep.2

lib/libc/tests/sys/Makefile

lib/libthr/thread/thr_private.h

lib/libthr/thread/thr_syscalls.c

sys/compat/freebsd32/freebsd32_misc.c

sys/compat/freebsd32/syscalls.master

sys/kern/kern_time.c

sys/kern/syscalls.master

sys/sys/syscallsubr.h

Add clock_nanosleep()
ClosedPublic
Actions

Revision Contents
Changeset List