[PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Mike Kravetz]

Clarify that madvise only works on full pages, and remove references
to 'bytes'.

Update MADV_DONTNEED and MADV_REMOVE sections to remove notes that
HugeTLB mappings are not supported.  Indicate the releases when they
were first supported as well as alignment restrictions.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
Acked-by: Peter Xu <peterx@redhat.com>
---
v2 -> v3 Rebased on man-pages-5.19-rc1.  Minor change to wording for
	sunsequent access of data after MADV_REMOVE.
v1 -> v2 Added releases when Huge TLB support was added and moved
	alignment requirements to corresponding section.  (Peter)
 man2/madvise.2 | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/man2/madvise.2 b/man2/madvise.2
index 2a8f1cd0a..becddce93 100644
--- a/man2/madvise.2
+++ b/man2/madvise.2
@@ -44,9 +44,13 @@ system call is used to give advice or directions to the kernel
 about the address range beginning at address
 .I addr
 and with size
+.IR length .
+.BR madvise ()
+only operates on whole pages, therefore
+.I addr
+must be page-aligned.  The value of
 .I length
-bytes.
-In most cases,
+is rounded up to a multiple of page size.  In most cases,
 the goal of such advice is to improve system or application performance.
 .PP
 Initially, the system call supported a set of "conventional"
@@ -126,7 +130,7 @@ The resident set size (RSS) of the calling process will be immediately
 reduced however.
 .IP
 .B MADV_DONTNEED
-cannot be applied to locked pages, Huge TLB pages, or
+cannot be applied to locked pages, or
 .B VM_PFNMAP
 pages.
 (Pages marked with the kernel-internal
@@ -136,6 +140,11 @@ flag are special memory areas that are not managed
 by the virtual memory subsystem.
 Such pages are typically created by device drivers that
 map the pages into user space.)
+.IP
+Support for Huge TLB pages was added in Linux v5.18.  Addresses within a
+mapping backed by Huge TLB pages must be aligned to the underlying Huge TLB
+page size, and the range length is rounded up to a multiple of the underlying
+Huge TLB page size.
 .\"
 .\" ======================================================================
 .\"
@@ -153,24 +162,24 @@ Note that some of these operations change the semantics of memory accesses.
 .\" commit f6b3ec238d12c8cc6cc71490c6e3127988460349
 Free up a given range of pages
 and its associated backing store.
-This is equivalent to punching a hole in the corresponding byte
+This is equivalent to punching a hole in the corresponding
 range of the backing store (see
 .BR fallocate (2)).
 Subsequent accesses in the specified address range will see
-bytes containing zero.
+data with a value of zero.
 .\" Databases want to use this feature to drop a section of their
 .\" bufferpool (shared memory segments) - without writing back to
 .\" disk/swap space.  This feature is also useful for supporting
 .\" hot-plug memory on UML.
 .IP
 The specified address range must be mapped shared and writable.
-This flag cannot be applied to locked pages, Huge TLB pages, or
+This flag cannot be applied to locked pages, or
 .B VM_PFNMAP
 pages.
 .IP
 In the initial implementation, only
 .BR tmpfs (5)
-was supported
+supported
 .BR MADV_REMOVE ;
 but since Linux 3.5,
 .\" commit 3f31d07571eeea18a7d34db9af21d2285b807a17
@@ -179,10 +188,12 @@ any filesystem which supports the
 .B FALLOC_FL_PUNCH_HOLE
 mode also supports
 .BR MADV_REMOVE .
-Hugetlbfs fails with the error
-.B EINVAL
-and other filesystems fail with the error
+Filesystems which do not support
+.BR MADV_REMOVE
+fail with the error
 .BR EOPNOTSUPP .
+.IP
+Support for Huge TLB filesystem was added in Linux v4.3.
 .TP
 .BR MADV_DONTFORK " (since Linux 2.6.16)"
 .\" commit f822566165dd46ff5de9bf895cfa6c51f53bb0c4
-- 
2.35.3

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 5220 bytes --]

Hi Mike,

On 6/9/22 01:45, Mike Kravetz wrote:
> Clarify that madvise only works on full pages, and remove references
> to 'bytes'.
> 
> Update MADV_DONTNEED and MADV_REMOVE sections to remove notes that
> HugeTLB mappings are not supported.  Indicate the releases when they
> were first supported as well as alignment restrictions.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> Acked-by: Peter Xu <peterx@redhat.com>

Please check some comments below.

Thanks,

Alex

> ---
> v2 -> v3 Rebased on man-pages-5.19-rc1.  Minor change to wording for
> 	sunsequent access of data after MADV_REMOVE.
> v1 -> v2 Added releases when Huge TLB support was added and moved
> 	alignment requirements to corresponding section.  (Peter)
>   man2/madvise.2 | 31 +++++++++++++++++++++----------
>   1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/man2/madvise.2 b/man2/madvise.2
> index 2a8f1cd0a..becddce93 100644
> --- a/man2/madvise.2
> +++ b/man2/madvise.2
> @@ -44,9 +44,13 @@ system call is used to give advice or directions to the kernel
>   about the address range beginning at address
>   .I addr
>   and with size
> +.IR length .
> +.BR madvise ()
> +only operates on whole pages, therefore
> +.I addr
> +must be page-aligned.  The value of

Please, use semantic newlines (i.e., break after '.').
See man-pages(7):
    Use semantic newlines
        In the source of a manual page, new sentences  should  be
        started on new lines, long sentences should be split into
        lines at clause breaks (commas, semicolons,  colons,  and
        so on), and long clauses should be split at phrase bound-
        aries.  This convention,  sometimes  known  as  "semantic
        newlines",  makes it easier to see the effect of patches,
        which often operate at the level of individual sentences,
        clauses, or phrases.


>   .I length
> -bytes.
> -In most cases,
> +is rounded up to a multiple of page size.  In most cases,
>   the goal of such advice is to improve system or application performance.
>   .PP
>   Initially, the system call supported a set of "conventional"
> @@ -126,7 +130,7 @@ The resident set size (RSS) of the calling process will be immediately
>   reduced however.
>   .IP
>   .B MADV_DONTNEED
> -cannot be applied to locked pages, Huge TLB pages, or
> +cannot be applied to locked pages, or
>   .B VM_PFNMAP
>   pages.
>   (Pages marked with the kernel-internal
> @@ -136,6 +140,11 @@ flag are special memory areas that are not managed
>   by the virtual memory subsystem.
>   Such pages are typically created by device drivers that
>   map the pages into user space.)
> +.IP
> +Support for Huge TLB pages was added in Linux v5.18.  Addresses within a
> +mapping backed by Huge TLB pages must be aligned to the underlying Huge TLB
> +page size, and the range length is rounded up to a multiple of the underlying
> +Huge TLB page size.
>   .\"
>   .\" ======================================================================
>   .\"
> @@ -153,24 +162,24 @@ Note that some of these operations change the semantics of memory accesses.
>   .\" commit f6b3ec238d12c8cc6cc71490c6e3127988460349
>   Free up a given range of pages
>   and its associated backing store.
> -This is equivalent to punching a hole in the corresponding byte
> +This is equivalent to punching a hole in the corresponding
>   range of the backing store (see
>   .BR fallocate (2)).
>   Subsequent accesses in the specified address range will see
> -bytes containing zero.
> +data with a value of zero.
>   .\" Databases want to use this feature to drop a section of their
>   .\" bufferpool (shared memory segments) - without writing back to
>   .\" disk/swap space.  This feature is also useful for supporting
>   .\" hot-plug memory on UML.
>   .IP
>   The specified address range must be mapped shared and writable.
> -This flag cannot be applied to locked pages, Huge TLB pages, or
> +This flag cannot be applied to locked pages, or
>   .B VM_PFNMAP
>   pages.
>   .IP
>   In the initial implementation, only
>   .BR tmpfs (5)
> -was supported
> +supported
>   .BR MADV_REMOVE ;
>   but since Linux 3.5,
>   .\" commit 3f31d07571eeea18a7d34db9af21d2285b807a17
> @@ -179,10 +188,12 @@ any filesystem which supports the
>   .B FALLOC_FL_PUNCH_HOLE
>   mode also supports
>   .BR MADV_REMOVE .
> -Hugetlbfs fails with the error
> -.B EINVAL
> -and other filesystems fail with the error
> +Filesystems which do not support
> +.BR MADV_REMOVE

s/BR/B/

See man(7):
        .B  Bold

        .BI Bold alternating with italics (especially useful  for
            function specifications)

        .BR Bold  alternating  with  Roman (especially useful for
            referring to other manual pages)

> +fail with the error
>   .BR EOPNOTSUPP .
> +.IP
> +Support for Huge TLB filesystem was added in Linux v4.3.
>   .TP
>   .BR MADV_DONTFORK " (since Linux 2.6.16)"
>   .\" commit f822566165dd46ff5de9bf895cfa6c51f53bb0c4


-- 
Alejandro Colomar
Linux man-pages comaintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

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

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Mike Kravetz]

On 6/9/22 06:24, Alejandro Colomar wrote:
> Hi Mike,
> 
> On 6/9/22 01:45, Mike Kravetz wrote:
>> Clarify that madvise only works on full pages, and remove references
>> to 'bytes'.
>>
>> Update MADV_DONTNEED and MADV_REMOVE sections to remove notes that
>> HugeTLB mappings are not supported.  Indicate the releases when they
>> were first supported as well as alignment restrictions.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> Acked-by: Peter Xu <peterx@redhat.com>
> 
> Please check some comments below.
> 
> Thanks,
>

Thank you!

And, my apologies for not looking at formatting requirements.
Will send a v4 shortly.

-- 
Mike Kravetz

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Peter Xu]

On Thu, Jun 09, 2022 at 11:48:06AM -0700, Mike Kravetz wrote:
> On 6/9/22 06:24, Alejandro Colomar wrote:
> > Hi Mike,
> > 
> > On 6/9/22 01:45, Mike Kravetz wrote:
> >> Clarify that madvise only works on full pages, and remove references
> >> to 'bytes'.
> >>
> >> Update MADV_DONTNEED and MADV_REMOVE sections to remove notes that
> >> HugeTLB mappings are not supported.  Indicate the releases when they
> >> were first supported as well as alignment restrictions.
> >>
> >> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> >> Acked-by: Peter Xu <peterx@redhat.com>
> > 
> > Please check some comments below.
> > 
> > Thanks,
> >
> 
> Thank you!
> 
> And, my apologies for not looking at formatting requirements.
> Will send a v4 shortly.

Alex,

Do you think we could add some of the semantic newline requirement into
CONTRIBUTING file explicitly?  Although there's a pointer to man7 man-pages
but the semantic newlines rules seem to be easily overlooked.

IMHO there can even be examples as you quoted in the link on "UNIX For
Beginners":

https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man7/man-pages.7?h=alx/main&id=6ff6f43d68164f99a8c3fb66f4525d145571310c

Maybe that'll ease both the maintainers and the contributors?

Thanks,

-- 
Peter Xu

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[David Hildenbrand]

On 09.06.22 22:13, Peter Xu wrote:
> On Thu, Jun 09, 2022 at 11:48:06AM -0700, Mike Kravetz wrote:
>> On 6/9/22 06:24, Alejandro Colomar wrote:
>>> Hi Mike,
>>>
>>> On 6/9/22 01:45, Mike Kravetz wrote:
>>>> Clarify that madvise only works on full pages, and remove references
>>>> to 'bytes'.
>>>>
>>>> Update MADV_DONTNEED and MADV_REMOVE sections to remove notes that
>>>> HugeTLB mappings are not supported.  Indicate the releases when they
>>>> were first supported as well as alignment restrictions.
>>>>
>>>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>>>> Acked-by: Peter Xu <peterx@redhat.com>
>>>
>>> Please check some comments below.
>>>
>>> Thanks,
>>>
>>
>> Thank you!
>>
>> And, my apologies for not looking at formatting requirements.
>> Will send a v4 shortly.
> 
> Alex,
> 
> Do you think we could add some of the semantic newline requirement into
> CONTRIBUTING file explicitly?  Although there's a pointer to man7 man-pages
> but the semantic newlines rules seem to be easily overlooked.
> 
> IMHO there can even be examples as you quoted in the link on "UNIX For
> Beginners":
> 
> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man7/man-pages.7?h=alx/main&id=6ff6f43d68164f99a8c3fb66f4525d145571310c
> 
> Maybe that'll ease both the maintainers and the contributors?

Maybe something similar checkpatch in Linux/qemu could point out a lot
of these possible issues and reduce Maintainer overhead. Nobody reads
docs after all if not forced to ;)


-- 
Thanks,

David / dhildenb

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2952 bytes --]

Hi, David and Peter!

On 6/10/22 11:01, David Hildenbrand wrote:
> On 09.06.22 22:13, Peter Xu wrote:
>> Alex,
>>
>> Do you think we could add some of the semantic newline requirement into
>> CONTRIBUTING file explicitly?


I plan to add something there by the end of this year more or less, when 
groff-1.23.0 is released.  See below.

>> Although there's a pointer to man7 man-pages
>> but the semantic newlines rules seem to be easily overlooked.
>>
>> IMHO there can even be examples as you quoted in the link on "UNIX For
>> Beginners":
>>
>> https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man7/man-pages.7?h=alx/main&id=6ff6f43d68164f99a8c3fb66f4525d145571310c

Yes, I think I'll point to the 'UNIX for Beginners::Hints for Preparing 
Documents' thing from man-pages(7) with a link.  Good idea.

>>
>> Maybe that'll ease both the maintainers and the contributors?
> 
> Maybe something similar checkpatch in Linux/qemu could point out a lot
> of these possible issues and reduce Maintainer overhead. Nobody reads
> docs after all if not forced to ;)
> 
> 

Actually, I was working on something like that.  I didn't tell because 
it's under development, and requires to build groff(1) from source from 
their git HEAD (I will probably make it more public when groff-1.23.0 is 
released).

It even runs checkpatch(1) (or a fork of it that I did to make it 
standalone)[1] for the EXAMPLES programs.

[1]: <http://www.alejandro-colomar.es/src/alx/linux/checkpatch.git/>

You can run `make -i lint` in the man-pages source tree _before_ 
applying your patch, to ignore all warnings not caused by you, and then 
run `make lint` _after_ your patch to see some warnings caused by your 
patch.  You'll also see warnings not created by you but that are in the 
same file that you modified, but you can ignore them.

It's not perfect, and currently it doesn't detect semantic newline 
problems, I think, but I guess it could be improved in the future.

There's also another make(1) target to compile and link the EXAMPLES 
programs, in case you want to try it, but I'm also working on it and 
still has many issues.  But if you want to try it, it's `make -i 
build-ld`, and then `make build-ld`.

You can try it, and maybe it helps you a bit.  Those (lint and build) 
targets also have subtargets, so you can use autocomplete to see what 
subtargets there are, and run only the ones you're interested in.  The 
Makefile may be too cryptic to try to read it from there, but you can 
try. :)  You'll also see a lot of EXTRA_*FLAGS to modify commands.

Since I expect groff-1.23.0 to be released by the end of this year, I 
think I'll wait for then to make these features public, and that'll give 
me some time to polish them.

Cheers,

Alex

-- 
Alejandro Colomar
Linux man-pages comaintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

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

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Peter Xu]

On Fri, Jun 10, 2022 at 11:38:53AM +0200, Alejandro Colomar wrote:
> It's not perfect, and currently it doesn't detect semantic newline problems,
> I think, but I guess it could be improved in the future.

Semantic newlines can be challenging as IIUC it's not deterministic?

I mean, I had a feeling that some paragraph could have multiple valid ways
to do the layout and newlines without violating the rule.  IMHO that could
be a challenging part for contributors.

(Or maybe the rule was deterministic but I didn't really fully digest it..)

But the tool (even without the newline detections) looks promising and
helpful.

Thanks,

-- 
Peter Xu

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1215 bytes --]

On 6/10/22 16:07, Peter Xu wrote:
> On Fri, Jun 10, 2022 at 11:38:53AM +0200, Alejandro Colomar wrote:
>> It's not perfect, and currently it doesn't detect semantic newline problems,
>> I think, but I guess it could be improved in the future.
> 
> Semantic newlines can be challenging as IIUC it's not deterministic?
> 
> I mean, I had a feeling that some paragraph could have multiple valid ways
> to do the layout and newlines without violating the rule.  IMHO that could
> be a challenging part for contributors.
> 
> (Or maybe the rule was deterministic but I didn't really fully digest it..)

You're completely right; it's not machine parseable.  But it could 
detect the easiest stuff such as "foo.  Bar" for example.  And only 
maybe "foo, bar".  But going further, that's a job for humans.

And yes, many times there are several ways to break the line while still 
following the rules.

> 
> But the tool (even without the newline detections) looks promising and
> helpful.

Thanks!  I'll try to make it usable.

Cheers,

Alex

> 
> Thanks,
> 


-- 
Alejandro Colomar
Linux man-pages comaintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

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

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 920 bytes --]

Hi Mike,

On 6/9/22 20:48, Mike Kravetz wrote:
> On 6/9/22 06:24, Alejandro Colomar wrote:
>> Hi Mike,
>>
>> On 6/9/22 01:45, Mike Kravetz wrote:
>>> Clarify that madvise only works on full pages, and remove references
>>> to 'bytes'.
>>>
>>> Update MADV_DONTNEED and MADV_REMOVE sections to remove notes that
>>> HugeTLB mappings are not supported.  Indicate the releases when they
>>> were first supported as well as alignment restrictions.
>>>
>>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>>> Acked-by: Peter Xu <peterx@redhat.com>
>>
>> Please check some comments below.
>>
>> Thanks,
>>
> 
> Thank you!
> 
> And, my apologies for not looking at formatting requirements.
> Will send a v4 shortly.
> 

I was revising old patches.  I'm just pinging you on this, to make sure 
it's not lost.

Thanks,

Alex


-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

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

Re: [PATCH v3] madvise.2: Clarify addr/length and update hugetlb support

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1104 bytes --]



On 8/15/22 22:41, Alejandro Colomar wrote:
> Hi Mike,
> 
> On 6/9/22 20:48, Mike Kravetz wrote:
>> On 6/9/22 06:24, Alejandro Colomar wrote:
>>> Hi Mike,
>>>
>>> On 6/9/22 01:45, Mike Kravetz wrote:
>>>> Clarify that madvise only works on full pages, and remove references
>>>> to 'bytes'.
>>>>
>>>> Update MADV_DONTNEED and MADV_REMOVE sections to remove notes that
>>>> HugeTLB mappings are not supported.  Indicate the releases when they
>>>> were first supported as well as alignment restrictions.
>>>>
>>>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>>>> Acked-by: Peter Xu <peterx@redhat.com>
>>>
>>> Please check some comments below.
>>>
>>> Thanks,
>>>
>>
>> Thank you!
>>
>> And, my apologies for not looking at formatting requirements.
>> Will send a v4 shortly.
>>
> 
> I was revising old patches.  I'm just pinging you on this, to make sure 
> it's not lost.

Never mind, I just checked that the patch was already applied.  I didn't 
find it by some error. :/

Cheers,

Alex

-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

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