Details
- Reviewers
kib jhb wblock - Group Reviewers
manpages - Commits
- rS297394: Reword descriptions of asserting locks held without WITNESS.
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Yeah I am going to clean this and rwlock(9) up. I wanted to ensure my logic was right here.
I like the opening sentence in rwlock(9). I think it sets the stage to then explain the exceptions to assertions in a non-witness kernel. The sentence is:
If WITNESS is not included in the kernel, then it is impossible to assert that the current thread does or does not hold a read lock.
I think listing that first makes it easier to understand the following text. I would probably try to match the text about SA_LOCKED and SA_SLOCKED ( so call it the "non-WITNESS case" perhaps). Maybe even best if this is at the end so you get:
If WITNESS is not included in the kernel, then it is impossible to assert that the current thread does or does not hold a shared lock. In the non-WITNESS case, the SA_LOCKED and SA_SLOCKED assertions merely check that some thread holds a shared lock. They do not ensure that the current thread holds a shared lock. Similarly, SA_UNLOCKED is only able to assert that the current thread does not hold an exclusive lock in the non-WITNESS case. It is not able to assert that the current thread does not hold a shared lock.
I like this overall approach. Some notes below:
If WITNESS is not included in the kernel, then it is impossible to assert that the current thread does or does not hold a shared lock. In the
How about "whether the current thread holds a shared lock." as a shorter way to say this?
non-WITNESS case, the SA_LOCKED and SA_SLOCKED assertions merely check that some thread holds a shared lock. They do not ensure that the current
s/that some/whether some/
thread holds a shared lock. Similarly, SA_UNLOCKED is only able to assert that the current thread does not hold an exclusive lock in the non-WITNESS case.
That sentence makes my head hurt, mostly by the last part. Can that be reordered for clarity?
"Similarly, SA_UNLOCKED with a non-WITNESS kernel can only assert that the current thread does not hold an exclusive lock."
It
is not able to assert that the current thread does not hold a shared lock.
Clarify more from feedback, use the same format as rwlock.9 and add the note
about RA_UNLOCKED only checking write lock.
| share/man/man9/sx.9 | ||
|---|---|---|
| 327 ↗ | (On Diff #14426) | This sentence IMO should put an emphasis on the 'current thread'. We do see that shared lock is taken, we just do not know who took it. This is clearly formulated later with some/any adj, but not there. Might be, 'it is impossible to assert which thread hold a shared lock'. |
A less redundant and probably clearer way to do this would be to split the two cases (WITNESS and non-WITNESS) into two sections.
With witness in the kernel,
(list of what happens with assertions when WITNESS is in the kernel.
When
.Dv WITNESS
has not been included in the kernel, it is impossible to assert which thread holds
a shared lock, and the
.Dv SA_LOCKED
and
.Dv SA_SLOCKED
assertions merely check whether any thread holds a shared lock.
| share/man/man9/sx.9 | ||
|---|---|---|
| 327 ↗ | (On Diff #14426) | I have several ideas here for this sentence. Using rwlock.9 for examples. I believe you were suggesting: "If WITNESS is not included in the kernel, then it is impossible to assert which thread holds a read lock." It lacks the negative though (unlocked): "If WITNESS is not included in the kernel, then it is impossible to assert which thread does or does not hold a read lock." Maybe this is clear enough, but the addition of "held anonymously" makes the following text clearer for me. "As read locks are held anonymously by threads, if WITNESS is not included in the kernel, then it is only possible to assert that a read lock is or is not held by any thread." "As read locks are held anonymously by threads, if WITNESS is not included in the kernel, then it is impossible to assert which thread does or does not hold a read lock." "As read locks are held anonymously by threads, if WITNESS is not included in the kernel, then it is impossible to assert whether the current thread does or does not own a read lock." (note 'own') I'm liking the last the most here as it is closer to what was already there with some clarity on the anonymity of the lock along with "own" which seems to be the missing clarity. The "by threads" part may not be needed too. Here's my final suggestion: "As read locks are held anonymously, if WITNESS is not included in the kernel, then it is impossible to assert whether the current thread does or does not own a read lock." |
What happens in that case is that everything works. This entire section is really about the non-WITNESS case.
When
.Dv WITNESS
has not been included in the kernel, it is impossible to assert which thread holds
a shared lock, and the
.Dv SA_LOCKED
and
.Dv SA_SLOCKED
assertions merely check whether any thread holds a shared lock.
| share/man/man9/sx.9 | ||
|---|---|---|
| 327 ↗ | (On Diff #14426) | The double-clause thing on your last suggestion makes my head hurt a bit. :-P Hmm. If we make it clear this entire paragraph is about the !WITNESS case and that these are all tied together, perhaps we can avoid explicitly repeating that fact. I honestly prefer the current version of the first sentence in this diff though. I think it's clear that the assertion is in relation to the current thread's view of the world (which all of the locking assertions are relevant towards) and that we just don't know if the current thread has a shared lock or not. However, I do think we can tighten the text a bit further which might get towards Warren's point (I think). "If WITNESS is not included in the kernel, it is impossible to assert whether the current thread does or does not hold a shared lock. As a result, the SA_LOCKED and SA_UNLOCKED assertions merely check whether any thread holds a shared lock. (The above also puts back the "They do not ensure..." sentence which I think is helpful in terms of clarity.) |
| share/man/man9/sx.9 | ||
|---|---|---|
| 327 ↗ | (On Diff #14426) | Right. If the condition has not changed, it does not need to be restated. In other words, factoring out a common expression. It is possible to invert some of the logic to reduce the "if not"s. "A kernel without WITNESS cannot assert whether the current thread holds a shared lock. SA_LOCKED and SA_UNLOCKED can only assert that *any* thread holds a shared lock. They cannot ensure that the current thread holds a shared lock. Further, SA_UNLOCK can only assert that the current thread does not hold an exclusive lock." Maybe a little clearer, maybe not. Maybe not accurate any more (it's been a long day). The mdoc for the emphasized "any" is .Em any |
| share/man/man9/sx.9 | ||
|---|---|---|
| 327 ↗ | (On Diff #14426) | I do think your version is clearer. The only tweak I would make is to bring back the "does or does not" in the first sentence. "current thread does or does not hold a shared lock." Otherwise I think one could read it as only applying to the positive assertion "does have a lock" but not to the negative assertion "does not have a lock". |
| share/man/man9/sx.9 | ||
|---|---|---|
| 327 ↗ | (On Diff #14426) | Added! Here is a full dress rehearsal with markup, clowns, circus animals, Shriners driving giant motorized computer mice, etc.: A kernel without .Dv WITNESS cannot assert whether the current thread does or does not hold a shared lock. .Dv SA_LOCKED and .Dv SA_UNLOCKED can only assert that .Em any thread holds a shared lock. They cannot ensure that the current thread holds a shared lock. Further, .Dv SA_UNLOCK can only assert that the current thread does not hold an exclusive lock. |
| share/man/man9/sx.9 | ||
|---|---|---|
| 327 ↗ | (On Diff #14426) | There's a few errors in here. I am updating the revision with the full fixed text. |