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

71–72

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–77

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.

77

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.

79

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

80

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

81

.Ft and s/structure//

85

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

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

90

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

94

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

function will return as if no timeout was pending.

95

.Ft and s/structure//

102

Possessive: application's

104

Comma:
functions, including the

109

s/Else/Otherwise,/

110

Avoid simultaneous access by obtaining an exclusive lock

116

Split this long sentence:

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

116

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

argument is a non-zero pointer to a valid

119

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

119

.Ft and s/structure//

120

The specified mutex is expected to be locked when calling any

122

functions other than the

148

functions.

168–172

The callout function is assumed to have released the specified

170

s/Else/Otherwise,/

196

This function is similar to the

197

This function is similar to the

198

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

199

s/initialised/initialized/

199

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

Should that be "read/write" instead?

203

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

207

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

231

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

251

s/Else/Otherwise,/

254

s/called/called,/

257

s/words/words,/

258

s/Else/Otherwise,/

281

s/Else/Otherwise,/

303

s/Else/Otherwise,/

311

.Fa pr
argument.

is redundant, and can be just

.Fa pr .

(There are lots of these.)

311

Again, redundant. Can be just

If
.Fa sbt
specifies...

312

Avoid the informal use of "you":

This function is used when high precision timeouts are needed.

323–324

"the" is not needed here.

332

Again, can just be

By default,
.Fa sbt
is treated...

Also, "is treated *as*".

344

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

360

This function works like

361

.Fn callout_reset_sbt ,

362

except the callback function given by

363–364

will be executed on the CPU which called this function.

367

Refer to
.Fa callout_init .

372

Otherwise, the callback function given by

374

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?

379

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 .

[• 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.