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

[Alejandro Colomar <alx@kernel.org>]

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

Hi Lorenzo,

On Wed, Apr 23, 2025 at 10:37:57AM +0100, Lorenzo Stoakes wrote:
> Hi Alejandro,
> 
> Sorry for being slow on this, LSF and workload has held me up a bit, will apply
> your feedback and send a v2 soon.

No problem.  Take as much time as you need.  :)


Have a lovely day!
Alex

> 
> Thanks, Lorenzo
> 
> On Sun, Mar 23, 2025 at 09:15:36PM +0100, Alejandro Colomar wrote:
> > 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/>

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

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