Details
- Reviewers
bjk emaste jilles - Group Reviewers
manpages - Commits
- rS306334: Document thr_suspend(2) and thr_wake(2).
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 53 ↗ | (On Diff #20646) | s/in suspended/in a suspended/ |
| 57 ↗ | (On Diff #20646) | "elapsion" is not a word, although it seems reasonable enough to make me wonder why. by elapsing of the timeout specified in the |
| 72 ↗ | (On Diff #20646) | Break the sentence here, also add articles: call, the thread is not put into a suspended state. Instead, the call |
| 78 ↗ | (On Diff #20646) | s/with/with an/ |
| 83 ↗ | (On Diff #20646) | s/Same as/As/ |
| 85 ↗ | (On Diff #20646) | called from another thread, the next |
| 92 ↗ | (On Diff #20646) | Passive -> active: (Backslash is not needed before dash.) |
| lib/libc/sys/thr_wake.2 | ||
| 55 ↗ | (On Diff #20646) | s/threads/thread's/ This is another very long and complex sentence which is hard to understand. Maybe: Passing the thread identifier of the executing thread (see .Xr thr_self 2 ) to .Fn thr_wake sets a thread's flag to cause the next signal-interruptible sleep of that thread in the kernel to fail immediately with the .Er EINTR error. |
| 62 ↗ | (On Diff #20646) | Just "If". |
| 68 ↗ | (On Diff #20646) | s/of/of the/ |
| 78 ↗ | (On Diff #20646) | No backslash needed before dash. |
| 84 ↗ | (On Diff #20646) | s/the following/these/ |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 55 ↗ | (On Diff #20646) | I would probably say "terminated by another thread calling", although terminated has the wrong connotation imho. "The state is exited by another thread calling" maybe |
| 57 ↗ | (On Diff #20646) | "elapsing of the timeout" sounds awkward to me though |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 57 ↗ | (On Diff #20646) | cem suggested "by expiration of the timeout" |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 48 ↗ | (On Diff #20671) | .Xr pthread_cond_timedwait 3 seems a closer match. |
| 106 ↗ | (On Diff #20671) | The specified timeout elapsed. (or: expired) |
| 110 ↗ | (On Diff #20671) | Hmm, isn't this a simple special case of the above [ETIMEDOUT] error? |
| 127 ↗ | (On Diff #20671) | The current libthr does not use this function. |
| lib/libc/sys/thr_wake.2 | ||
| 47 ↗ | (On Diff #20671) | .Xr pthread_cond_broadcast 3 seems a closer match. |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 48 ↗ | (On Diff #20671) | Well, condition variable is both more cumbersome to use (needs mutex) and requires more mutual involvement of the suspender and victim. Sometimes it is good, when you want a consistent state at the safe suspension point. Sometimes it contradicts the intent of using the suspension, e.g. when you just want to look at a stuff without other threads intervening. I added references to cond_wait and broadcast, but did not removed suspend_np and resume_np. |
| 110 ↗ | (On Diff #20671) | Yes, but I think that some explicitness there is useful, same as e.g. select(2) man notes that poll is performed with zero timeval. |
| 127 ↗ | (On Diff #20671) | Yes, I know, and thr_wake is used, but not for resume. Intent of the note is to inform that the interface is mostly the internal interface between kernel and some threading library, not inform about exact implementation details. Do you still request to remove the second half of the sentence ? |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 57 ↗ | (On Diff #20676) | "This state", probably. |
| 62 ↗ | (On Diff #20676) | It is the act of delivery, not the signal itself, that effects the wakeup, as I understand it. So, "or by the delivery of a signal to the suspended thread"? |
| 68 ↗ | (On Diff #20676) | Maybe "explicit thr_wake"? |
| 72 ↗ | (On Diff #20676) | Maybe "If a wake from thr_wake was delivered before [...]" |
| 78 ↗ | (On Diff #20676) | "If a thread previously called thr_wake with its own thread identifier [...]" |
| 93 ↗ | (On Diff #20676) | This looks like ".Rv -std thr_wait"? |
| 112 ↗ | (On Diff #20676) | "a zero time interval" |
| 124 ↗ | (On Diff #20676) | s/the$// |
| lib/libc/sys/thr_wake.2 | ||
| 54 ↗ | (On Diff #20676) | (There are separate .Po and .Pc macros, though I don't always recommend their use.) |
| 61 ↗ | (On Diff #20676) | s/the/an/ |
| 79 ↗ | (On Diff #20676) | .Rv -std again, I think. |
| 103 ↗ | (On Diff #20676) | s/the// |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 48 ↗ | (On Diff #20671) | Hmm, I don't think pthread_suspend_np should be recommended like this because the risk of lost wakeups and inconsistent state is high, and it should not be the first option considered. A condition variable (or a semaphore) needs a bit more setup but is easier to get right. pthread_suspend_np does belong in the SEE ALSO section because of the similarity in name. |
| 110 ↗ | (On Diff #20671) | OK. |
| 127 ↗ | (On Diff #20671) | I suppose it can be replaced with something that is not factually incorrect, such as The |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 48 ↗ | (On Diff #20671) | IMO the reciprocal requirements of the semaphore or condvar is what makes crucial difference with pthread_suspend_np(3). And I think that there are legitimate cases where preemptive suspension is wanted and is the only robust approach. It is more related to debugging/introspective code, and cannot substitute normal synchonization. I reworded the sentence to give the advise you want to give, but still mention pthread_suspend_np (which I want). |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 48 ↗ | (On Diff #20671) | I like this idea but I don't like the word "voluntaristic". Perhaps s/for typical safe but voluntaristic suspension/for typical safe suspension with cooperation of the thread being suspended/. |
| lib/libc/sys/thr_wake.2 | ||
| 50 ↗ | (On Diff #20694) | Please use similar wording here as in the corresponding part of thr_suspend.2. |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 57 ↗ | (On Diff #20646) | Maybe when the time specified by .Fa timeout has elapsed, |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 57 ↗ | (On Diff #20646) | Do you mean the following (diff is against the committed version): diff --git a/lib/libc/sys/thr_suspend.2 b/lib/libc/sys/thr_suspend.2 index 61320ea..b470d18 100644 --- a/lib/libc/sys/thr_suspend.2 +++ b/lib/libc/sys/thr_suspend.2 @@ -61,9 +61,9 @@ system call puts the calling thread in a suspended state, where it is not eligible for CPU time. This state is exited by another thread calling .Xr thr_wake 2 , -by expiration of the timeout specified in the +when the time interval specified by .Fa timeout -argument, +has elapsed, or by the delivery of a signal to the suspended thread. .Pp If the |
| lib/libc/sys/thr_suspend.2 | ||
|---|---|---|
| 57 ↗ | (On Diff #20646) | I think that works, yes. "time interval" can be changed to just "interval", but either way. Thanks! |