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

[Alejandro Colomar <alx@kernel.org>]

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

Hi Muhammad,

[CC += Branden]

On Thu, Oct 19, 2023 at 06:12:45PM +0500, Muhammad Usama Anjum wrote:
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
> The feature has been added to mm-stable:
> https://lore.kernel.org/all/20231018213453.BF1ACC43395@smtp.kernel.org
> 
> Changes since v1:
> - Several formatting updates
> - Added some additional sentences

Wow, the formatting is very well done.  Great job!  Patch applied.
See a few small comments below.

> ---
>  man2/ioctl_pagemap_scan.2 | 203 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 203 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..c257072d7
> --- /dev/null
> +++ b/man2/ioctl_pagemap_scan.2
> @@ -0,0 +1,203 @@
> +.\" This manpage is Copyright (C) 2023 Collabora;
> +.\" Written by Muhammad Usama Anjum <usama.anjum@collabora.com>
> +.\"
> +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> +.\"
> +.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 ", "
> +.BR "                          struct page_region "and " PAGE_IS_* "constants " */"

That space is not necessary after a closing '"' is something I don't
know why exists.  I changed that slightly.

Also, we use the Oxford comma (a comma right before 'and' and 'or'.

-.BR "#include <linux/fs.h>" "  /* Definition of " struct " " pm_scan_arg ", "
-.BR "                          struct page_region "and " PAGE_IS_* "constants " */"
+.BR "#include <linux/fs.h>" "  /* Definition of " "struct pm_scan_arg" ,
+.BR "                          struct page_region" ", and " PAGE_IS_* " constants */"

> +.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.
> +The information is returned with
> +.B PAGE_SIZE
> +granularity.
> +.PP
> +To start tracking the written state (flag) of a page or range of memory,
> +the
> +.B UFFD_FEATURE_WP_ASYNC
> +must be enabled by
> +.B UFFDIO_API
> +.BR ioctl (2)
> +on
> +.B userfaultfd
> +and memory range must be registered with
> +.B UFFDIO_REGISTER
> +.BR ioctl (2)
> +in
> +.B UFFDIO_REGISTER_MODE_WP
> +mode.
> +.SS Supported page flags
> +The following page table entry flags are supported:
> +.TP
> +.B PAGE_IS_WPALLOWED
> +The page has asynchronous write-protection enabled.
> +.TP
> +.B PAGE_IS_WRITTEN
> +The page has been written to from the time it was write protected.
> +.TP
> +.B PAGE_IS_FILE
> +The page is file backed.
> +.TP
> +.B PAGE_IS_PRESENT
> +The page is present in the memory.
> +.TP
> +.B PAGE_IS_SWAPPED
> +The page is swapped.
> +.TP
> +.B PAGE_IS_PFNZERO
> +The page has zero PFN.
> +.TP
> +.B PAGE_IS_HUGE
> +The page is THP or Hugetlb backed.
> +.SS Supported operations
> +The get operation is always performed
> +if the output buffer is specified.
> +The other operations are as following:
> +.TP
> +.B PM_SCAN_WP_MATCHING
> +Write protect the matched pages.
> +.TP
> +.B PM_SCAN_CHECK_WPASYNC
> +Abort the scan
> +when a page is found
> +which doesn't have the Userfaultfd Asynchronous Write protection enabled.
> +.SS The \f[I]struct pm_scan_arg\f[] argument
> +.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;

I prefer two spaces between the type and the name.  I got that habit
from nginx.
<https://nginx.org/en/docs/dev/development_guide.html#code_style>

 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;
+    __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
> +.TP
> +.B size
> +This field should be set to the size of the structure in bytes,
> +as in
> +.IR "sizeof(struct pm_scan_arg)" .

We try to use \~ for a fillable space; it has the nice effect of
removing the quotes.

-.IR "sizeof(struct pm_scan_arg)" .
+.IR sizeof(struct\~pm_scan_arg) .

> +.TP
> +.B flags
> +The operations to be performed are specified in it.
> +.TP
> +.B start
> +The starting address of the scan is specified in it.
> +.TP
> +.B end
> +The ending address of the scan is specified in it.
> +.TP
> +.B walk_end
> +The kernel returns the scan's ending address in it.
> +The
> +.I walk_end
> +equal to
> +.I end
> +means that scan has completed on the entire range.
> +.TP
> +.B vec
> +The address of
> +.I page_region
> +array for output.
> +.PP
> +.in +8n

Ahh, this is great, because I needed to explain to groff(1) maintainers
what is the problem with TP that I was complaining about in another
thread.

Branden, here's what I mean.  If you're new to man(7), it is rather
unintuitive what to do here.

Muhammad, in this project, we usually use IP to continuate a TP.  PP
would break the indentation back to before the TP, which is why you
needed so much in 'in'.

Another solution, which we're discussing is wrapping everything is RS/RE.

I applied this:

-.PP
-.in +8n
+.IP
+.in +4n


> +.EX
> +struct page_region {
> +    __u64 start;
> +    __u64 end;
> +    __u64 categories;
> +};
> +.EE
> +.in
> +.TP
> +.B vec_len
> +The length of the
> +.I page_region
> +struct array.
> +.TP
> +.B max_pages
> +It is the optional limit for the number of output pages required.
> +.TP
> +.B category_inverted
> +.BI PAGE_IS_ *
> +categories which values match if 0 instead of 1.
> +.TP
> +.B category_mask
> +Skip pages for which any
> +.BI PAGE_IS_ *
> +category doesn't match.
> +.TP
> +.B category_anyof_mask
> +Skip pages for which no
> +.BI PAGE_IS_ *
> +category matches.
> +.TP
> +.B return_mask
> +.BI PAGE_IS_ *
> +categories that are to be reported in
> +.IR page_region .
> +.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
> +.IR flags ,
> +invalid
> +.IR categories ,
> +the
> +.I start
> +address isn't aligned with
> +.B 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
> +.IR end .
> +.TP
> +.B ENOMEM
> +No memory is available.
> +.TP
> +.B EINTR
> +Fetal signal is pending.

And a bit more of semantic newlines:

@@ -163,29 +163,32 @@ .SH ERRORS
 Error codes can be one of, but are not limited to, the following:
 .TP
 .B EINVAL
-Invalid arguments i.e., invalid
+Invalid arguments i.e.,
+invalid
 .I size
-of the argument, invalid
+of the argument,
+invalid
 .IR flags ,
 invalid
 .IR categories ,
 the
 .I start
 address isn't aligned with
-.B PAGE_SIZE
+.BR PAGE_SIZE ,
 or
 .I vec_len
 is specified when
 .I vec
-is
-.BR NULL .
+is NULL.
 .TP
 .B EFAULT
 Invalid
 .I arg
-pointer, invalid
+pointer,
+invalid
 .I vec
-pointer or invalid address range specified by
+pointer,
+or invalid address range specified by
 .I start
 and
 .IR end .


Cheers,
Alex

> +.SH STANDARDS
> +Linux.
> +.SH HISTORY
> +Linux 6.7.
> +.SH SEE ALSO
> +.BR ioctl (2)
> -- 
> 2.42.0
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]