Hi Mathieu, On 2/15/23 18:09, Mathieu Desnoyers wrote: > On 2023-02-14 20:20, G. Branden Robinson wrote: >> [CC list violently trimmed; for those who remain, this is mostly man >> page style issues] > > [ Gently added linux-man to CC list. ;-) ] :-) > >> >> At 2023-02-14T23:29:37+0100, Alejandro Colomar wrote: >>> On 2/14/23 20:54, Mathieu Desnoyers wrote: >>>> +per-thread data structure shared between kernel and user-space. >>> >>> This last 'user-space' is not adjectivated, so it should go without >>> a hyphen, according to common English rules. >> >> +1 > > done > > >> >> Also I like your coinage. "Adjectivated yeast" is reflexive and >> tautological! >> >>>> +.RB ( "struct rseq" ) >>> >>> We format types in italics, so this should be '.RI'. >> >> +1 > > OK, so it's italics for both types and arguments. > > I will replace all the bold markers for "struct rseq" and "struct > rseq_cs" to italic in the description (but not in the synopsis section > and not in the code snippets). > >> >>>> +Only one >>>> +.BR rseq () >>>> +ABI can be registered per thread, so user-space libraries and >>>> +applications must follow a user-space ABI defining how to share this >>>> +resource. >>> >>> Please use semantic newlines. See man-pages(7): >>> >>> Use semantic newlines >>> In the source of a manual page, new sentences should be started on new >>> lines, long sentences should be split into lines at clause breaks (com‐ >>> mas, semicolons, colons, and so on), and long clauses should be split >>> at phrase boundaries. This convention, sometimes known as "semantic >>> newlines", makes it easier to see the effect of patches, which often >>> operate at the level of individual sentences, clauses, or phrases. >> >> I think I've said this before, but, strictly, commas in particular can >> separate things that are not clauses. Clauses have subjects and >> predicates. >> >> Might it be better to say simply: >> >> Start each sentence on a new line. Split long sentences where >> punctuated by commas, semicolons, and colons. >> >> With this there is not even any need to discuss "phrase boundaries". >> > > I've modified to: > > Only one > .BR rseq () > ABI can be registered per thread, > so user-space libraries and applications must follow a user-space ABI > defining how to share this resource. > > Hopefully that's correct. Yes, that's good. > > >>> In the above lines, that would mean breaking after the comma, >>> and not leaving resource in a line of its own. >> >> The latter is inevitably going to happen from time to time simply due to >> sentence length and structure and the line length used by one's text >> editor. I don't think an "orphan word" (what typographers call this) is >> symptomatic of anything in *roff source when filling is enabled. >> >>>> +The ABI defining how to share this resource between applications and >>>> +libraries is defined by the C library. >>>> +Allocation of the per-thread >>>> +.BR rseq () >>>> +ABI and its registration to the kernel is handled by glibc since version >>>> +2.35. >>>> +.PP >>>> +The >>>> +.BR rseq () >>>> +ABI per-thread data structure contains a >>>> +.I rseq_cs >>>> +field which points to the currently executing critical section. >>> >>> currently-executing should probably use a hyphen >>> (if I understood the line correctly). >> >> This is not the case, according to some style authorities. Dave Kemper >> convinced me of this on the groff list. >> >> Here is one resource. >> >> https://www.editorgroup.com/blog/to-hyphenate-or-not-to-hyphenate/ >> >>> See an interesting discussion in the groff@ mailing list: >>> <https://lists.gnu.org/archive/html/groff/2022-10/msg00015.html> >> >> That's not _squarely_ on point, as none of "block", "device", or "based" >> is an adverb. "Currently" is. > > Leaving unchanged based on this discussion. > >> >>>> +For each thread, a single rseq critical section can run at any given >>>> +point. >>>> +Each critical section need to be implemented in assembly. >>> >>> needs? >> >> +1 > > done. > >> >>>> +.TP >>>> +.B Structure alignment >>> >>> Let's remove the bold here. It's not necessary for marking a constant >>> or something that needs bold. And the indentation is already making >>> it stand out, so bold is a bit too much aggressive to the reader. >> >> I agree; if it wouldn't be styled in running text, it doesn't need >> styling as a paragraph tag; it already stands out by dint of its >> placement as a tag. >> >>>> +Its value should always be confirmed by reading the cpu_id field before >>> >>> cpu_id should be formatted (.I). >> >> +1 > > done > >> >>>> +user-space performs any side-effect >>>> +(e.g. storing to memory). >>>> +.IP >>>> +This field is always guaranteed to hold a valid CPU number in the range >>>> +[ 0 .. nr_possible_cpus - 1 ]. >>> >>> Please use interval notation: >>> [0, nr_possible_cpus) >>> or >>> [0, nr_possible_cpus - 1] >>> whichever looks better to you. >>> >>> We did some consistency fix recently: >>> <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=147a60d792a5db8f3cb93ea16eefb73e16c1fb91> >>> >>> Also, do we have a more standard way of saying nr_possible_cpus? >>> Should we say nproc? > > nproc(1) means: > > Print the number of processing units available to the current > process, which may be less than the number of online processors > > Which is the number of cpus currently available (AFAIU the result of the > cpuset and sched affinity). > > What I really mean here is the maximum value for possible cpus which can > be hotplugged into the system. So it's not the maximum number of > possible CPUs per se, but rather the maximum enabled bit in the possible > CPUs mask. > > Note that we could express this differently as well: rather than saying > that it guarantees a value in the range [0, nr_possible_cpus - 1], we > could say that the values are guaranteed to be part of the possible cpus > mask, which would actually more accurate in case the possible cpus mask > has a hole (it tends to happen with things like lxc containers nowadays). > > Do you agree that we should favor expressing this in terms of belonging > to the possible cpumask set rather than a range starting from 0 ? On 2/15/23 18:12, Mathieu Desnoyers wrote: > Actually, the field may contain the value 0 even if 0 is not part of the > possible cpumask. So forget what I just said about being guaranteed to > be part of the possible cpus mask. > > Thoughts ? > > Thanks, > > Mathieu I don't have a full understanding, so I will trust you for deciding what is best. I'll try to understand it, but my kernel knowledge is rather limited :) I suggest writing a detailed description, instead of (or complementary to it) just using a range, since readers might wonder as I did, what nr_possible_cpus is (it's not really described anywhere so far). With a worded description, we can later improve it if we find it not clear enough, but should be enough for an initial page. Thanks! Alex > > Thanks, > > Mathieu > -- <http://www.alejandro-colomar.es/> GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
