[patch 1/2] mm, doc: cleanup and clarify munmap behavior for hugetlb memory

munmap(2) of hugetlb memory requires a length that is hugepage aligned,
otherwise it may fail.  Add this to the documentation.

This also cleans up the documentation and separates it into logical
units: one part refers to MAP_HUGETLB and another part refers to
requirements for shared memory segments.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 Documentation/vm/hugetlbpage.txt | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/Documentation/vm/hugetlbpage.txt b/Documentation/vm/hugetlbpage.txt
--- a/Documentation/vm/hugetlbpage.txt
+++ b/Documentation/vm/hugetlbpage.txt

@@ -289,15 +289,20 @@ file systems, write system calls are not.
 Regular chown, chgrp, and chmod commands (with right permissions) could be
 used to change the file attributes on hugetlbfs.
 
-Also, it is important to note that no such mount command is required if the
+Also, it is important to note that no such mount command is required if
 applications are going to use only shmat/shmget system calls or mmap with
-MAP_HUGETLB.  Users who wish to use hugetlb page via shared memory segment
-should be a member of a supplementary group and system admin needs to
-configure that gid into /proc/sys/vm/hugetlb_shm_group.  It is possible for
-same or different applications to use any combination of mmaps and shm*
-calls, though the mount of filesystem will be required for using mmap calls
-without MAP_HUGETLB.  For an example of how to use mmap with MAP_HUGETLB see
-map_hugetlb.c.
+MAP_HUGETLB.  For an example of how to use mmap with MAP_HUGETLB see map_hugetlb
+below.
+
+Users who wish to use hugetlb memory via shared memory segment should be a
+member of a supplementary group and system admin needs to configure that gid
+into /proc/sys/vm/hugetlb_shm_group.  It is possible for same or different
+applications to use any combination of mmaps and shm* calls, though the mount of
+filesystem will be required for using mmap calls without MAP_HUGETLB.
+
+When using munmap(2) to unmap hugetlb memory, the length specified must be
+hugepage aligned, otherwise it will fail with errno set to EINVAL.
+
 
 Examples
 ========

When MAP_HUGETLB memory is unmapped, the length must be hugepage aligned,
otherwise it fails with -EINVAL.

All tests currently behave correctly, but it's better to explcitly test
the return value for completeness and document the requirement,
especially if users copy map_hugetlb.c as a sample implementation.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 tools/testing/selftests/powerpc/mm/hugetlb_vs_thp_test.c | 8 ++++++--
 tools/testing/selftests/vm/hugetlbfstest.c               | 4 +++-
 tools/testing/selftests/vm/map_hugetlb.c                 | 6 +++++-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/powerpc/mm/hugetlb_vs_thp_test.c b/tools/testing/selftests/powerpc/mm/hugetlb_vs_thp_test.c
--- a/tools/testing/selftests/powerpc/mm/hugetlb_vs_thp_test.c
+++ b/tools/testing/selftests/powerpc/mm/hugetlb_vs_thp_test.c

@@ -21,9 +21,13 @@ static int test_body(void)
 		 * Typically the mmap will fail because no huge pages are
 		 * allocated on the system. But if there are huge pages
 		 * allocated the mmap will succeed. That's fine too, we just
-		 * munmap here before continuing.
+		 * munmap here before continuing.  munmap() length of
+		 * MAP_HUGETLB memory must be hugepage aligned.
 		 */
-		munmap(addr, SIZE);
+		if (munmap(addr, SIZE)) {
+			perror("munmap");
+			return 1;
+		}
 	}
 
 	p = mmap(addr, SIZE, PROT_READ | PROT_WRITE,

diff --git a/tools/testing/selftests/vm/hugetlbfstest.c b/tools/testing/selftests/vm/hugetlbfstest.c
--- a/tools/testing/selftests/vm/hugetlbfstest.c
+++ b/tools/testing/selftests/vm/hugetlbfstest.c

@@ -34,6 +34,7 @@ static void do_mmap(int fd, int extra_flags, int unmap)
 	int *p;
 	int flags = MAP_PRIVATE | MAP_POPULATE | extra_flags;
 	u64 before, after;
+	int ret;
 
 	before = read_rss();
 	p = mmap(NULL, length, PROT_READ | PROT_WRITE, flags, fd, 0);

@@ -44,7 +45,8 @@ static void do_mmap(int fd, int extra_flags, int unmap)
 			!"rss didn't grow as expected");
 	if (!unmap)
 		return;
-	munmap(p, length);
+	ret = munmap(p, length);
+	assert(!ret || !"munmap returned an unexpected error");
 	after = read_rss();
 	assert(llabs(after - before) < 0x40000 ||
 			!"rss didn't shrink as expected");

diff --git a/tools/testing/selftests/vm/map_hugetlb.c b/tools/testing/selftests/vm/map_hugetlb.c
--- a/tools/testing/selftests/vm/map_hugetlb.c
+++ b/tools/testing/selftests/vm/map_hugetlb.c

@@ -73,7 +73,11 @@ int main(void)
 	write_bytes(addr);
 	ret = read_bytes(addr);
 
-	munmap(addr, LENGTH);
+	/* munmap() length of MAP_HUGETLB memory must be hugepage aligned */
+	if (munmap(addr, LENGTH)) {
+		perror("munmap");
+		exit(1);
+	}
 
 	return ret;
 }

On Thu, 2015-03-26 at 16:23 -0700, David Rientjes wrote:

Acked-by: Michael Ellerman <mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>

cheers

On Thu, 26 Mar 2015, David Rientjes wrote:

munmap(2) of hugetlb memory requires a length that is hugepage aligned,
otherwise it may fail.  Add this to the documentation.

This also cleans up the documentation and separates it into logical
units: one part refers to MAP_HUGETLB and another part refers to
requirements for shared memory segments.

Signed-off-by: David Rientjes <redacted>
---

If this is the route we are going to take, this behavoir needs to be
called out prominently in the mmap/munmap man page.

On Fri, 27 Mar 2015, Eric B Munson wrote:

quoted

munmap(2) of hugetlb memory requires a length that is hugepage aligned,
otherwise it may fail.  Add this to the documentation.

This also cleans up the documentation and separates it into logical
units: one part refers to MAP_HUGETLB and another part refers to
requirements for shared memory segments.

Signed-off-by: David Rientjes <rientjes@google.com>
---

If this is the route we are going to take, this behavoir needs to be
called out prominently in the mmap/munmap man page.

Yeah, that was my next step, but before we get mtk involved I was trying 
to get this merged since man2/mmap.2 already has a 
.I Documentation/vm/hugetlbpage.txt for MAP_HUGETLB so the man page patch 
can simply reference this addition to the file as justification.

On Thu, 26 Mar 2015, David Rientjes wrote:

munmap(2) of hugetlb memory requires a length that is hugepage aligned,
otherwise it may fail.  Add this to the documentation.

Thanks for taking this on, David.  But although munmap(2) is the one
Davide called out, it goes beyond that, doesn't it?  To mprotect and
madvise and ...

I don't want to work out the list myself: is_vm_hugetlb_page() is
special-cased all over, and different syscalls react differently.

Which is another reason why, like you, I much prefer not to interfere
with the long established behavior: it would be very easy to introduce
bugs and worse inconsistencies.

And mprotect(2) is a good example of why we should not mess around
with the long established API here: changing an mprotect from failing
on a particular size to acting on a larger size is not a safe change.

Eric, I apologize for bringing you in to the discussion, and then
ignoring your input.  I understand that you would like MAP_HUGETLB
to behave more understandably.  We can all agree that the existing
behavior is unsatisfying.  But it's many years too late now to 
change it around - and I suspect that a full exercise to do so would
actually discover some good reasons why the original choices were made.

Perhaps just adding something like "The same is true for mprotect(2)
and other such memory system calls." is good enough for here.

+
 
 Examples
 ========

On Sun, 29 Mar 2015, Hugh Dickins wrote:

On Thu, 26 Mar 2015, David Rientjes wrote:

quoted

munmap(2) of hugetlb memory requires a length that is hugepage aligned,
otherwise it may fail.  Add this to the documentation.

Thanks for taking this on, David.  But although munmap(2) is the one
Davide called out, it goes beyond that, doesn't it?  To mprotect and
madvise and ...

I don't want to work out the list myself: is_vm_hugetlb_page() is
special-cased all over, and different syscalls react differently.

Which is another reason why, like you, I much prefer not to interfere
with the long established behavior: it would be very easy to introduce
bugs and worse inconsistencies.

And mprotect(2) is a good example of why we should not mess around
with the long established API here: changing an mprotect from failing
on a particular size to acting on a larger size is not a safe change.

Eric, I apologize for bringing you in to the discussion, and then
ignoring your input.  I understand that you would like MAP_HUGETLB
to behave more understandably.  We can all agree that the existing
behavior is unsatisfying.  But it's many years too late now to 
change it around - and I suspect that a full exercise to do so would
actually discover some good reasons why the original choices were made.

No worries, my main concern was avoiding the confusion that led me down
the rabbit hole of compaction and mlock.  As long as the documentation,
man pages, and the code all agree I am satisfied.  I would have
preferred to make the code match the docs, but I understand that
changing the code now introduces a risk of breaking userspace.

It is charitable of you to assume that there were good reasons for the
original decision.  But as the author of the code in question, I suspect
the omission was one of my own inexperience.

Eric

On Mon, 30 Mar 2015, Eric B Munson wrote:

On Sun, 29 Mar 2015, Hugh Dickins wrote:

quoted

Eric, I apologize for bringing you in to the discussion, and then
ignoring your input.  I understand that you would like MAP_HUGETLB
to behave more understandably.  We can all agree that the existing
behavior is unsatisfying.  But it's many years too late now to 
change it around - and I suspect that a full exercise to do so would
actually discover some good reasons why the original choices were made.

No worries, my main concern was avoiding the confusion that led me down
the rabbit hole of compaction and mlock.  As long as the documentation,
man pages, and the code all agree I am satisfied.  I would have
preferred to make the code match the docs, but I understand that
changing the code now introduces a risk of breaking userspace.

It is charitable of you to assume that there were good reasons for the
original decision.  But as the author of the code in question, I suspect
the omission was one of my own inexperience.

No, you are both too modest and too arrogant :)

You were extending the existing hugetlbfs infrastructure to be
accessible through a MAP_HUGETLB interface.  You therefore inherited
the defects (some probably necessary, others perhaps not) of the
original hugetlbfs implementation, which is where this disagreeable
behaviour comes from.

If you were to ask for MAP_HUGETLB to behave differently from mapping
hugetlbfs here, I would shout no.  For a start, we'd have to add a
VM_HUGETLB2 flag so that each place that tests VM_HUGETLB (usually
through is_vm_hugetlb_page(vma) - sic) could decide how to behave
instead.

I for one have neither time nor inclination to write or review
any such patch.

Hugh

On Sun, 29 Mar 2015, Hugh Dickins wrote:

quoted

munmap(2) of hugetlb memory requires a length that is hugepage aligned,
otherwise it may fail.  Add this to the documentation.

Thanks for taking this on, David.  But although munmap(2) is the one
Davide called out, it goes beyond that, doesn't it?  To mprotect and
madvise and ...

Yes, good point, munmap(2) isn't special in this case, the alignment to 
the native page size of the platform should apply to madvise, mbind, 
mincore, mlock, mprotect, remap_file_pages, etc.

I'd hesitate to compile any authoritative list on the behavior in 
Documentation/vm/hugetlbpage.txt since it would exclude future extensions, 
but I'll update it to be more inclusive of other mm syscalls rather than 
specify only munmap(2).

Don't only specify munmap(2) behavior with respect the hugetlb memory, all 
other syscalls get naturally aligned to the native page size of the 
processor.  Rather, pick out munmap(2) as a specific example.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 Documentation/vm/hugetlbpage.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/vm/hugetlbpage.txt b/Documentation/vm/hugetlbpage.txt
index 1270fb1..030977f 100644
--- a/Documentation/vm/hugetlbpage.txt
+++ b/Documentation/vm/hugetlbpage.txt

@@ -313,8 +313,11 @@ into /proc/sys/vm/hugetlb_shm_group.  It is possible for same or different
 applications to use any combination of mmaps and shm* calls, though the mount of
 filesystem will be required for using mmap calls without MAP_HUGETLB.
 
-When using munmap(2) to unmap hugetlb memory, the length specified must be
-hugepage aligned, otherwise it will fail with errno set to EINVAL.
+Syscalls that operate on memory backed by hugetlb pages only have their lengths
+aligned to the native page size of the processor; they will normally fail with
+errno set to EINVAL or exclude hugetlb pages that extend beyond the length if
+not hugepage aligned.  For example, munmap(2) will fail if memory is backed by
+a hugetlb page and the length is smaller than the hugepage size.
 
 
 Examples

--

To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Thu, 2 Apr 2015, David Rientjes wrote:

Don't only specify munmap(2) behavior with respect the hugetlb memory, all 
other syscalls get naturally aligned to the native page size of the 
processor.  Rather, pick out munmap(2) as a specific example.

Signed-off-by: David Rientjes <rientjes@google.com>

Thanks, yes, good wording: it is best to be a bit vague here,
since each msyscall takes the approach most convenient for it.

Acked-by: Hugh Dickins <hughd@google.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Thu, 2 Apr 2015 15:50:15 -0700 (PDT)
David Rientjes [off-list ref] wrote:

Don't only specify munmap(2) behavior with respect the hugetlb memory, all 
other syscalls get naturally aligned to the native page size of the 
processor.  Rather, pick out munmap(2) as a specific example.

So I was going to apply this to the docs tree, but it doesn't even come
close.  What tree was this patch generated against?

Thanks,

jon

On Sat, 4 Apr 2015, Jonathan Corbet wrote:

On Thu, 2 Apr 2015 15:50:15 -0700 (PDT)
David Rientjes [off-list ref] wrote:

quoted

Don't only specify munmap(2) behavior with respect the hugetlb memory, all 
other syscalls get naturally aligned to the native page size of the 
processor.  Rather, pick out munmap(2) as a specific example.

So I was going to apply this to the docs tree, but it doesn't even come
close.  What tree was this patch generated against?

Sorry, it's not intended to go through the docs tree, it's a patch to fix 
mm-doc-cleanup-and-clarify-munmap-behavior-for-hugetlb-memory.patch in 
-mm.  It's been merged into that tree, but I would still appreciate your 
ack!

On Thu, 9 Apr 2015 12:46:09 -0700 (PDT)
David Rientjes [off-list ref] wrote:

It's been merged into that tree, but I would still appreciate your 
ack!

Easy enough.

Acked-by: Jonathan Corbet <corbet@lwn.net>

jon