Page MenuHomeFreeBSD

FreeBSD callout rewrite and cleanup
ClosedPublic

Authored by hselasky on Jan 4 2015, 8:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 4:16 AM
Unknown Object (File)
Wed, Nov 13, 4:46 PM
Unknown Object (File)
Thu, Nov 7, 9:58 PM
Unknown Object (File)
Mon, Oct 28, 2:35 PM
Unknown Object (File)
Oct 26 2024, 8:15 PM
Unknown Object (File)
Oct 24 2024, 10:21 AM
Unknown Object (File)
Oct 23 2024, 7:31 PM
Unknown Object (File)
Oct 22 2024, 11:25 AM

Details

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.
  1. Shared lock support has been removed, because it prevents atomic stop of the callback function.
  1. A new API to drain callouts asynchronously has been added, called "callout_drain_async()".
  1. Updated timeout(9) manual page
  1. Optimise away "cc_bucket" and "cc_next" variables.
Test Plan

To debug spinlocks set:

debug.witness.skipspin=0

In /boot/loader.conf

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
hselasky added a subscriber: Unknown Object (MLST).

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

Integrate changes after r278469.

Update diff after r278623. No other changes.

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

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

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

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

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.

97

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

.Ft and s/structure//

107

.Ft and s/structure//

114

Possessive: application's

116

Comma:
functions, including the

121

s/Else/Otherwise,/

122

Avoid simultaneous access by obtaining an exclusive lock

132

Split this long sentence:

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

135

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

144

.Ft and s/structure//

149

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

argument is a non-zero pointer to a valid

153

The specified mutex is expected to be locked when calling any

155

functions other than the

159

functions.

165

The callout function is assumed to have released the specified

167

s/Else/Otherwise,/

173

This function is similar to the

175

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

176

s/initialised/initialized/

182

This function is similar to the

184

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

Should that be "read/write" instead?

188

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

199

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

223

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

243

s/Else/Otherwise,/

246

s/called/called,/

249

s/words/words,/

250

s/Else/Otherwise,/

273

s/Else/Otherwise,/

295

s/Else/Otherwise,/

309

.Fa pr
argument.

is redundant, and can be just

.Fa pr .

(There are lots of these.)

310

Avoid the informal use of "you":

This function is used when high precision timeouts are needed.

313

Again, redundant. Can be just

If
.Fa sbt
specifies...

324

"the" is not needed here.

334

Again, can just be

By default,
.Fa sbt
is treated...

Also, "is treated *as*".

346

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

362

This function works like

363

.Fn callout_reset_sbt ,

364

except the callback function given by

366

will be executed on the CPU which called this function.

375

Otherwise, the callback function given by

377

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

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

Refer to
.Fa callout_init .

Integrate manual page comments from Warren Block.

share/man/man9/timeout.9
98

Missing the word "if".

126

British->American: s/behaviour/behavior/

140

Add comma after "zero".

295

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

308

s/timeouts./timeouts are needed./

362

s/by/by the/

364

s/CPU/the CPU/

370

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

378

s/by the/by/

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

Integrate manual page comments from Warren Block.

Disallow task switching during hard tick interrupts.

Replace spinlock_enter/exit with KASSERT's.

MPSAFE callouts do not support CPU migration with this implementation

hselasky updated this object.

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.

Man page looks pretty good, thanks!

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

Rebase patch to latest FreeBSD-current.

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

This revision was automatically updated to reflect the committed changes.