On 2023-01-06 12:50, Alejandro Colomar wrote:
Hi Mathieu,
See some comments below.
Cheers,
Alex
[...]
We now use SPDX-License-Identifier.
OK
.\"
.TH RSEQ 2 2020-06-05 "Linux" "Linux Programmer's Manual"
We use lowercase for the function name (or to be more precise, the same
case that the function name uses.
The date is specified with a placeholder that is replaced at the time of
creation of the tarball.
The 5th argument is unspecified.
The 4th argument is now the project name and a placeholder for the version.
See an example:
$ cat man2/membarrier.2 | grep '^.TH'
.TH membarrier 2 (date) "Linux man-pages (unreleased)"
OK
.SH NAME
rseq \- Restartable sequences system call
We use lowercase here, so s/Restartable/restartable/
OK
.SH SYNOPSIS
.nf
.B #include <linux/rseq.h>
Is there a glibc wrapper for this syscall? Do you expect that it will
be added relatively soon? Or is it expected that this syscall will be
called through syscall(2) for many years?
The system call is only for registering/unregistering the per-thread
area, so I expect it will only be used through syscall(2), and typically
only from libc.
User applications/libraries are expected to use the per-thread area to
communicate with the kernel, but that does not require use of rseq(2).
If so, it may be better to document it directly as such, like for
example membarrier:
SYNOPSIS
#include <linux/membarrier.h> /* Definition of MEMBARRIER_*
constants */
#include <sys/syscall.h> /* Definition of SYS_* constants */
#include <unistd.h>
int syscall(SYS_membarrier, int cmd, unsigned int flags, int
cpu_id);
Note: glibc provides no wrapper for membarrier(), necessitating
the use
of syscall(2).
OK
.sp
s/sp/PP/
OK
.BI "int rseq(struct rseq * " rseq ", uint32_t " rseq_len ", int "
flags ",
Is it valid to pass NULL in 'rseq'? If it is, we now document that
using _Nullable. See for example recv(2):
SYNOPSIS
#include <sys/socket.h>
ssize_t recv(int sockfd, void buf[.len], size_t len,
int flags);
ssize_t recvfrom(int sockfd, void buf[restrict .len], size_t len,
int flags,
struct sockaddr *_Nullable restrict src_addr,
socklen_t *_Nullable restrict addrlen);
ssize_t recvmsg(int sockfd, struct msghdr *msg, int flags);
OK
uint32_t " sig ");
.sp
.sp is unnecessary here.
OK
.fi is missing (it's the "closing" pair to .nf).
OK
.SH DESCRIPTION
Use .PP instead of blank lines.
OK
The
.BR rseq ()
ABI accelerates specific user-space operations by registering a
per-thread data structure shared between kernel and user-space. This
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.
OK, done across the entire the manual page source.
[...]
.PP
Some examples of operations that can be accelerated or improved
by this ABI:
.IP \[bu] 2
Use 3 instead of 2. See man-pages:
Lists
There are different kinds of lists:
[...]
Bullet lists
Elements are preceded by bullet symbols (\(bu).
Anything that
doesn’t fit elsewhere is usually covered by this type of
list.
[...]
There should always be exactly 2 spaces between the list symbol
and the
elements. This doesn’t apply to "tagged paragraphs", which use
the de‐
fault indentation rules.
The rationale for that was that if you use 1 space, then the list
introducer can be confused with the list contents. Two spaces makes the
difference more clear.
OK
Also, we use \(bu instead of \[bu]. I'm not particularly worried by
using it, but I prefer being consistent at which one we use.
OK
[...]
.TP
.in +4n
I guess you're looking for .RS/.RE. It would wrap all the indented
stuff, replacing .in.
OK.
.I cpu_id_start
Always-updated value of the CPU number on which the registered thread is
running. Its value is guaranteed to always be a possible CPU number,
even when rseq is not registered. Its value should always be confirmed by
rseq (and maybe other cases around too) should be formatted in italics,
since it's a variable name (.I).
"rseq" here does not refer to a variable name, but rather to the system
call. Should it be formatted in italics ? I format it as ".BR" elsewhere
in the man page.
I'll push a first update without the italics changes, please let me know
if there are still issues left.
[...]
.in +4n
.I flags
Flags indicating the restart behavior of this structure. Can be a
combination
of:
.IP \[bu]
This list should be a tagged paragraph instead. See man-pages(7):
Lists
There are different kinds of lists:
Tagged paragraphs
These are used for a list of tags and their
descriptions. When
the tags are constants (either macros or numbers) they
are in
bold. Use the .TP macro.
An example is this "Tagged paragraphs" subsection is itself.
[...]
Bullet lists
Elements are preceded by bullet symbols (\(bu).
Anything that
doesn’t fit elsewhere is usually covered by this type of
list.
[...]
OK
[...]
.SH RETURN VALUE
A return value of 0 indicates success. On error, \-1 is returned, and
.I errno
is set appropriately.
.SH ERRORS
.TP
.B EINVAL
Either
.I flags
contains an invalid value, or
.I rseq
contains an address which is not appropriately aligned, or
.I rseq_len
contains an incorrect size.
.TP
.B ENOSYS
The
.BR rseq ()
system call is not implemented by this kernel.
.TP
.B EFAULT
.I rseq
is an invalid address.
Doesn't this result in a SEGV? It's trying to access invalid memory.
We had some discussion about this in other syscalls, and concluded that
that's undefined behavior, and a crash is valid behavior (and probably a
good thing to do), right? I'm just curious about the view from the
kernel point of view.
If the registered rseq pointer / size points to invalid memory on rseq
registration, the rseq registration system call will return -1,
errno=EFAULT. If at some point _after_ registration the mapping becomes
invalid (e.g. unmapped without prior unregistration), then a SIGSEGV can
be triggered.
I was not aware of this discussion a regarding returning EFAULT errno vs
SIGSEGV. If this is becoming a consensus across system calls to segfault
rather than return EFAULT errno, I'm open to improve sys_rseq accordingly.
.TP
.B EBUSY
Restartable sequence is already registered for this thread.
.TP
.B EPERM
The
.I sig
argument on unregistration does not match the signature received
on registration.
.SH VERSIONS
The
.BR rseq ()
system call was added in Linux 4.18.
.SH CONFORMING TO
We call that section STANDARDS now.
OK
I've done a first pass applying all the relevant changes. I've pushed
the changes into the librseq master branch. I may have omitted
bold/italic for some identifiers. If it's the case, please let me know.
Thanks!
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
