Re: [PATCH man-pages v3] madvise.2: add MADV_GUARD_INSTALL, MADV_GUARD_REMOVE description

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Dec 04, 2024 at 09:43:25PM +0100, Alejandro Colomar wrote:
> Hi Lorenzo,
>
> On Mon, Dec 02, 2024 at 04:58:29PM +0000, Lorenzo Stoakes wrote:
> > Lightweight guard region support has been added to Linux 6.13, which adds
> > MADV_GUARD_INSTALL and MADV_GUARD_REMOVE flags to the madvise() system
> > call. Therefore, update the manpage for madvise() and describe these
> > operations.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Thanks for the patch!  A few minor comments below.
>
> > ---
> > v3:
> > * Don't describe SIGSEGV as a fatal signal as per Jann.
> >
> > v2:
> > * Updated to use semantic newlines as suggested by Alejandro.
> > * Avoided emboldening parens as suggested by Alejandro.
> > * One very minor grammatical fix.
> > https://lore.kernel.org/all/20241129155943.85215-1-lorenzo.stoakes@oracle.com
> >
> > v1:
> > https://lore.kernel.org/all/20241129093205.8664-1-lorenzo.stoakes@oracle.com
> >
> >  man/man2/madvise.2 | 93 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> >
> > diff --git a/man/man2/madvise.2 b/man/man2/madvise.2
> > index 4f2210ee2..4cb5e7302 100644
> > --- a/man/man2/madvise.2
> > +++ b/man/man2/madvise.2
> > @@ -676,6 +676,91 @@ or secret memory regions created using
> >  Note that with
> >  .BR MADV_POPULATE_WRITE ,
> >  the process can be killed at any moment when the system runs out of memory.
> > +.TP
> > +.BR MADV_GUARD_INSTALL " (since Linux 6.13)"
> > +Install a lightweight guard region into the range specified by
> > +.I addr
> > +and
> > +.IR size ,
> > +causing any read or write in the range to result in a
> > +.B SIGSEGV
> > +signal being raised.
> > +.IP
> > +If the region maps memory pages they will be cleared as part of the operation,
> > +though if
> > +.B MADV_GUARD_INSTALL
> > +is applied to regions containing pre-existing lightweight guard regions,
> > +they are left in place.
> > +.IP
> > +This operation is only supported for writable anonymous private mappings which
> > +have not been mlock'd.
> > +An
> > +.B EINVAL
> > +error is returned if it is attempted on any other kind of mapping.
> > +.IP
> > +This operation is more efficient than mapping a new region of memory
> > +.BR PROT_NONE ,
> > +as it does not require the establishment of new mappings,
> > +instead regions of an existing mapping simply have their page tables
> > +manipulated to establish the desired behavior.
> > +No additional memory is used.
> > +.IP
> > +Lightweight guard regions remain on fork
> > +(except for any parts which have had
> > +.B MADV_WIPEONFORK
> > +applied to them),
> > +and are not removed by
> > +.BR MADV_DONTNEED ,
> > +.BR MADV_FREE ,
> > +.BR MADV_PAGEOUT ,
> > +or
> > +.BR MADV_COLD .
> > +.IP
> > +Attempting to
> > +.BR mlock ()
>
> We should specify the chapter in the manual: (2)

Ack will fix.

>
> > +lightweight guard regions will fail,
> > +as will
> > +.B MADV_POPULATE_READ
> > +or
> > +.BR MADV_POPULATE_WRITE .
> > +.IP
> > +If the mapping has its attributes changed,
> > +or is split or partially unmapped,
> > +any existing guard regions remain in place
> > +(except if they are unmapped).
> > +.IP
> > +If a mapping is moved using
> > +.BR mremap (),
>
> Same here.

Will also fix :)

>
> > +lightweight guard regions are moved with it.
> > +.IP
> > +Lightweight guard regions are removed when unmapped,
> > +on process teardown,
> > +or when the
> > +.B MADV_GUARD_REMOVE
> > +operation is applied to them.
> > +.TP
> > +.BR MADV_GUARD_REMOVE " (since Linux 6.13)"
> > +Remove any lightweight guard regions which exist in the range specified by
> > +.I addr
> > +and
> > +.IR size .
> > +.IP
> > +All mappings in the range other than lightweight guard regions are left in place
> > +(including mlock'd mappings).
> > +The operation is,
> > +however,
> > +only valid for writable anonymous private mappings,
>
> Wouldn't it be better to say "valid only" instead of "only valid"?
>
> (Non-native speaker here, though.)

Non-native speakers often pick up mistakes native speakers make because
we're so used to the language we can decode 'clunky' sounding things and
mistakenly write poorly as a result.

You're correct, your version is almost certainly grammatically correct but
far more importantly reads far more nicely!

Will fix.

>
> Have a lovely night!
> Alex

Thanks!

>
> > +returning an
> > +.B EINVAL
> > +error otherwise.
> > +.IP
> > +When lightweight guard regions are removed,
> > +they act as empty regions of the containing mapping.
> > +Since only writable anonymous private mappings are supported,
> > +they therefore become zero-fill-on-demand pages.
> > +.IP
> > +If any transparent huge pages are encountered in the operation,
> > +they are left in place.
> >  .SH RETURN VALUE
> >  On success,
> >  .BR madvise ()
> > @@ -787,6 +872,14 @@ or
> >  or secret memory regions created using
> >  .BR memfd_secret(2) .
> >  .TP
> > +.B EINVAL
> > +.I advice
> > +is
> > +.B MADV_GUARD_INSTALL
> > +or
> > +.BR MADV_GUARD_REMOVE ,
> > +but the specified address range contains an unsupported mapping.
> > +.TP
> >  .B EIO
> >  (for
> >  .BR MADV_WILLNEED )
> > --
> > 2.47.1
>
> --
> <https://www.alejandro-colomar.es/>