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

[Vlastimil Babka <vbabka@suse.cz>]

On 12/5/24 19:09, Lorenzo Stoakes wrote:
> On Thu, Dec 05, 2024 at 06:50:09PM +0100, Vlastimil Babka wrote:
>> On 12/5/24 11:41, 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.
>> >
>> > Reviewed-by: Jann Horn <jannh@google.com>
>> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> > ---
>> > v4:
>> > * Reference function chapters as per Alejandro.
>> > * Minor rewording as per Alejandro.
>> >
>> > v3:
>> > * Don't describe SIGSEGV as a fatal signal as per Jann.
>> > https://lore.kernel.org/all/20241202165829.72121-1-lorenzo.stoakes@oracle.com
>> >
>> > 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..7d682fa40 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
>>
>> Hm this reads a bit ambiguous. One could read it as the memory pages are
>> being cleared, but it's the page tables.
> 
> This was really hard to word, because you don't want to say unmapped, and saying
> 'clearing page tables' or 'zapping' is clear to us but not necessarily to a
> reader. 'Clearing mapping' makes it ambiguous vs. munmap(), etc. etc.

Maybe saying "removed" instead of "cleared" would be better?

Anyway, I don't want to cause bikeshedding, so in any case:

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

> But you want to make it clear (no pun intended) that anon pages, if there's any
> data, it's probably lost. So I think this is a distinction that doesn't matter.
> 
> Will revisit once we support file-backed mappings.
>