FreeBSD callout rewrite and cleanup

[• hselasky]

Summary

Update callout clients in the kernel area to use the callout API properly, like cv_timedwait(). Previously there was some custom sleepqueue code in the callout subsystem. All of that has now been removed and we allow callouts to be protected by spinlocks. This allows us to tear down the callback like done with regular mutexes, and a "td_slpmutex" has been added to "struct thread" to atomically teardown the "td_slpcallout". Further the "TDF_TIMOFAIL" and "SWT_SLEEPQTIMO" states can now be completely removed.

Summary of changes:

1. Make consistent callout API which also supports spinlocks for the callback function. This has been done to allow atomic callout stop of "td_slpcallout" without the need of many kernel threading quirks.

2. Shared lock support has been removed, because it prevents atomic stop of the callback function.

3. A new API to drain callouts asynchronously has been added, called "callout_drain_async()".

4. Updated timeout(9) manual page

5. Optimise away "cc_bucket" and "cc_next" variables.

Test Plan

To debug spinlocks set:

debug.witness.skipspin=0

In /boot/loader.conf

[• hselasky]

The last patch also corrects the first argument for the "cpu_new_callout()" function.

[• hselasky]

Add full context to patch.

[• hselasky]

Integrate changes after r278469.

[• hselasky]

Update diff after r278623. No other changes.

[wblock]

This is as much as I have time for at the moment. I'll add doc to the reviewers list

share/man/man9/timeout.9

73 ↗

(On Diff #3809)

That is kind of a difficult sentence to parse. Does this retain the meaning?

API is used to schedule a one-time call to an arbitrary function at a specific
future time.

76 ↗

(On Diff #3809)

The original is an "aside":

Consumers of this API are required to allocate a callout structure (struct callout) for each pending function invocation.

The parentheses are probably the wrong markup for the new form, and "struct callout structure" is redundant. Maybe this:

Consumers of this API are required to allocate a
.Ft struct callout
for each pending function invocation.

79 ↗

(On Diff #3809)

As above, use .Ft and the word "structure" is redundant.

80 ↗

(On Diff #3809)

"should be" is a recommendation. Does it mean I ought to call those functions before freeing, but will work fine without it? Otherwise, consider "must be drained by a call to".

85 ↗

(On Diff #3809)

The rule is to use American English spellings unless the entire document was written with British spellings. See
https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/writing-style-guidelines.html

88 ↗

(On Diff #3809)

It would be better to say
.Pq Deprecated, see blahblah for current usage.
This function is used to prepare a

90 ↗

(On Diff #3809)

Use .Ft and remove the redundant word "structure".

94 ↗

(On Diff #3809)

"Was" is a little odd here, but "were" is not quite right either. How about:

function will return as if no timeout was pending.

97 ↗

(On Diff #3809)

Again, better to say the function is deprecated and give a pointer to the replacement in a separate sentence. Then explain what this one does in the next sentence.

101 ↗

(On Diff #3809)

.Ft and s/structure//

107 ↗

(On Diff #3809)

.Ft and s/structure//

114 ↗

(On Diff #3809)

Possessive: application's

116 ↗

(On Diff #3809)

Comma:
functions, including the

121 ↗

(On Diff #3809)

s/Else/Otherwise,/

122 ↗

(On Diff #3809)

Avoid simultaneous access by obtaining an exclusive lock

132 ↗

(On Diff #3809)

Split this long sentence:

function is called.
It is also expected, but currently not

135 ↗

(On Diff #3809)

This last part needs to be split out into a separate sentence, starting with ", except when the".

144 ↗

(On Diff #3809)

.Ft and s/structure//

149 ↗

(On Diff #3809)

Passive "should be" and should, and "specify a pointer"... How about:

argument is a non-zero pointer to a valid

153 ↗

(On Diff #3809)

The specified mutex is expected to be locked when calling any

155 ↗

(On Diff #3809)

functions other than the

159 ↗

(On Diff #3809)

functions.

165 ↗

(On Diff #3809)

The callout function is assumed to have released the specified

167 ↗

(On Diff #3809)

s/Else/Otherwise,/

173 ↗

(On Diff #3809)

This function is similar to the

175 ↗

(On Diff #3809)

function, but accepts a read-mostly type of lock.

176 ↗

(On Diff #3809)

s/initialised/initialized/

182 ↗

(On Diff #3809)

This function is similar to the

184 ↗

(On Diff #3809)

function, but accepts a reader-writer type of lock.

Should that be "read/write" instead?

188 ↗

(On Diff #3809)

As above, separate sentence to say it is deprecated and point to the replacement.

199 ↗

(On Diff #3809)

argument is a valid pointer to a function that takes a single

223 ↗

(On Diff #3809)

"canceled" is spelled "cancelled" elsewhere in this man page. The first is modern American.

243 ↗

(On Diff #3809)

s/Else/Otherwise,/

246 ↗

(On Diff #3809)

s/called/called,/

249 ↗

(On Diff #3809)

s/words/words,/

250 ↗

(On Diff #3809)

s/Else/Otherwise,/

273 ↗

(On Diff #3809)

s/Else/Otherwise,/

295 ↗

(On Diff #3809)

s/Else/Otherwise,/

309 ↗

(On Diff #3809)

.Fa pr
argument.

is redundant, and can be just

.Fa pr .

(There are lots of these.)

310 ↗

(On Diff #3809)

Avoid the informal use of "you":

This function is used when high precision timeouts are needed.

313 ↗

(On Diff #3809)

Again, redundant. Can be just

If
.Fa sbt
specifies...

324 ↗

(On Diff #3809)

"the" is not needed here.

334 ↗

(On Diff #3809)

Again, can just be

By default,
.Fa sbt
is treated...

Also, "is treated *as*".

346 ↗

(On Diff #3809)

Needs a serial comma before "and":
...1/4, and so on.

362 ↗

(On Diff #3809)

This function works like

363 ↗

(On Diff #3809)

.Fn callout_reset_sbt ,

364 ↗

(On Diff #3809)

except the callback function given by

366 ↗

(On Diff #3809)

will be executed on the CPU which called this function.

375 ↗

(On Diff #3809)

Otherwise, the callback function given by

377 ↗

(On Diff #3809)

will be executed on the same CPU...

The rest of that line is unclear. As shown previously in the man page? As run previously on the CPU?

382 ↗

(On Diff #3809)

This function works like
.Fn callout_reset_sbt ,
except the callback function given by
.Fa func
will be executed on the CPU given by
.Fa cpu .

392 ↗

(On Diff #3809)

Refer to
.Fa callout_init .

[• hselasky]

Integrate manual page comments from Warren Block.

[wblock]

share/man/man9/timeout.9

98 ↗

(On Diff #3840)

Missing the word "if".

126 ↗

(On Diff #3840)

British->American: s/behaviour/behavior/

140 ↗

(On Diff #3840)

Add comma after "zero".

295 ↗

(On Diff #3840)

Use "cannot". "can not" with a space means "not doing it is allowed".

308 ↗

(On Diff #3840)

s/timeouts./timeouts are needed./

362 ↗

(On Diff #3840)

s/by/by the/

364 ↗

(On Diff #3840)

s/CPU/the CPU/

370 ↗

(On Diff #3840)

If this means "CPU selection cannot be changed while the callout subsystem is processing the callback for completion.", then s/can not/cannot/.

378 ↗

(On Diff #3840)

s/by the/by/

Either "given by the func argument will" or "given by func will". The second is less wordy,

[• hselasky]

Integrate manual page comments from Warren Block.

[• hselasky]

Disallow task switching during hard tick interrupts.

[• hselasky]

Add context to diff.

[• hselasky]

Replace spinlock_enter/exit with KASSERT's.

[• hselasky]

MPSAFE callouts do not support CPU migration with this implementation

[• hselasky]

Restore how callout_lock() worked, so that a callout can migrate at any time regardless of callout type.
Update manual page to reflect this change.
Use an unsigned integer for callout buckets.
The temporary LIST used by the callout code only needs to be initialized once at startup.

[wblock]

Man page looks pretty good, thanks!

[• hselasky]

Rebase patch to latest FreeBSD-current.

[• hselasky]

Revert more chunks as part of integration latest 11-current changes.