Rewrite timeout(9) to be callout(9)-centric instead. Move the description
of timeout(9) to the end and mark it prominently as deprecated. Document
somewhat how times are specified for the 'sbt' variants. Better explain
how using callout_init_*() to associate a lock with a callout resolves
common races.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| share/man/man9/timeout.9 | ||
|---|---|---|
| 135 ↗ | (On Diff #1793) | A question on clarity: "not permitted" implies that something is monitoring the callout function, but I suspect it really means "these things will fail spectacularly if attempted in callout functions." |
| 174 ↗ | (On Diff #1793) | "respectively" is probably not needed here. It could be removed, or maybe replaced with "depending on the type of lock desired." |
| 179 ↗ | (On Diff #1793) | "it" is nonspecific. If the callout was cancelled, (Note: supposedly, "canceled" is the American spelling. It has always looked wrong to me, but...) |
| 181 ↗ | (On Diff #1793) | If the callout was not cancelled, |
| 184 ↗ | (On Diff #1793) | This does not seem to be needed and can be removed. |
| 208 ↗ | (On Diff #1793) | s/The following/These/ |
| 301 ↗ | (On Diff #1793) | s/a/the/ |
| 315 ↗ | (On Diff #1793) | s/interrupts/interrupts,/ |
| 317 ↗ | (On Diff #1793) | s/The following/These/ |
| 349 ↗ | (On Diff #1793) | Trailing whitespace on this line. |
| 350 ↗ | (On Diff #1793) | Trailing whitespace on this line. |
| 358 ↗ | (On Diff #1793) | "the time expires via the func argument" is potentially confusing. This sentence can be rearranged. The (Could use "identifies" rather than "specifies".) |
| 359 ↗ | (On Diff #1793) | Should? Or "must"? |
| 384 ↗ | (On Diff #1793) | s/Normally/Normally,/ |
| 500 ↗ | (On Diff #1793) | s/it's/its/ |
| 508 ↗ | (On Diff #1793) | This should be "indicates". But the whole sentence is not great. This is a little better: The return value from I'm not fond of asides, but that's really how this is including callout_reset and callout_schedule: The return value from |
Comment Actions
Various edits from Warren.
| share/man/man9/timeout.9 | ||
|---|---|---|
| 135 ↗ | (On Diff #1793) | When INVARIANTS are enabled the kernel will panic if you try to sleep. (The callout threads run with TD_NO_SLEEPING() enabled and the status of that flag is checked inside the sleep code via a KASSERT()), so it is monitored in that sense. Would you prefer "Callout functions must not sleep." instead? |
| 174 ↗ | (On Diff #1793) | Removed. |
| 179 ↗ | (On Diff #1793) | Done. My fingers do default to cancelled, but since you mentioned it I did a bit of research. It is true that I use fueled, not fuelled (and canceled is more inline with that than cancelled). That said, old habits die hard and I will leave all the double-l's here to be consistent. |
| 181 ↗ | (On Diff #1793) | Done. |
| 184 ↗ | (On Diff #1793) | Hmm, it is from an earlier edit. What I am trying to convey is that two things here form the guarantee stated in the last sentence of this paragraph (that the races are avoided):
You have to have both parts for the race to be avoided. The "In addition" was attempting to delineate the two parts (though it does read a bit awkwardly). Do you have a better suggestion of how to distinguish the two parts? |
| 208 ↗ | (On Diff #1793) | Done. |
| 301 ↗ | (On Diff #1793) | Done, though part of the reason I did that is that the precision is optional. (If you set it to zero it is similar to using ticks where there is no precision at all.) |
| 315 ↗ | (On Diff #1793) | I guess this is a subordinate clause? I can see that if I replace 'reducing' with 'thus reducing...' or 'which reduces...'. Without the helper word I don't see it. |
| 317 ↗ | (On Diff #1793) | Done. |
| 349 ↗ | (On Diff #1793) | Fixed. I believe both this and the next are bugs in the current page? |
| 350 ↗ | (On Diff #1793) | Fixed. |
| 358 ↗ | (On Diff #1793) | Done. I overuse specify, so identifies is a good alternative. |
| 359 ↗ | (On Diff #1793) | I copied this verbatim from the existing text for timeout(), but must is more correct. Done. |
| 384 ↗ | (On Diff #1793) | Done. |
| 385 ↗ | (On Diff #1793) | That is what the next few sentences seek to convey. I wanted to introduce the general case first before defining the exceptions. Perhaps it should be moved to the end (though that is a bit awkward as well). Hmm, I think I know how to fix this. I'll reword this quite a bit in the next update. |
| 465 ↗ | (On Diff #1793) | In an earlier thread on arch@, jmg@ found it confusing that this paragraph did not explicitly state that only one of the approaches was needed (he assumed he needed to use all three). I think some way to indicate that only one of the approaches should be used needs to be explicitly stated somehow. "One of the approaches can be used" doesn't read well (needs "following approaches" or "approaches below") |
| 466 ↗ | (On Diff #1793) | Done. |
| 467 ↗ | (On Diff #1793) | Done. |
| 481 ↗ | (On Diff #1793) | Done. |
| 500 ↗ | (On Diff #1793) | Argh, I usually don't confuse those. Done. |
| 508 ↗ | (On Diff #1793) | I should have caught the subject/verb disagreement. :( I think making the aside explicit is probably best. For some reason having it be 'or' instead of 'and' triggers something in my head as I don't want the reader to assume it's somehow optional behavior of callout_reset/schedule, but I know it doesn't actually say that. |
| share/man/man9/timeout.9 | ||
|---|---|---|
| 135 ↗ | (On Diff #1793) | Yes, or maybe add the specifics in your response. "Callout functions must not sleep. Callouts run with TD_NO_SLEEPING() enabled. If INVARIANTS is enabled in the kernel, a KASSERT() checks the status of that flag and panics if a callout attempts to sleep." ...or something like that. |
| 184 ↗ | (On Diff #1793) | It's a little unclear. The first part appears to be just how it works. It's not really a condition or anything the user does, just background information. To me, then, it looks like the user responsibility is all in the second sentence: "The callout subsystem locks the associated lock before the callout function is called, and thus can abort the callout without invoking the function. To avoid races, the calling code must hold the associated lock when it invokes callout_reset/schedule/stop." |
| 301 ↗ | (On Diff #1793) | It might be worth adding that as another sentence after this one. If the precision is set to zero, it is similar to using ticks where there is no precision at all. |
| 315 ↗ | (On Diff #1793) | I see it as needing a pause to actually speak that sentence. It could actually be split into two sentences. ...allows the callout subsystem to coalesce callouts scheduled close to each other into fewer timer interrupts. This reduces processing overhead and power consumption. |
| 349 ↗ | (On Diff #1793) | Yes. |
Comment Actions
I think there are two other open items:
- I rewrote the bit about callouts being assigned to CPUs.
- A suggestion on how best to convey that the 3 sample ways of avoiding races are independent and only one is needed while removing "following"
| share/man/man9/timeout.9 | ||
|---|---|---|
| 135 ↗ | (On Diff #1793) | Ok, I'll just go with "must not sleep". I think having all the other details in there is a bit wordy (or more that that sort of thing should be documented in some other more generic place) |
| 184 ↗ | (On Diff #1793) | That's close. One other wrinkle is that the API actually requires the user to hold the associated lock even if the caller doesn't care about the races. I'm worried someone might read your suggestion and think the locking is optional. It is very true that it starts out with "this is just how it works", and I want to keep that. After going around a few times in my head, I've shortened it a bit and reordered it. |
| 301 ↗ | (On Diff #1793) | Hmm, that is sort of implied below, where I talk about non-zero precision values. Also, the old ticks actually isn't quite the same now that I think about it. ticks counts use C_HARDCLOCK so they have a precision of roughly 1/hz seconds. I think I will just let this lie. |
Comment Actions
Sorry if I commented on an outdated version. My regard for phabricator wavers between "the greatest thing ever" and "an anti-UI inspired by github".
| share/man/man9/timeout.9 | ||
|---|---|---|
| 466 ↗ | (On Diff #1844) | "but be blocked" just looks wrong. "but is blocked" is better. |
| 469 ↗ | (On Diff #1844) | Two ways to handle this: "Any one of the following approaches..." "There are three main techniques for avoiding synchronization concerns." |
| 185 ↗ | (On Diff #1793) | Not sure about "must" there. Maybe a reordering: "Do not cancel or reschedule a callout without holding the associated lock." |
| 315 ↗ | (On Diff #1793) | This still needs a pause, or preferably a sentence break. |
Comment Actions
Yeah, the problem is that you want to reply to a previous comment, but the threads are "tied" to the version of the diff they start at. I've run into the same issue with other code review tools. :( What you really want is for the thread to "follow" the diffs, but I suspect that's hard to do (and hard to get the UI correct as well).
I think there are two things left:
- The description of CPU pinning that I rewrote. It is at lines 380 - 413 in the latest diff.
- The description of how associated locks work. That is at lines 175 - 184 in the latest diff.
| share/man/man9/timeout.9 | ||
|---|---|---|
| 466 ↗ | (On Diff #1844) | Agreed. |
| 469 ↗ | (On Diff #1844) | I went with the second (mostly, I kept "addressing these" instead of "avoiding") |
| 185 ↗ | (On Diff #1793) | The reason I used must is that this is a hard requirement (enforced via an assertion), so figured it qualified for "must" in the RFC sense. OTOH, I reordered this section in the most recent version based on the earlier feedback. It now states this requirement earlier before describing the the race, etc. |
| 315 ↗ | (On Diff #1793) | Ah, I did add a comma after interrupts in later edits. |
| share/man/man9/timeout.9 | ||
|---|---|---|
| 178 ↗ | (On Diff #1844) | If you say this out loud, a pause before the "and" indicates a comma. I'd say it that way, but maybe that's just me. |
| 181 ↗ | (On Diff #1844) | Same here, a pause before the "and". |
| 183 ↗ | (On Diff #1844) | And here, pausing before the "because". |
| 404 ↗ | (On Diff #1844) | "to which it is currently assigned." Yes, there are people who say ending sentences with a preposition is fine now. Maybe so, but it reads better the other way. |
| 406 ↗ | (On Diff #1844) | s/The softclock/Softclock/ |
| 410 ↗ | (On Diff #1844) | s/The softclock/Softclock/ |
| share/man/man9/timeout.9 | ||
|---|---|---|
| 178 ↗ | (On Diff #1844) | Here it is a compound predicate (bob drove and washed his car) rather than a compound sentence (bob drove his car, and joe washed the car). If It said 'and the callout subsystem releases' that would make it a compound sentence I think. |
| 181 ↗ | (On Diff #1844) | You are right. "The associated lock is released" would be its own sentence. |
| 183 ↗ | (On Diff #1844) | Hmm, I was taught that if the subordinate clause came first, you put a comma between it and the main clause, but if the main clause comes first, there is no comma between the two clauses. |
| 404 ↗ | (On Diff #1844) | That's much better, yes. |
| 406 ↗ | (On Diff #1844) | Done. |
| 410 ↗ | (On Diff #1844) | Done. |
| share/man/man9/timeout.9 | ||
|---|---|---|
| 183 ↗ | (On Diff #1844) | It is long. :( It also repeats that 'stopping or rescheduling' which makes it long. How about replacing this entire sentence with: "This ensures that stopping or rescheduling the callout will abort any I think that states the desired behavior much more succinctly and in a way that should link to the previous sentences. |
Comment Actions
[inline]
Overall, in quite good shape -- thank you!
| share/man/man9/timeout.9 | ||
|---|---|---|
| 314 ↗ | (On Diff #1881) | "The following" might be more clear than "These", which gives no context as to whether it is the preceding or following text being described. |
| 327 ↗ | (On Diff #1881) | comma before "similar"? |
| 330 ↗ | (On Diff #1881) | "the handler" |
| 696 ↗ | (On Diff #1881) | I think there should be a "to" in here somewhere. |
| 718 ↗ | (On Diff #1881) | The past tense may be better, here -- "must have been" |
| 780 ↗ | (On Diff #1881) | Weren't (un)timeout reimplemented in terms of callouts? |
| share/man/man9/timeout.9 | ||
|---|---|---|
| 183 ↗ | (On Diff #1844) | Yes, much better! |
| share/man/man9/timeout.9 | ||
|---|---|---|
| 314 ↗ | (On Diff #1881) | Hah! Warren asked me to change this from "The following" to "these". (It is "The following" in the current manpage) |
| 327 ↗ | (On Diff #1881) | Hmm, I guess this is an aside (you could put parentheses around this bit), so that makes sense. |
| 330 ↗ | (On Diff #1881) | "Run handler" reads fine to me, but "the handler" probably is better. I'm less certain of the latter suggestion though. You wouldn't say "lauch a rocket into the space" you would just say "launch a rocket into space". I think this is because "space" in this context is a specific entity, not a generic one (as opposed to "drive my car to the store" where "store" is generic). Hardware interrupt context is similarly a very specific thing rather than a generic placeholder for any one of several things. |
| 696 ↗ | (On Diff #1881) | Perhaps, though this is just the old text relocated. I've tried to avoid updating this since it will be removed before 11 ships. |
| 718 ↗ | (On Diff #1881) | Similar here (old text just moved to the bottom of the page) |
| 780 ↗ | (On Diff #1881) | Not really. The work referenced here was committed in r29680 and included 'struct callout'. The callout_* API was added later by Garrett Wollman in r44510 about 2 years later. This section is in need of updating though because I have no idea how much the sbintime changes diverged from the design in the cited technical report. |