Rewrite time-bound sleeps use of callouts
ClosedPublic
Actions

Authored by kib on Jul 6 2016, 12:00 PM.

Details

Reviewers

emaste
jhb

Group Reviewers

manpages

Commits

rS303926: MFC r303426:
rS303426: Rewrite subr_sleepqueue.c use of callouts to not depend on the

Summary

Due to continuous rototiling of the callout_stop(9) KPI and continuous breakage of features required by the sleepq code, I decided to rewrite subr_sleepqueue.c use of callouts to not depend on the specifics of callout KPI.

The main change is that instead of requiring precise callouts, code maintains absolute time to wake up. Callouts now should ensure that a wake occurs at the requested monent, but we can tolerate both run-away callout, and callout_stop(9) lying about running callout either way.

I was not sure about the 'nested sleep', which is mentioned in kern_synch.c. For now, I explicitely assert that previous wake time was invalidated when new time-bound sleep is initiated. So far, the assert was never fired in the tests.

Test Plan

Peter Holm did generic testing, also he performed the tests to ensure that usermode sleeps behave sane (i.e. no early wakeups in absence of signals) and that the distribution of over-time is same for old and new code. See https://people.freebsd.org/~pho/select4/.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib updated this revision to Diff 18187.Jul 6 2016, 12:00 PM

kib retitled this revision from to Rewrite time-bound sleeps use of callouts.

kib updated this object.

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

kib added reviewers: jhb, emaste.

kib set the repository for this revision to rS FreeBSD src repository - subversion.

kib added a subscriber: pho.

Herald added a subscriber: imp. · View Herald TranscriptJul 6 2016, 12:00 PM

The nested sleep is bogus and should just be asserted away. I don't think it can happen.

The only other requests would be to document callout_when() and C_PRECALC.

sys/kern/subr_sleepqueue.c
604 ↗	(On Diff #18187)	To be clear, this does require 'td' to be type-stable (which it already is). Not sure if that warrants a comment? You could in theory make the clearing of TDF_TIMEOUT and _callout_stop_safe() calls unconditional? Also, it looks like this can now just be a plain 'callout_stop()'?

jhb accepted this revision.Jul 26 2016, 4:43 PM

jhb edited edge metadata.

This revision is now accepted and ready to land.Jul 26 2016, 4:43 PM

Added the callout_when(9) doc as well.

sys/kern/subr_sleepqueue.c
604 ↗	(On Diff #18187)	I added the comment. I do not want the unconditional call to callout_stop() because of the callout resources used, e.g. the callout lock is taken even for non-pending callout. And since TDF_TIMEOUT is checked anyway, it is slightly prudent to not clear the flag.

Document callout_when(9).

Change callout_when(9) interface to include precision. In the previous version of the patch, callout_reset_on() used the calculated absolute time for precision extraction, instead of relative timeout.

Remove comment for removed TDF_TIMOFAIL flag.

This revision now requires review to proceed.Jul 27 2016, 7:16 AM

Herald added a reviewer: manpages. · View Herald TranscriptJul 27 2016, 7:16 AM

jhb accepted this revision.Jul 27 2016, 4:36 PM

jhb edited edge metadata.

jhb added inline comments.

share/man/man9/timeout.9
401 ↗	(On Diff #18787)	Perhaps reword the second sentence as: The .Fa sbt and .Fa pr values should be calculated by an earlier call to .Fn callout_when which uses the user-supplied .Fa sbt , .Fa pr , and .Fa flags values.
531 ↗	(On Diff #18787)	No need for commas after 'run' and 'time'.
544 ↗	(On Diff #18787)	s/Resulted/Resulting/
sys/kern/kern_thread.c
386 ↗	(On Diff #18787)	I wonder if this drain in fact means the threads do not require type-safety for this particular change (sorry I missed that previously).

This revision is now accepted and ready to land.Jul 27 2016, 4:36 PM

kib marked 3 inline comments as done.Jul 27 2016, 4:49 PM

kib added inline comments.

sys/kern/kern_thread.c
386 ↗	(On Diff #18787)	It is only done for non-last thread of the process. So even if the type-safety is not needed, the re-use of the process and thread might result in the timeout from the previous allocation to affect current sleep.

jhb editing of the man page text.

This revision now requires review to proceed.Jul 27 2016, 4:52 PM

Herald edited edge metadata. · View Herald TranscriptJul 27 2016, 4:52 PM

bjk added a subscriber: bjk.Jul 27 2016, 5:37 PM

bjk added inline comments.

share/man/man9/timeout.9
127 ↗	(On Diff #18819)	Probably better to use Fo/Fa/Fc than a continuation line, even though the previous ones do so.
397 ↗	(On Diff #18819)	"the absolute time"
400 ↗	(On Diff #18819)	I think there should be an article before "non-adjustable", but am not sure I understand what is going on so I can't say which article. Is the "Non-adjustable requested precision" just meaning that the callout system isn't allowed to be sloppy when scheduling the event? Ah, a later chunk of the diff helps make things clearer. How about "the pr argument specifies the requested precision, which will not be adjusted during the scheduling process."?
544 ↗	(On Diff #18819)	.Fn
546 ↗	(On Diff #18819)	"The resulting".
548 ↗	(On Diff #18819)	"the resulting" again.