From: Ingo Schwarze Subject: Re: pthread man.3 inode conservation To: Ted Unangst Cc: tech@openbsd.org Date: Sat, 21 Jun 2025 16:55:49 +0200 Hello Ted, Ted Unangst wrote on Sat, Jun 21, 2025 at 12:26:05AM -0400: > How about this? There's a lot of one sentence man pages in libpthread, > which I think are probably not very useful. It's a lackluster experience > having to read four or five different man pages just to use a condition > variable. Why isn't this all in one place? I agree with the general direction. There are no hard rules, it is always a matter of judgement, but the general idea is that functions should be documented together when they are so closely related or so similar that keeping them together saves text and/or makes the text easier to follow, but pages should be split when they become large - sometimes, a dozen functions already warrants a split, sometimes (especially for ill-designed interfaces) two or even three dozen functions in a single page can still be OK. Avoiding unnecessary churn is also a consideration, but the case of excessive splitting you are showing here looks bad enough that a bit of churn feels tolerable. > This diff more or less mechanically combines the pages. I think a > bit more narrative structure can be added, but at least you can get > some idea of what these functions are for and how they work together > in a single page now. You have to be more careful with Copyright and license headers. The page pthread_cond_init.3 is under a three-clause BSD license with JOHN BIRRELL and THE REGENTS in the warranty disclaimer. The page pthread_condattr_init.3, however, is under a two-clause BSD license with THE COPYRIGHT HOLDER(S) in the warranty disclaimer. It would arguably be legal to combine both files if you put something like the following on top: Parts of this file are distributed under this Copyright and license: ... Other parts of this file are distributed under this Copyright and license: ... However, i advise against that. If files are under different licenses, even if the licences are compatible and allow combining, it helps clarity to not combine them unless there are very strong reasons, and i do not consider combining manual pages that *can* be kept separate a strong enough reason to risk even minor confusion about Copyright and licensing. Here is a list of the pages you are touching: page name clauses warranty discl Copyright holder pthread_cond_init 3 BIRRELL+REGENTS Brian Cully 1997 pthread_condattr_init 2 COPYRIGHT HOLDER(S) Jason Evans 2000 pthread_cond_broadcast 3 BIRRELL+REGENTS Brian Cully 1997 pthread_cond_destroy 3 BIRRELL+REGENTS Brian Cully 1997 pthread_cond_signal 3 BIRRELL+REGENTS Brian Cully 1997 pthread_cond_timedwait 3 BIRRELL+REGENTS Brian Cully 1997 pthread_cond_wait 3 BIRRELL+REGENTS Brian Cully 1997 So fixing this seems as easy as taking pthread_condattr_init.3 out of the merge. But even then, you still have to check (carefully, with diff(1)) that the Copyright, license, and warranty disclaimer headers are word-for-word identical, and then merge Copyright holder and Copyright year lists (in this case, they are all just Brian Cully and 1997, so nothing to merge). Finally, i'd suggest copying all the $FreeBSD$ CVS ID lines into the combined file, ideally in the same order that text is moved in. Doing that helps future maintainers in case anybody ever wants to merge anything back or forth among operating systems, or in case doubts about the provenance of the text should ever arise. As it stands, your diff is NOT OK because it is mandatory to have all functions documented in the SYNOPSIS listed in the NAME section as well, usually in the same order. It is also NOT OK because your order in the DESCRIPTION differs from your order in the SYNOPSIS. In very exceptional cases, that may perhaps be acceptable, but i see no reason to make an exception here. Finally, your diff is NOT OK because it fails to touch the .Nd one-line description. With you diff, .Nd create a condition variable remains but becomes badly misleading. The .Nd line should ideally be a concise summary of everything described in the manual page and should also clearly distinguish the scope of the manual page from the scopes of related manual pages, if that is possible without turning wordy. Maybe something like this could work: .Nd conditionally block and unblock threads But maybe you have a better understanding of the purpose of this stuff than i do and can come up with a summary that is even more to the point. > Starting with condition variables. Please be careful to not touch any pages having David Leonard (d@) listed as an author right now; i'm currently juggling large diffs for those and would hate to experience conflicts. My in-line remarks are suggestions, not conditions for going ahead. > Index: Makefile.inc > =================================================================== > RCS file: /home/cvs/src/lib/libpthread/man/Makefile.inc,v > diff -u -p -r1.37 Makefile.inc > --- Makefile.inc 12 Jan 2019 00:16:03 -0000 1.37 > +++ Makefile.inc 21 Jun 2025 04:19:48 -0000 > @@ -19,13 +19,7 @@ MAN+= \ > pthread_barrierattr_getpshared.3 \ > pthread_cleanup_pop.3 \ > pthread_cleanup_push.3 \ > - pthread_condattr_init.3 \ As explained, i'd prefer to keep the above separate. > - pthread_cond_broadcast.3 \ > - pthread_cond_destroy.3 \ > pthread_cond_init.3 \ > - pthread_cond_signal.3 \ > - pthread_cond_timedwait.3 \ > - pthread_cond_wait.3 \ > pthread_cancel.3 \ > pthread_create.3 \ > pthread_detach.3 \ > Index: pthread_cond_init.3 > =================================================================== > RCS file: /home/cvs/src/lib/libpthread/man/pthread_cond_init.3,v > diff -u -p -r1.13 pthread_cond_init.3 > --- pthread_cond_init.3 7 Jun 2025 00:16:52 -0000 1.13 > +++ pthread_cond_init.3 21 Jun 2025 04:17:28 -0000 > @@ -40,6 +40,24 @@ > .In pthread.h > .Ft int > .Fn pthread_cond_init "pthread_cond_t *cond" "const pthread_condattr_t *attr" > +.Ft int > +.Fn pthread_cond_destroy "pthread_cond_t *cond" > +.Ft int > +.Fn pthread_cond_broadcast "pthread_cond_t *cond" > +.Ft int > +.Fn pthread_cond_signal "pthread_cond_t *cond" > +.Ft int > +.Fn pthread_cond_wait "pthread_cond_t *cond" "pthread_mutex_t *mutex" > +.Ft int > +.Fn pthread_cond_timedwait "pthread_cond_t *cond" "pthread_mutex_t *mutex" "const struct timespec *abstime" Lines of 80 characters or more are very ugly, and there is no need for them here. My preferred solution would be to switch all the .Fn to .Fo in this SYNOPSIS. A close second would be to only switch those functions from .Fn to .Fo that have more than one argument. Consider the recommendations in https://mandoc.bsd.lv/mdoc/style/synopsis_func.html : To show the prototype of a function, using Fo usually results in more readable source code than using Fn, in particular for functions having more than one argument. Avoid input lines of 80 characters and more, and avoid input line continuation with a backslash character at the end of an input line. For functions having only one single argument, Fn is usually acceptable if the whole macro fits on one input line, but there is nothing wrong with using Fo for consistency with other, more complicated functions on the same page. If the type and name are both very short, using Fn with two arguments may result in source code that is easier to read. (Yes, i need to update the recommendation regarding .Lb at the top of that page.) > +.Ft int > +.Fn pthread_condattr_init "pthread_condattr_t *attr" > +.Ft int > +.Fn pthread_condattr_destroy "pthread_condattr_t *attr" > +.Ft int > +.Fn pthread_condattr_setclock "pthread_condattr_t *attr" "clockid_t clock_id" > +.Ft int > +.Fn pthread_condattr_getclock "pthread_condattr_t *attr" "clockid_t *clock_id" Again, i'd prefer to not move those in here. > .Sh DESCRIPTION > The > .Fn pthread_cond_init > @@ -50,12 +68,84 @@ If > is > .Dv NULL , > the default attributes are used. > +.Pp > +The > +.Fn pthread_cond_broadcast > +function unblocks all threads waiting for the condition variable > +.Fa cond . > +The > +.Fn pthread_cond_signal > +function unblocks at least one thread waiting for the condition variable > +.Fa cond . > +.Pp > +The > +.Fn pthread_cond_wait > +function atomically blocks the current thread waiting on the condition > +variable specified by > +.Fa cond , > +and unblocks the mutex specified by > +.Fa mutex . > +The waiting thread unblocks only after another thread calls > +.Fn pthread_cond_signal > +or > +.Fn pthread_cond_broadcast > +with the same condition variable. > +The > +.Fn pthread_cond_timedwait > +function does the same, but returns after the system time reaches > +.Fa abstime . > +The current thread then reacquires the lock on > +.Fa mutex . Here, i suggest saying For both functions, the current thread then reacquires the lock on .Fa mutex . because with your current text, readers might be mislead to think that the final sentence only applies to pthread_cond_timedwait() and not to pthread_cond_wait(). > +.Pp > +The > +.Fn pthread_cond_destroy > +function frees the resources allocated by the condition variable > +.Fa cond . This needs to be moved up. Alternatively, it could be moved down in the SYNOPSIS. I think i slightly prefer documenting init() and destroy() together at the top over constructing a bracket in the order "init, use, destroy". > +.Pp > +Condition variable attributes are used to specify parameters to > +.Fn pthread_cond_init . > +One attribute object can be used in multiple calls to > +.Fn pthread_cond_init , > +with or without modifications between calls. > +.Pp > +The > +.Fn pthread_condattr_init > +function initializes > +.Fa attr > +with all the default condition variable attributes. > +.Pp > +The > +.Fn pthread_condattr_destroy > +function destroys > +.Fa attr . > +.Pp > +The > +.Fn pthread_condattr_setclock > +function sets the clock attribute of > +.Fa attr > +to the value of the > +.Fa clock_id > +parameter. > +The > +.Fn pthread_condattr_getclock > +function copies the value of the clock attribute from > +.Fa attr > +to the location pointed to by the > +.Fa clock_id > +parameter. > +The clock attribute is the ID of the clock against which the timeout of > +.Fn pthread_cond_timedwait > +is compared; > +the default value of the clock attribute is > +.Dv CLOCK_REALTIME . Again, i'd prefer to not move those here. > .Sh RETURN VALUES > If successful, the > .Fn pthread_cond_init > function will return zero and put the new condition variable ID into > .Fa cond , > otherwise an error number will be returned to indicate the error. > +Other functions similarly return zero for success and error numbers > +for failure. I suggest shortening this to: These functions return 0 if successful or a positive error number to indicate failure. What is put into output parameters does not belong into the RETURN VALUES section but into the DESCRIPTION, like so: The .Fn pthread_cond_init function creates a new condition variable in .Pf * Fa cond , with attributes ... > .Sh ERRORS > .Fn pthread_cond_init > will fail if: > @@ -71,13 +161,23 @@ variable. > The system temporarily lacks the resources to create another condition > variable. > .El > +.Pp > +Other functions may fail if: I'd prefer "The other functions fail if:". Without the definite article, it is unclear whether this is maybe intended to mean "some other", and if so, which. Besides, i don't think this needs "will" or "may". I know very large numbers of our pages say "will fail", but i see no good reason for that. When touching something anyway, it would seem nice to me to switch to "FOO fails if". I dislike "may" even more than "will" in this context. The term "may" is standardese for "implementations are free to either report this as an error, or to ignore the situation and succeed anyway". We want to document what our implementation actually does, not what it would be allowed to do instead, right? > +.Bl -tag -width Er > +.It Bq Er EINVAL > +The value specified by > +.Fa cond > +or another argument is invalid. I'd prefer being specific: The value specified by .Fa cond , .Fa mutex , or .Fa abstime is invalid. > +.It Bq Er EBUSY > +The variable > +.Fa cond > +is locked by another thread. Can you make it clear that this only applies to pthread_cond_destroy()? For example by prefixing it with: pthread_cond_destroy() also fails if: > +.It Bq Er ETIMEDOUT > +The system time has reached or exceeded the time specified in > +.Fa abstime . > +.El Can you make it clear that this only applies to pthread_cond_timedwait()? For example by prefixing it with: pthread_cond_timedwait() also fails if: > .Sh SEE ALSO > -.Xr pthread_cond_broadcast 3 , > -.Xr pthread_cond_destroy 3 , > -.Xr pthread_cond_signal 3 , > -.Xr pthread_cond_timedwait 3 , > -.Xr pthread_cond_wait 3 > +.Xr pthread_mutex_init 3 Probably .Xr pthread_condattr_init 3 should be added here, right? > .Sh STANDARDS > -.Fn pthread_cond_init > -conforms to > +These functions conform to > .St -p1003.1-96 . While here, would you mind also checking whether this can be updated to -p1003.1-2024 ? Thanks for doing this work, Ingo