Re: [PATCH] madvise.2: update MADV_GUARD_INSTALL, MADV_GUARD_REMOVE description

[Alejandro Colomar <alx@kernel.org>]

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

Hi Lorenzo,

On Mon, Mar 17, 2025 at 09:06:53PM +0000, Lorenzo Stoakes wrote:
> Lightweight guard region support has been extended in Linux 6.15,
> permitting the use of these features for file-backed and read-only
> mappings.
> 
> Update the description for these operations in the madvise manpage to
> describe the changed behaviour.
> ---
>  man/man2/madvise.2 | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/man/man2/madvise.2 b/man/man2/madvise.2
> index bd2b90b7a..37492c8cf 100644
> --- a/man/man2/madvise.2
> +++ b/man/man2/madvise.2
> @@ -697,9 +697,22 @@ is applied to regions
>  containing pre-existing lightweight guard regions,
>  they are left in place.
>  .IP
> -This operation is supported
> -only for writable anonymous private mappings
> -which have not been mlock'd.
> +Prior to Linux v6.15 This operation was supported

We don't use 'v' for version numbers.

	alx@devuan:~/src/linux/man-pages/man-pages/contrib$ grep -rho 'Linux [1-6][^ ]*' | wc -l
	7679
	alx@devuan:~/src/linux/man-pages/man-pages/contrib$ grep -rho 'Linux v[1-6][^ ]*' | wc -l
	13

(Oh, well, I need to fix those 13 places.)

Also, there should be a comma (and lowercase):

	Prior to Linux 6.15,
	this operation ...

> +only for writable anonymous private mappings.
> +Since Linux v6.15 both anonymous and file-backed

A comma here too.

> +mappings are supported, including read-only mappings.

I'd break the line after the comma (and put 'mappings ...,' in the
previous one).

> +.IP
> +The mapping must not be mlock'd,
> +nor can they map hugetlb ranges

mapping is singular, but they is plural.  Did I misunderstand, or is it
a typo?

> +or special mappings

I think there should be some punctuation here, but let's revisit after
the rest of the paragraph is revised.

> +for example,
> +mappings marked with kernel-internal flags such as
> +.B VM_PFNMAP
> +or
> +.BR VM_IO ,
> +or secret memory regions created using
> +.BR memfd_secret(2) .

The space should go before (2), not after.

> +.IP
>  An
>  .B EINVAL
>  error is returned if it is attempted on any other kind of mapping.
> @@ -756,19 +769,23 @@ and
>  .IP
>  All mappings in the range
>  other than lightweight guard regions
> -are left in place
> -(including mlock'd mappings).
> -The operation is,
> -however,
> -valid only for writable anonymous private mappings,
> +are left in place.
> +The operation is supported on those mappings
> +permitted by
> +.B MADV_GUARD_INSTALL
> +in addition to mlock()'d mappings,
>  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.
> +Anonymous private mappings therefore become

I'd put 'therefore' as the first word in the sentence, followed by a
comma.

> +zero-fill-on-demand pages,
> +and file-backed mappings are repopulating with the

repopulat{ing => ed}?

> +memory contents from the up-to-date contents of the
> +underlying mapped file.
> +

We can't have blank source lines.  That results in a diagnostic.

>  .IP
>  If any transparent huge pages are encountered in the operation,
>  they are left in place.
> -- 
> 2.48.1
> 

Have a lovely night!
Alex

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

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