Re: [PATCH 2/2] ioctl_pagemap_scan: add page for pagemap_scan IOCTL

Alejandro Colomar
2023-10-17T17:12:45Z

[-- Attachment #1: Type: text/plain, Size: 12824 bytes --]

Hi Muhammad,

On Tue, Oct 17, 2023 at 08:01:32PM +0500, Muhammad Usama Anjum wrote:
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

Please link to the discussion where this feature is being discussed, in
case someone needs to review it in the future.

> ---
> man2/ioctl_pagemap_scan.2 | 183 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 183 insertions(+)
> create mode 100644 man2/ioctl_pagemap_scan.2
>
> diff --git a/man2/ioctl_pagemap_scan.2 b/man2/ioctl_pagemap_scan.2
> new file mode 100644
> index 000000000..4e096028d
> --- /dev/null
> +++ b/man2/ioctl_pagemap_scan.2
> @@ -0,0 +1,183 @@
> +.\" This manpage is Copyright (C) 2023 Collabora;
> +.\" Written by Muhammad Usama Anjum <usama.anjum@collabora.com>
> +.\"
> +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> +.\"
> +.\" Commit 84ceddb3c2b0c280936f28f450d65f46cb7411c6
> +.\"
> +.TH ioctl_pagemap_scan 2 2023-10-17 "Linux man-pages (unreleased)"
> +.SH NAME
> +ioctl_pagemap_scan \- get and/or clear page flags
> +.SH LIBRARY
> +Standard C library
> +.RI ( libc ", " \-lc )
> +.SH SYNOPSIS
> +.nf
> +.BR "#include <linux/fs.h>" " /* Definition of " struct " " pm_scan_arg ", " struct " " page_region " and " PAGE_IS_* " constants */"

Please split the comment into several lines, so that the manual page can
be read in an 80-column terminal without wrapping lines.

> +.B #include <sys/ioctl.h>
> +.PP
> +.BI "int ioctl(int " pagemap_fd ", PAGEMAP_SCAN, struct pm_scan_arg *" arg );
> +.fi
> +.SH DESCRIPTION
> +This
> +.BR ioctl (2)
> +is used to get and optionally clear some specific flags from page table entries.
> +

Please use an empty roff(7) request instead of a empty input line. That
is, use a line with just a '.', that is, s/^$/./

$ make lint build check --debug=print
TROFF .tmp/man/man2/ioctl_pagemap_scan.2.cat.set
! (troff -wbreak -man -Tutf8 -rLL=78n -rCHECKSTYLE=3 -ww <.tmp/man/man2/ioctl_pagemap_scan.2.cat.troff 2>&1 >.tmp/man/man2/ioctl_pagemap_scan.2.cat.set \
| grep -v -f './share/lint/groff/man.ignore.grep' \
||:; \
) \
| grep ^ >&2
an.tmac:man2/ioctl_pagemap_scan.2:25: style: blank line in input
...

> +.SS Supported page flags
> +The following page table entry flags are support:

s/support/supported/

> +.TP
> +.BR PAGE_IS_WPALLOWED

'.BR' is wrong here (it alternates bold and roman). It should be '.B',
which is just bold. You can check groff_man(7) and groff_man_style(7).
See these warnings:

$ make lint build check --debug=print
TROFF .tmp/man/man2/ioctl_pagemap_scan.2.cat.set
! (troff -wbreak -man -Tutf8 -rLL=78n -rCHECKSTYLE=3 -ww <.tmp/man/man2/ioctl_pagemap_scan.2.cat.troff 2>&1 >.tmp/man/man2/ioctl_pagemap_scan.2.cat.set \
| grep -v -f './share/lint/groff/man.ignore.grep' \
||:; \
) \
| grep ^ >&2
an.tmac:man2/ioctl_pagemap_scan.2:25: style: blank line in input
an.tmac:man2/ioctl_pagemap_scan.2:29: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:32: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:35: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:38: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:41: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:44: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:47: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:49: style: blank line in input
an.tmac:man2/ioctl_pagemap_scan.2:53: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:56: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:58: style: blank line in input
an.tmac:man2/ioctl_pagemap_scan.2:79: style: blank line in input
an.tmac:man2/ioctl_pagemap_scan.2:81: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:88: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:91: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:94: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:97: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:99: style: .IR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:101: style: .IR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:104: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:106: style: .IR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:119: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:121: style: .IR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:124: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:127: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:130: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:133: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:136: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:138: style: .IR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:139: style: blank line in input
an.tmac:man2/ioctl_pagemap_scan.2:157: style: .BR expects at least 2 arguments, got 1
an.tmac:man2/ioctl_pagemap_scan.2:163: style: .BR expects at least 2 arguments, got 1
make: *** [share/mk/build/catman.mk:54: .tmp/man/man2/ioctl_pagemap_scan.2.cat.set] Error 1
make: *** Deleting file '.tmp/man/man2/ioctl_pagemap_scan.2.cat.set'

> +Page has async-write-protection enabled

Here and below:
s/Page/The page/

> +.TP
> +.BR PAGE_IS_WRITTEN
> +Page has been written to from the time it was write protected
> +.TP
> +.BR PAGE_IS_FILE
> +Page is file backed
> +.TP
> +.BR PAGE_IS_PRESENT
> +Page is present in the memory
> +.TP
> +.BR PAGE_IS_SWAPPED
> +Page is in swapped
> +.TP
> +.BR PAGE_IS_PFNZERO
> +Page has zero PFN
> +.TP
> +.BR PAGE_IS_HUGE
> +Page is THP or Hugetlb backed
> +

s/^$/./

> +.SS Supported Operations

I think s/Operations/operations/

> +The get operation is always performed if the output buffer is specified. The other operations are as following:

Please use semantic newlines. Input lines should never go past column
80, except for very good reasons.

$ MANWIDTH=72 man man-pages | sed -n '/Use semantic newlines/,/^$/p'
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 (commas, 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 in‐
dividual sentences, clauses, or phrases.

Also, see this warning:

$ make lint build check --debug=print
LINT (mandoc) .tmp/man/man2/ioctl_pagemap_scan.2.lint-man.mandoc.touch
! (mandoc -man -Tlint man2/ioctl_pagemap_scan.2 2>&1 \
| grep -v -f './share/lint/mandoc/man.ignore.grep' \
||:; \
) \
| grep ^ >&2
mandoc: man2/ioctl_pagemap_scan.2:51:111: STYLE: input text line longer than 80 bytes: The get operation is...
mandoc: man2/ioctl_pagemap_scan.2:57:93: STYLE: input text line longer than 80 bytes: Abort the scan when ...
mandoc: man2/ioctl_pagemap_scan.2:60:2: WARNING: skipping paragraph macro: PP after SS
make: *** [share/mk/lint/man/man.mk:32: .tmp/man/man2/ioctl_pagemap_scan.2.lint-man.mandoc.touch] Error 1

> +.TP
> +.BR PM_SCAN_WP_MATCHING
> +Write protect the matched pages
> +.TP
> +.BR PM_SCAN_CHECK_WPASYNC
> +Abort the scan when a page which isn't registered with Userfaultfd Asynchronous Write protect

Semantic newlines.

> +

s/^$/./

> +.SS The struct pm_scan_arg Argument

Types should go in italics. To do this inside SH or SS, do:

.SS The \f[I]struct pm_scan_arg\f[] Argument.

Also, why Argument and not argument?

> +.PP

This .PP is redundant.

$ make lint build check --debug=print
LINT (mandoc) .tmp/man/man2/ioctl_pagemap_scan.2.lint-man.mandoc.touch
! (mandoc -man -Tlint man2/ioctl_pagemap_scan.2 2>&1 \
| grep -v -f './share/lint/mandoc/man.ignore.grep' \
||:; \
) \
| grep ^ >&2
mandoc: man2/ioctl_pagemap_scan.2:51:111: STYLE: input text line longer than 80 bytes: The get operation is...
mandoc: man2/ioctl_pagemap_scan.2:57:93: STYLE: input text line longer than 80 bytes: Abort the scan when ...
mandoc: man2/ioctl_pagemap_scan.2:60:2: WARNING: skipping paragraph macro: PP after SS
make: *** [share/mk/lint/man/man.mk:32: .tmp/man/man2/ioctl_pagemap_scan.2.lint-man.mandoc.touch] Error 1

> +.in
> +.EX
> +struct pm_scan_arg {
> + __u64 size;
> + __u64 flags;
> + __u64 start;
> + __u64 end;
> + __u64 walk_end;
> + __u64 vec;
> + __u64 vec_len;
> + __u64 max_pages
> + __u64 category_inverted;
> + __u64 category_mask;
> + __u64 category_anyof_mask
> + __u64 return_mask;
> +};
> +.EE
> +.in
> +

This blank line makes no sense.

> +.TP
> +.BR size
> +The size of
> +.I arg
> +is specified in it. It'll help in future if extension is make to

Semantic newlines.

Also, I have a hard time understanding the second sentence.
You may want to check the wording used in other pages for structures
where the size is kind of a version marker.

> +.I struct pm_scan_arg
> +in future.
> +.TP
> +.BR flags
> +The operations to be performed are specified in it.
> +.TP
> +.BR start
> +The starting address of the scan is specified in it.
> +.TP
> +.BR end
> +The ending address of the scan is specified in it.
> +.TP
> +.BR walk_end
> +The kernel returns the scan's ending address in it. The

Semantic newlines.

> +.IR walk_end
> +equal to
> +.IR end
> +means that scan has completed on the entire range.
> +.TP
> +.BR vec
> +The address of
> +.IR page_region
> +array for output
> +.PP
> +.in +8n
> +.EX
> +struct page_region {
> + __u64 start;
> + __u64 end;
> + __u64 categories;
> +};
> +.EE
> +.in
> +.TP
> +.BR vec_len
> +The length of the
> +.IR page_region

Similarly, this should be I, not IR.

> +struct array
> +.TP
> +.BR max_pages
> +Optional limit for number of output pages
> +.TP
> +.BR category_inverted
> +PAGE_IS_* categories which values match if 0 instead of 1

PAGE_IS_* should probably be in mixed bold/italics:

.BI PAGE_IS_ *

See groff_man_stlye(7):

Use bold for ...
..., and for literals
that are major topics of the subject under discussion;
for example, this page uses bold for macro, string, and
register names. In an .EX/.EE example of interactive
I/O (such as a shell session), set only user input in
bold.

Use italics for ...
..., and anywhere a parameter requiring replacement
by the user is encountered. ...

> +.TP
> +.BR category_mask
> +Skip pages for which any category doesn't match
> +.TP
> +.BR category_anyof_mask
> +Skip pages for which no category matches
> +.TP
> +.BR return_mask
> +Page categories that are to be reported in
> +.IR page_region
> +

s/^$/./

> +.SH RETURN VALUE
> +On error, \-1 is returned, and
> +.I errno
> +is set to indicate the error.
> +.SH ERRORS
> +Error codes can be one of, but are not limited to, the following:
> +.TP
> +.B EINVAL
> +Invalid arguments i.e., invalid
> +.I size
> +of the argument, invalid
> +.I flags
> +, invalid
> +.I categories
> +, the
> +.I start
> +address isn't aligned with
> +.BR PAGE_SIZE
> +or
> +.I vec_len
> +is specified when
> +.I vec
> +is
> +.BR NULL.
> +.TP
> +.B EFAULT
> +Invalid
> +.I arg
> +pointer, invalid
> +.I vec
> +pointer or invalid address range specified by
> +.I start
> +and
> +.I end
> +.TP
> +.B ENOMEM
> +No memory is available
> +.TP
> +.B EINTR
> +Fetal signal pending
> +.SH STANDARDS
> +Linux.

Please add a HISTORY section indicating at least the Linux version where
it was (will be) introduced.

Also, it would probably be useful to add an EXAMPLES section with a
small example of use.

Thanks,
Alex

> +.SH SEE ALSO
> +.BR ioctl (2)
> --
> 2.40.1
>

--
<https://www.alejandro-colomar.es/>