| Message ID | 20241205104125.67518-1-lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [man-pages,v4] madvise.2: add MADV_GUARD_INSTALL, MADV_GUARD_REMOVE description | expand |
Hi Lorenzo, Jann, > Subject: Re: [PATCH man-pages v4] madvise.2: add MADV_GUARD_INSTALL, MADV_GUARD_REMOVE description We use uppercase after the prefix, so s/add/Add/. On Thu, Dec 05, 2024 at 10:41:25AM +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 The right amount of inter-sentence space is two. See this: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/CONTRIBUTING.d/patches/description?id=bcf7d00fa4c7ce270f07d6e347c01b1f1e37580f> <https://web.archive.org/web/20171217060354/http://www.heracliteanriver.com/?p=324> > operations. > > Reviewed-by: Jann Horn <jannh@google.com> Thanks for the review! > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Thanks for the patch! I've applied it, with some minor tweaks. See comments below. <https://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git/commit/?h=contrib&id=bb405ee3f6039226267fb1c6d2cb1fbb18d835bf> Here's the diff that I applied when amending your patch: diff --git i/man/man2/madvise.2 w/man/man2/madvise.2 index adb372424..fa24f6bf6 100644 --- i/man/man2/madvise.2 +++ w/man/man2/madvise.2 @@ -678,7 +678,8 @@ .SS Linux-specific advice values 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 +Install a lightweight guard region +into the range specified by .I addr and .IR size , @@ -686,22 +687,27 @@ .SS Linux-specific advice values .B SIGSEGV signal being raised. .IP -If the region maps memory pages they will be cleared as part of the operation, +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, +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. +This operation is supported +only 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 +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 @@ -740,12 +746,15 @@ .SS Linux-specific advice values 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 +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 +All mappings in the range +other than lightweight guard regions +are left in place (including mlock'd mappings). The operation is, however, > --- > 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. I've broken lines a little bit more, even though they were correct, just for having shorter lines. > * 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 > +.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 I missed this before. It was the same misplacement of only. :) Have a lovely day! Alex
On Thu, Dec 05, 2024 at 01:20:37PM +0100, Alejandro Colomar wrote: > Hi Lorenzo, Jann, > > > Subject: Re: [PATCH man-pages v4] madvise.2: add MADV_GUARD_INSTALL, MADV_GUARD_REMOVE description > > We use uppercase after the prefix, so s/add/Add/. > > On Thu, Dec 05, 2024 at 10:41:25AM +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 > > The right amount of inter-sentence space is two. See this: > <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/CONTRIBUTING.d/patches/description?id=bcf7d00fa4c7ce270f07d6e347c01b1f1e37580f> > <https://web.archive.org/web/20171217060354/http://www.heracliteanriver.com/?p=324> > > > operations. > > > > Reviewed-by: Jann Horn <jannh@google.com> > > Thanks for the review! > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > Thanks for the patch! I've applied it, with some minor tweaks. See > comments below. > <https://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git/commit/?h=contrib&id=bb405ee3f6039226267fb1c6d2cb1fbb18d835bf> Thanks all seems reasonable. Just a quick question for future changes - I see you reference git://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git - but I've been working against git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git - is the latter occasionally synced from the former? Or should I be working against your personal repo for future changes? Thanks, Lorenzo > > Here's the diff that I applied when amending your patch: > > diff --git i/man/man2/madvise.2 w/man/man2/madvise.2 > index adb372424..fa24f6bf6 100644 > --- i/man/man2/madvise.2 > +++ w/man/man2/madvise.2 > @@ -678,7 +678,8 @@ .SS Linux-specific advice values > 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 > +Install a lightweight guard region > +into the range specified by > .I addr > and > .IR size , > @@ -686,22 +687,27 @@ .SS Linux-specific advice values > .B SIGSEGV > signal being raised. > .IP > -If the region maps memory pages they will be cleared as part of the operation, > +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, > +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. > +This operation is supported > +only 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 > +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 > @@ -740,12 +746,15 @@ .SS Linux-specific advice values > 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 > +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 > +All mappings in the range > +other than lightweight guard regions > +are left in place > (including mlock'd mappings). > The operation is, > however, > > > --- > > 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. > > I've broken lines a little bit more, even though they were correct, just > for having shorter lines. > > > * 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 > > +.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 > > I missed this before. It was the same misplacement of only. :) > > Have a lovely day! > Alex > > > -- > <https://www.alejandro-colomar.es/>
Hi Lorenzo, On Thu, Dec 05, 2024 at 12:26:56PM +0000, Lorenzo Stoakes wrote: > On Thu, Dec 05, 2024 at 01:20:37PM +0100, Alejandro Colomar wrote: > > Thanks for the patch! I've applied it, with some minor tweaks. See > > comments below. > > <https://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git/commit/?h=contrib&id=bb405ee3f6039226267fb1c6d2cb1fbb18d835bf> > > Thanks all seems reasonable. > > Just a quick question for future changes - I see you reference > git://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git - but > I've been working against > git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git - is the latter > occasionally synced from the former? The 'main' branch in that server is usually at the same point that 'master' at <kernel.org>. They're the same thing, just on different servers. Then, there's a 'contrib' branch in my server, which I use to buffer patches I apply from others. That allows me to amend typos and other mistakes (or even drop patches) before pushing to master. Here's my workflow: 1) I push always first to 'contrib', which triggers CI on my server, and lets me know if all's good (it runs many linters set up in the build system). 2) Then I let know the contributor I've pushed there. Then I leave it there for a day or so. 3) If I don't find anything wrong in a day or so, I push to 'main' in my server, which regenerates the PDF book in my website: <https://www.alejandro-colomar.es/share/dist/man-pages/git/HEAD/man-pages-HEAD.pdf>. 4) After the PDF is generated correctly, I push to <kernel.org>'s 'master'. You can think of this contrib branch as a 'next' from the kernel, but I pull from it much more often. > Or should I be working against your > personal repo for future changes? Most of the time, it's enough to use the 'master' branch from <kernel.org>. In some cases, for example if you have several patches for a single page, if I have applied some of them, and you need to rebase, it would make sense to base on that 'contrib' branch. Or if I have applied some changes recently to it and might conflict with yours, I'll ask you to rebase. Cheers, Alex > > Thanks, Lorenzo
On Thu, Dec 05, 2024 at 01:43:58PM +0100, Alejandro Colomar wrote: > Hi Lorenzo, > > On Thu, Dec 05, 2024 at 12:26:56PM +0000, Lorenzo Stoakes wrote: > > On Thu, Dec 05, 2024 at 01:20:37PM +0100, Alejandro Colomar wrote: > > > Thanks for the patch! I've applied it, with some minor tweaks. See > > > comments below. > > > <https://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git/commit/?h=contrib&id=bb405ee3f6039226267fb1c6d2cb1fbb18d835bf> > > > > Thanks all seems reasonable. > > > > Just a quick question for future changes - I see you reference > > git://www.alejandro-colomar.es/src/alx/linux/man-pages/man-pages.git - but > > I've been working against > > git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git - is the latter > > occasionally synced from the former? > > The 'main' branch in that server is usually at the same point that > 'master' at <kernel.org>. They're the same thing, just on different > servers. > > Then, there's a 'contrib' branch in my server, which I use to buffer > patches I apply from others. That allows me to amend typos and other > mistakes (or even drop patches) before pushing to master. > > Here's my workflow: > > 1) I push always first to 'contrib', which triggers CI on my server, > and lets me know if all's good (it runs many linters set up in the > build system). > > 2) Then I let know the contributor I've pushed there. Then I leave it > there for a day or so. > > 3) If I don't find anything wrong in a day or so, I push to 'main' in > my server, which regenerates the PDF book in my website: > <https://www.alejandro-colomar.es/share/dist/man-pages/git/HEAD/man-pages-HEAD.pdf>. > > 4) After the PDF is generated correctly, I push to <kernel.org>'s > 'master'. > > You can think of this contrib branch as a 'next' from the kernel, but I > pull from it much more often. > > > Or should I be working against your > > personal repo for future changes? > > Most of the time, it's enough to use the 'master' branch from > <kernel.org>. > > In some cases, for example if you have several patches for a single > page, if I have applied some of them, and you need to rebase, it would > make sense to base on that 'contrib' branch. Or if I have applied some > changes recently to it and might conflict with yours, I'll ask you to > rebase. > > Cheers, > Alex Perfect, appreciate the explanation! Thanks :) > > > > > Thanks, Lorenzo > > -- > <https://www.alejandro-colomar.es/>
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. > +.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. Not sure if "mlock'd" is the canonical term, I think I've seen "locked" used before, which I don't think it's great. Maybe Alejandro knows better. (there's also another "mlock'd" later in the patch)
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. 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. > > > +.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. > > Not sure if "mlock'd" is the canonical term, I think I've seen "locked" used > before, which I don't think it's great. Maybe Alejandro knows better. > > (there's also another "mlock'd" later in the patch) > There's many different ways of saying this it's not really hugely consistent. I've seen mlock'ed mlock'd mlock()ed etc. etc. So there is no 'canonical' term, and I think it's pretty clear what is meant here. There's also a reference to "mlock'd" earlier in the file (though in a comment...)
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. >
Hi Vlastimil, Lorenzo, On Thu, Dec 05, 2024 at 09:43:09PM +0100, Vlastimil Babka wrote: > >> > +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: I would actually like you to bikeshed. :-) These wording issues tend to be important, and finding the right wording is worth the bikeshedding. On the other hand, sometimes we just don't have ideas, and merging and later fixing is also okay. > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Thanks! Have a lovely night! Alex > > > 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. > > >
On Thu, Dec 05, 2024 at 09:43:09PM +0100, Vlastimil Babka wrote: > 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? I was about to put more philosophical points here then realised we already have a better solution... we can say 'the regions will become zero-fill-on-demand'. Let me respin a v5 with this corrected. We will need to revisit once we support file-backed pages. > > Anyway, I don't want to cause bikeshedding, so in any case: No it's appreciated! Review should be critical so we can make this as good as we can make it. > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> Thanks! > > > 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. > >
On Fri, Dec 06, 2024 at 11:03:56AM +0000, Lorenzo Stoakes wrote: > On Thu, Dec 05, 2024 at 09:43:09PM +0100, Vlastimil Babka wrote: > > 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? > > I was about to put more philosophical points here then realised we already have > a better solution... we can say 'the regions will become zero-fill-on-demand'. > > Let me respin a v5 with this corrected. We will need to revisit once we > support file-backed pages. Actually, as discussed on IRC, I think your suggested 'replaced' is better, so: "If the region maps memory pages, those mappings will be replaced". Will address in v5. > > > > > Anyway, I don't want to cause bikeshedding, so in any case: > > No it's appreciated! Review should be critical so we can make this as good > as we can make it. > > > > > Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > > Thanks! > > > > > > 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. > > >
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 +.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 (2) +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 (2), +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, +valid only for writable anonymous private 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. +.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 )