Re: [PATCH 1/1] man/man2/ioctl_userfaultfd.2, UFFDIO_MOVE.2const: Add UFFDIO_MOVE page

[Suren Baghdasaryan <surenb@google.com>]

On Wed, Apr 23, 2025 at 10:15 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Wed, Apr 23, 2025 at 1:16 AM Alejandro Colomar <alx@kernel.org> wrote:
> >
> > Hi Suren,
> >
> > On Tue, Apr 22, 2025 at 06:12:03PM -0700, Suren Baghdasaryan wrote:
> > > Documentation was extracted from the original patch written by Andrea
> > > Arcangeli and upstreamed in [1]. Minor edits were made to maintain
> > > the same documentation style as other userfaultfd ioctl commands.
> > >
> > > [1] <https://lore.kernel.org/all/20231206103702.3873743-3-surenb@google.com/>
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >
> > It looks good, but there are a few issues commented below.
> > Thanks for the patch!
>
> Thanks for the feedback. Working on addressing your comments and will
> post v2 today.

Answers to your comments are below.
Resulting v2 is posted at
https://lore.kernel.org/all/20250423195309.2841410-1-surenb@google.com/
Thanks!

>
> >
> >
> > Have a lovely day!
> > Alex
> >
> > > ---
> > >  man/man2/ioctl_userfaultfd.2     |   2 +
> > >  man/man2const/UFFDIO_MOVE.2const | 149 +++++++++++++++++++++++++++++++
> > >  2 files changed, 151 insertions(+)
> > >  create mode 100644 man/man2const/UFFDIO_MOVE.2const
> > >
> > > diff --git a/man/man2/ioctl_userfaultfd.2 b/man/man2/ioctl_userfaultfd.2
> > > index 3cb1b8305..5ec08ca55 100644
> > > --- a/man/man2/ioctl_userfaultfd.2
> > > +++ b/man/man2/ioctl_userfaultfd.2
> > > @@ -69,6 +69,8 @@ events.
> > >  .TQ
> > >  .BR UFFDIO_COPY (2const)
> > >  .TQ
> > > +.BR UFFDIO_MOVE (2const)
> > > +.TQ
> > >  .BR UFFDIO_ZEROPAGE (2const)
> > >  .TQ
> > >  .BR UFFDIO_WAKE (2const)
> > > diff --git a/man/man2const/UFFDIO_MOVE.2const b/man/man2const/UFFDIO_MOVE.2const
> > > new file mode 100644
> > > index 000000000..ebeefde22
> > > --- /dev/null
> > > +++ b/man/man2const/UFFDIO_MOVE.2const
> > > @@ -0,0 +1,149 @@
> > > +'\" t
> >
> > This shouldn't be there.  There should be a diagnostic from the build
> > system about it being spurious: that line is there only in pages that
> > use tables (.TS/.TE), to let man(1) know that it should invoke tbl(1).

Ack.

> >
> > > +.\" Written by Andrea Arcangeli <aarcange@redhat.com>
> > > +.\"
> > > +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> > > +.\"
> > > +.TH UFFDIO_MOVE 2const (date) "Linux man-pages (unreleased)"
> > > +.SH NAME
> > > +UFFDIO_MOVE
> > > +\-
> > > +atomically move a continuous memory chunk into the userfault registered range
> > > +.SH LIBRARY
> > > +Standard C library
> > > +.RI ( libc ,\~ \-lc )
> > > +.SH SYNOPSIS
> > > +.nf
> > > +.BR "#include <linux/userfaultfd.h>" "  /* Definition of " UFFD* " constants */"
> > > +.B #include <sys/ioctl.h>
> > > +.P
> > > +.BI "int ioctl(int " fd ", UFFDIO_MOVE, struct uffdio_move *" argp );
> > > +.P
> > > +.B #include <linux/userfaultfd.h>
> > > +.P
> > > +.fi
> > > +.EX
> > > +.B struct uffdio_move {
> > > +.BR "    __u64  dst;" "   /* Destination of move */"
> > > +.BR "    __u64  src;" "   /* Source of move */"
> > > +.BR "    __u64  len;" "   /* Number of bytes to move */"
> >
> > Are we in time to name this size instead of len?  Length usually refers
> > to the number of non-zero characters in a string, while size refers to
> > number of bytes in some object, which is more appropriate in these
> > cases.
> >
> > If this has already been released in the kernel, don't worry about it,
> > but if it hasn't, maybe we should call it size?

Sorry, it was released back in 6.8.


> >
> > > +.BR "    __u64  mode;" "  /* Flags controlling behavior of move */"
> > > +.BR "    __s64  move;" "  /* Number of bytes moved, or negated error */"
> > > +.B };
> > > +.EE
> > > +.SH DESCRIPTION
> > > +Atomically move a continuous memory chunk into the userfault registered
> > > +range and optionally wake up the blocked thread.
> >
> > Please use semantic newlines.  In this case, I'd break the line before
> > 'into', and before 'and'.

Ack.


> >
> > $ 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 indi‐
> >        vidual sentences, clauses, or phrases.
> >
> > > +.P
> > > +The following value may be bitwise ORed in
> > > +.I mode
> >
> > Please use .mode instead of mode.  That makes it more obvious that we're
> > talking about a struct member.  I know most pages don't do this, but I'm
> > planning a global change for consistency soon; since this page is new,
> > we can start clean and do it as
> >
> >         .I .mode
> >
> > This is done in a few cases already in fanotify(7), for example.

Ack. I assume that should be done everywhere else.

> >
> > > +to change the behavior of the
> > > +.B UFFDIO_MOVE
> > > +operation:
> > > +.TP
> > > +.B UFFDIO_MOVE_MODE_DONTWAKE
> > > +Do not wake up the thread that waits for page-fault resolution
> > > +.TP
> > > +.B UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES
> > > +Allow holes in the source virtual range that is being moved.
> > > +When not specified, the holes will result in
> > > +.B ENOENT
> > > +error.
> > > +When specified, the holes will be accounted as successfully
> > > +moved memory. This is mostly useful to move hugepage aligned
> > > +virtual regions without knowing if there are transparent
> > > +hugepages in the regions or not, but preventing the risk of
> > > +having to split the hugepage during the operation.
> >
> > Please use semantic newlines.  In this case, I'd break:
> >
> > -  after ','
> > -  after '.'
> > -  after 'useful'
> > -  before 'without'
> > -  after ','
> > -  after 'of'

Ack.

> >
> > > +.P
> > > +The
> > > +.I move
> > > +field is used by the kernel to return the number of bytes
> > > +that was actually moved, or an error (a negated
> >
> > I'd break:
> >
> > -  after 'kernel'
> > -  after ','
> > -  before '('

Ack.

> >
> > > +.IR errno -style
> > > +value). The
> >
> > Always break after '.'

Ack.

> >
> > See also: <https://web.archive.org/web/20171217060354/http://www.heracliteanriver.com/?p=324>
> >
> > > +.I move
> > > +field is output-only;
> > > +it is not read by the
> > > +.B UFFDIO_MOVE
> > > +operation.
> > > +.P
> > > +The operation may fail for various reasons. Usually, remapping of
> > > +pages that are not exclusive to the given process fail; once KSM
> > > +might deduplicate pages or fork() COW-shares pages during fork()
> > > +with child processes, they are no longer exclusive. Further, the
> > > +kernel might only perform lightweight checks for detecting whether
> > > +the pages are exclusive, and return -EBUSY in case that check fails.
> > > +To make the operation more likely to succeed, KSM should be
> > > +disabled, fork() should be avoided or MADV_DONTFORK should be
> > > +configured for the source VMA before fork().
> >
> > Please use semantic newlines.
> >
> > Also, a few things like EBUSY and MADV_DONTFORK should be marked up.

Ack.

> >
> > > +.SH RETURN VALUE
> > > +On success,
> > > +0 is returned.
> > > +In this case, the entire area was moved.
> > > +.P
> > > +On error, \-1 is returned and
> > > +.I errno
> > > +is set to indicate the error.
> > > +.SH ERRORS
> > > +.TP
> > > +.B EAGAIN
> > > +The number of bytes moved (i.e., the value returned in the
> > > +.I move
> > > +field)
> > > +does not equal the value that was specified in the
> > > +.I len
> > > +field.
> > > +.TP
> > > +.B EINVAL
> > > +Either
> > > +.I dst
> > > +or
> > > +.I len
> > > +was not a multiple of the system page size, or the range specified by
> > > +.I src
> > > +and
> > > +.I len
> > > +or
> > > +.I dst
> > > +and
> > > +.I len
> > > +was invalid.
> > > +.TP
> > > +.B EINVAL
> > > +An invalid bit was specified in the
> > > +.I mode
> > > +field.
> > > +.TP
> > > +.BR ENOENT
> > > +The source virtual memory range has unmapped holes and
> > > +.B UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES
> > > +is not set.
> > > +.TP
> > > +.BR EEXIST
> > > +The destination virtual memory range is fully or partially
> > > +mapped.
> > > +.TP
> > > +.BR EBUSY
> > > +The pages in the source virtual memory range are either
> > > +pinned or not exclusive to the process. The kernel might
> > > +only perform lightweight checks for detecting whether the
> > > +pages are exclusive. To make the operation more likely to
> > > +succeed, KSM should be disabled, fork() should be avoided
> > > +or MADV_DONTFORK should be configured for the source virtual
> > > +memory area before fork().
> >
> > Semantic newlines.
> >
> > This paragraph is repeating part of the information from the last
> > paragraph in the DESCRIPTION.  Do we want to de-duplicate somehow?

Yes, I folded this into the appropriate error code description.

> >
> > > +.TP
> > > +.BR ENOMEM
> > > +Allocating memory needed for the operation failed.
> > > +.TP
> > > +.BR ESRCH
> > > +The target process has exited at the time of a UFFDIO_MOVE
> >
> > UFFDIO_MOVE should be bold.

Ack.

> >
> > > +operation.
> > > +.SH STANDARDS
> > > +Linux.
> > > +.SH HISTORY
> > > +Linux 6.8.
> > > +.SH SEE ALSO
> > > +.BR ioctl (2),
> > > +.BR ioctl_userfaultfd (2),
> > > +.BR userfaultfd (2)
> > > +.P
> > > +.I linux.git/\:Documentation/\:admin\-guide/\:mm/\:userfaultfd.rst
> > >
> > > base-commit: 80e2715270fc05d5627c26f88e4c1ba8b093f510
> > > --
> > > 2.49.0.805.g082f7c87e0-goog
> > >
> >
> > --
> > <https://www.alejandro-colomar.es/>