Page MenuHomeFreeBSD
Differential D1438

FreeBSD callout rewrite and cleanup
ClosedPublic
Actions

Authored by • hselasky on Jan 4 2015, 8:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 28, 12:08 AM
Unknown Object (File)
Tue, Nov 26, 3:27 PM
Unknown Object (File)
Tue, Nov 26, 3:21 PM
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)
Oct 28 2024, 2:35 PM
Unknown Object (File)
Oct 26 2024, 8:15 PM
View All Files
Subscribers
avg
freebsd-net-list
jch
wblock

Details

Reviewers
kib
emaste
markj
neel
jhb
erj
delphij
rrs
imp
lstewart
gnn
sbruno
adrian
rwatson
Commits
rS282414: Major callout subsystem cleanup and rewrite:
rS277528: Revert for r277213:
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 set the repository for this revision to rS FreeBSD src repository - subversion.Feb 1 2015, 12:06 PM
• hselasky added a subscriber: Unknown Object (MLST).
• hselasky added a comment.Feb 1 2015, 2:30 PM

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

• hselasky updated this revision to Diff 3607.Feb 3 2015, 3:10 PM

Add full context to patch.

• hselasky updated this revision to Diff 3734.Feb 11 2015, 5:33 PM

Integrate changes after r278469.

• hselasky updated this revision to Diff 3809.Feb 17 2015, 10:57 AM

Update diff after r278623. No other changes.

wblock added a subscriber: wblock.Feb 17 2015, 11:28 PM

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 .

wblock added a reviewer: Doc Committers.Feb 17 2015, 11:28 PM
• hselasky updated this revision to Diff 3840.Feb 18 2015, 12:02 PM

Integrate manual page comments from Warren Block.

wblock added inline comments.Feb 20 2015, 2:06 AM
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 updated this revision to Diff 3868.Feb 20 2015, 9:09 AM

Integrate manual page comments from Warren Block.

• hselasky updated this revision to Diff 3896.Feb 21 2015, 4:29 PM

Disallow task switching during hard tick interrupts.

• hselasky updated this revision to Diff 3897.Feb 21 2015, 4:30 PM

Add context to diff.

• hselasky updated this revision to Diff 3922.Feb 23 2015, 8:50 AM

Replace spinlock_enter/exit with KASSERT's.

• hselasky updated this revision to Diff 3951.Feb 24 2015, 3:26 PM

MPSAFE callouts do not support CPU migration with this implementation

• hselasky updated this revision to Diff 3956.Feb 24 2015, 6:02 PM
• 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.

wblock added a comment.Feb 26 2015, 10:28 PM

Man page looks pretty good, thanks!

jch added a subscriber: jch.Apr 16 2015, 2:59 PM
• hselasky updated this revision to Diff 4869.Apr 17 2015, 7:36 AM
• hselasky set the repository for this revision to rS FreeBSD src repository - subversion.

Rebase patch to latest FreeBSD-current.

• hselasky updated this revision to Diff 4870.Apr 17 2015, 7:48 AM

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

• hselasky set the repository for this revision to rS FreeBSD src repository - subversion.Apr 17 2015, 7:49 AM
avg added a subscriber: avg.Apr 17 2015, 12:58 PM
eadler removed a reviewer: Doc Committers.Apr 30 2015, 1:03 AM
Closed by commit rS282414: Major callout subsystem cleanup and rewrite: (authored by • hselasky). · Explain WhyMay 4 2015, 9:34 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
kern/
3 lines
12 lines
6 lines
2 lines
13 lines
8 lines
1058 lines
147 lines
ofed/
include/
linux/
2 lines
sys/
19 lines
14 lines
5 lines

Diff 3440

View Options

kern/init_main.c

Loading...
View Options

kern/kern_condvar.c

Loading...
View Options

kern/kern_lock.c

Loading...
View Options

kern/kern_switch.c

Loading...
View Options

kern/kern_synch.c

Loading...
View Options

kern/kern_thread.c

Loading...
View Options

kern/kern_timeout.c

Loading...
View Options

kern/subr_sleepqueue.c

Loading...
View Options

ofed/include/linux/completion.h

Loading...
View Options

sys/_callout.h

Loading...
View Options

sys/callout.h

Loading...
View Options

sys/proc.h

Loading...