hugetlb fixes

PATCH In review Joern Engel 4 participants 13 messages Jun 18, 2013 – Jul 3, 2013

Subsystems (3)

Memory management Memory mapping HugeTLB

lore

Share & tools

lore

Joern Engel

author

2013-06-18 20:17 UTC #0 lore raw

cover hugetlb fixes

As everyone knows, hugetlbfs sucks. But it is also necessary for large memory machines, so we should make it suck less. Top of my list were lack of rss accounting and refusing mmap with MAP_HUGETLB when using hugetlbfs. The latter generally created a know in every brain I explained this to.

Test program below is failing before these two patches and passing after.

Joern Engel (2):
hugetlb: properly account rss
mmap: allow MAP_HUGETLB for hugetlbfs files

 mm/hugetlb.c |    4 ++++
 mm/mmap.c    |   12 ++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

Signature · 92 lines

1.7.10.4

#define _GNU_SOURCE
#include <assert.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

typedef unsigned long long u64;

static size_t length = 1 << 24;

static u64 read_rss(void)
{
	char buf[4096], *s = buf;
	int i, fd;
	u64 rss;

	fd = open("/proc/self/statm", O_RDONLY);
	assert(fd > 2);
	memset(buf, 0, sizeof(buf));
	read(fd, buf, sizeof(buf) - 1);
	for (i = 0; i < 1; i++)
		s = strchr(s, ' ') + 1;
	rss = strtoull(s, NULL, 10);
	return rss << 12; /* assumes 4k pagesize */
}

static void do_mmap(int fd, int extra_flags, int unmap)
{
	int *p;
	int flags = MAP_PRIVATE | MAP_POPULATE | extra_flags;
	u64 before, after;

	before = read_rss();
	p = mmap(NULL, length, PROT_READ | PROT_WRITE, flags, fd, 0);
	assert(p != MAP_FAILED ||
			!"mmap returned an unexpected error");
	after = read_rss();
	assert(llabs(after - before - length) < 0x40000 ||
			!"rss didn't grow as expected");
	if (!unmap)
		return;
	munmap(p, length);
	after = read_rss();
	assert(llabs(after - before) < 0x40000 ||
			!"rss didn't shrink as expected");
}

static int open_file(const char *path)
{
	int fd, err;

	unlink(path);
	fd = open(path, O_CREAT | O_RDWR | O_TRUNC | O_EXCL
			| O_LARGEFILE | O_CLOEXEC, 0600);
	assert(fd > 2);
	unlink(path);
	err = ftruncate(fd, length);
	assert(!err);
	return fd;
}

int main(void)
{
	int hugefd, fd;

	fd = open_file("/dev/shm/hugetlbhog");
	hugefd = open_file("/hugepages/hugetlbhog");

	system("echo 100 > /proc/sys/vm/nr_hugepages");
	do_mmap(-1, MAP_ANONYMOUS, 1);
	do_mmap(fd, 0, 1);
	do_mmap(-1, MAP_ANONYMOUS | MAP_HUGETLB, 1);
	do_mmap(hugefd, 0, 1);
	do_mmap(hugefd, MAP_HUGETLB, 1);
	/* Leak the last one to test do_exit() */
	do_mmap(-1, MAP_ANONYMOUS | MAP_HUGETLB, 0);
	printf("oll korrekt.\n");
	return 0;
}

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

Joern Engel

author

2013-06-18 20:17 UTC #1 lore raw

PATCH 1/2 hugetlb: properly account rss

When moving a program from mmap'ing small pages to mmap'ing huge pages, a remarkable drop in rss ensues. For some reason hugepages were never accounted for in rss, which in my book is a clear bug. Sadly this bug has been present in hugetlbfs since it was merged back in 2002. There is every chance existing programs depend on hugepages not being counted as rss.

I think the correct solution is to fix the bug and wait for someone to complain. It is just as likely that noone cares - as evidenced by the fact that noone seems to have noticed for ten years.

Signed-off-by Joern Engel <[email protected]>

---
 mm/hugetlb.c |    4 ++++
 1 file changed, 4 insertions(+)

mm/hugetlb.c +4 -0

index 1a12f5b..705036c 100644
@@ -1174,6 +1174,7 @@ static struct page *alloc_huge_page(struct vm_area_struct *vma,
 	set_page_private(page, (unsigned long)spool);
 
 	vma_commit_reservation(h, vma, addr);
+	add_mm_counter(vma->vm_mm, MM_ANONPAGES, pages_per_huge_page(h));
 	return page;
 }

@@ -2406,6 +2407,9 @@ again:
 		if (pte_dirty(pte))
 			set_page_dirty(page);
 
+		/* -pages_per_huge_page(h) wouldn't get sign-extended */
+		add_mm_counter(vma->vm_mm, MM_ANONPAGES, -1 << h->order);
+
 		page_remove_rmap(page);
 		force_flush = !__tlb_remove_page(tlb, page);
 		if (force_flush)

Signature · 7 lines

1.7.10.4

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

Andrew Morton

maintainer

2013-06-28 22:03 UTC #2 lore raw

Re: [PATCH 1/2] hugetlb: properly account rss

Quoted 11 lines · On Tue, 18 Jun 2013 14:47:04 -0400 Joern Engel <[email protected]> wrote:

When moving a program from mmap'ing small pages to mmap'ing huge pages, a remarkable drop in rss ensues. For some reason hugepages were never accounted for in rss, which in my book is a clear bug. Sadly this bug has been present in hugetlbfs since it was merged back in 2002. There is every chance existing programs depend on hugepages not being counted as rss.
I think the correct solution is to fix the bug and wait for someone to complain. It is just as likely that noone cares - as evidenced by the fact that noone seems to have noticed for ten years.

Yes, that is a concern. I'll toss it in there so we can see what happens, but I fear that any problems will take a long time to be discovered.

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

Steve Capper

2013-07-03 13:41 UTC #3 lore raw

Re: [PATCH 1/2] hugetlb: properly account rss

In reply to Joern Engel

Quoted 7 lines · Joern Engel

When moving a program from mmap'ing small pages to mmap'ing huge pages, a remarkable drop in rss ensues. For some reason hugepages were never accounted for in rss, which in my book is a clear bug. Sadly this bug has been present in hugetlbfs since it was merged back in 2002. There is every chance existing programs depend on hugepages not being counted as rss.

Quoted 9 lines

I think the correct solution is to fix the bug and wait for someone to complain. It is just as likely that noone cares - as evidenced by the fact that noone seems to have noticed for ten years.
Signed-off-by Joern Engel <[email protected]>
---
 mm/hugetlb.c |    4 ++++
 1 file changed, 4 insertions(+)

Hi,
This patch has caused a few warnings for me today when it was integrated into linux-next. The libhugetlbfs test suite gave me:

[ 94.320661] BUG: Bad rss-counter state mm:ffff880119461040 idx:1 val:-512
[ 94.330346] BUG: Bad rss-counter state mm:ffff880119460680 idx:1 val:-2560
[ 94.341746] BUG: Bad rss-counter state mm:ffff880119460d00 idx:1 val:-512
[ 94.347518] BUG: Bad rss-counter state mm:ffff880119460d00 idx:1 val:-512
[ 94.415203] BUG: Bad rss-counter state mm:ffff8801194f9040 idx:1 val:-1024

[ ...]

I think I've found the cause; MAP_SHARED mappings. alloc_huge_page and __unmap_hugepage_range are called for shared pages. Also, __unmap_hugepage_range is called more times than alloc_huge_page (which makes sense as multiple views of a shared mapping are unmapped) leading to negative counter values.

Excluding VM_SHARED VMAs from the counter increment/decrement stopped the warnings for me. Although this may not be the best way to address the issue.

Cheers,

Signature · 7 lines

Steve

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

Steve Capper

2013-07-03 14:56 UTC #4 lore raw

Re: [PATCH 1/2] hugetlb: properly account rss

On Wed, Jul 03, 2013 at 02:41:19PM +0100, Steve Capper wrote:

[ ... ]

Quoted 2 lines

Excluding VM_SHARED VMAs from the counter increment/decrement stopped the warnings for me.

Whoops sorry, the fork copy on write test flagged a BUG, hugetlb_cow will need to be examined too.

Cheers,

Signature · 7 lines

Steve

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

Joern Engel

author

2013-06-18 20:17 UTC #5 lore raw

PATCH 2/2 mmap: allow MAP_HUGETLB for hugetlbfs files

In reply to Joern Engel

It is counterintuitive at best that mmap'ing a hugetlbfs file with MAP_HUGETLB fails, while mmap'ing it without will a) succeed and b) return huge pages.

Signed-off-by Joern Engel <[email protected]>

---
 mm/mmap.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/mmap.c +10 -2

index 2a594246..76eb6df 100644
@@ -33,6 +33,7 @@
 #include <linux/uprobes.h>
 #include <linux/rbtree_augmented.h>
 #include <linux/sched/sysctl.h>
+#include <linux/magic.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>

@@ -1313,6 +1314,11 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	return addr;
 }
 
+static inline int is_hugetlb_file(struct file *file)
+{
+	return file->f_inode->i_sb->s_magic == HUGETLBFS_MAGIC;
+}
+
 SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 		unsigned long, prot, unsigned long, flags,
 		unsigned long, fd, unsigned long, pgoff)

@@ -1322,11 +1328,12 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 
 	if (!(flags & MAP_ANONYMOUS)) {
 		audit_mmap_fd(fd, flags);
-		if (unlikely(flags & MAP_HUGETLB))
-			return -EINVAL;
 		file = fget(fd);
 		if (!file)
 			goto out;
+		retval = -EINVAL;
+		if (unlikely(flags & MAP_HUGETLB && !is_hugetlb_file(file)))
+			goto out_fput;
 	} else if (flags & MAP_HUGETLB) {
 		struct user_struct *user = NULL;
 		/*

@@ -1346,6 +1353,7 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 	flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
 
 	retval = vm_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+out_fput:
 	if (file)
 		fput(file);
 out:

Signature · 7 lines

1.7.10.4

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

Jianguo Wu

2013-06-19 01:23 UTC #6 lore raw

Re: [PATCH 2/2] mmap: allow MAP_HUGETLB for hugetlbfs files

Hi Joern，

Quoted 34 lines · Joern Engel

It is counterintuitive at best that mmap'ing a hugetlbfs file with MAP_HUGETLB fails, while mmap'ing it without will a) succeed and b) return huge pages.

Signed-off-by Joern Engel <[email protected]>

---
 mm/mmap.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

mm/mmap.c +6 -0

index 2a594246..76eb6df 100644
@@ -33,6 +33,7 @@
 #include <linux/uprobes.h>
 #include <linux/rbtree_augmented.h>
 #include <linux/sched/sysctl.h>
+#include <linux/magic.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>

@@ -1313,6 +1314,11 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
 	return addr;
 }
 
+static inline int is_hugetlb_file(struct file *file)
+{
+	return file->f_inode->i_sb->s_magic == HUGETLBFS_MAGIC;
+}
+
 SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 		unsigned long, prot, unsigned long, flags,
 		unsigned long, fd, unsigned long, pgoff)

mm/mmap.c SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len, · line 1335

Quoted 10 lines

mm/mmap.c +2 -2

@@ -1322,11 +1328,12 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 
 	if (!(flags & MAP_ANONYMOUS)) {
 		audit_mmap_fd(fd, flags);
-		if (unlikely(flags & MAP_HUGETLB))
-			return -EINVAL;
 		file = fget(fd);
 		if (!file)
 			goto out;
+		retval = -EINVAL;
+		if (unlikely(flags & MAP_HUGETLB && !is_hugetlb_file(file)))

We already have is_file_hugepages().

Thanks,
Jianguo Wu

Quoted 12 lines

mm/mmap.c +2 -0

+			goto out_fput;
 	} else if (flags & MAP_HUGETLB) {
 		struct user_struct *user = NULL;
 		/*

@@ -1346,6 +1353,7 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 	flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
 
 	retval = vm_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+out_fput:
 	if (file)
 		fput(file);
 out:

Jörn Engel

author

2013-06-19 17:55 UTC #7 lore raw

PATCH 1/1 mmap: allow MAP_HUGETLB for hugetlbfs files v2

It is counterintuitive at best that mmap'ing a hugetlbfs file with MAP_HUGETLB fails, while mmap'ing it without will a) succeed and b) return huge pages.
v2: use is_file_hugepages(), as suggested by Jianguo

Signed-off-by Joern Engel <[email protected]>

Cc Jianguo Wu <[email protected]>

---
 mm/mmap.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/mmap.c +4 -2

index 2a594246..cdc8e7a 100644
@@ -1322,11 +1322,12 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 
 	if (!(flags & MAP_ANONYMOUS)) {
 		audit_mmap_fd(fd, flags);
-		if (unlikely(flags & MAP_HUGETLB))
-			return -EINVAL;
 		file = fget(fd);
 		if (!file)
 			goto out;
+		retval = -EINVAL;
+		if (unlikely(flags & MAP_HUGETLB && !is_file_hugepages(file)))
+			goto out_fput;
 	} else if (flags & MAP_HUGETLB) {
 		struct user_struct *user = NULL;
 		/*

@@ -1346,6 +1347,7 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 	flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
 
 	retval = vm_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+out_fput:
 	if (file)
 		fput(file);
 out:

Signature · 7 lines

1.7.10.4

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

Jianguo Wu

2013-06-20 01:32 UTC #8 lore raw

Re: [PATCH] mmap: allow MAP_HUGETLB for hugetlbfs files v2

Hi Joern,

I cannot apply this patch to Linus tree, as the code has been modified since commit af73e4d9506d ("hugetlbfs: fix mmap failure in unaligned size request").

Thanks,
Jianguo Wu

Quoted 38 lines · Jörn Engel

Signed-off-by Joern Engel <[email protected]>

Cc Jianguo Wu <[email protected]>

---
 mm/mmap.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

mm/mmap.c +4 -2

index 2a594246..cdc8e7a 100644
@@ -1322,11 +1322,12 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 
 	if (!(flags & MAP_ANONYMOUS)) {
 		audit_mmap_fd(fd, flags);
-		if (unlikely(flags & MAP_HUGETLB))
-			return -EINVAL;
 		file = fget(fd);
 		if (!file)
 			goto out;
+		retval = -EINVAL;
+		if (unlikely(flags & MAP_HUGETLB && !is_file_hugepages(file)))
+			goto out_fput;
 	} else if (flags & MAP_HUGETLB) {
 		struct user_struct *user = NULL;
 		/*

@@ -1346,6 +1347,7 @@ SYSCALL_DEFINE6(mmap_pgoff, unsigned long, addr, unsigned long, len,
 	flags &= ~(MAP_EXECUTABLE | MAP_DENYWRITE);
 
 	retval = vm_mmap_pgoff(file, addr, len, prot, flags, pgoff);
+out_fput:
 	if (file)
 		fput(file);
 out:

Jörn Engel

author

2013-06-19 17:55 UTC #9 lore raw

Re: [PATCH 2/2] mmap: allow MAP_HUGETLB for hugetlbfs files

In reply to Jianguo Wu

Quoted 2 lines · Jianguo Wu

We already have is_file_hugepages().

Indeed. Much nicer now. Thanks!

JA?rn

--
The grand essentials of happiness are: something to do, something to love, and something to hope for.
-- Allan K. Chalmers

Jörn Engel

author

2013-06-18 20:20 UTC #10 lore raw

In reply to Joern Engel

Quoted 3 lines · Joern Engel

Test program below is failing before these two patches and passing after.

Actually, do we have a place to stuff kernel tests? And if not, should we have one?

JA?rn

--
My second remark is that our intellectual powers are rather geared to master static relations and that our powers to visualize processes evolving in time are relatively poorly developed.
-- Edsger W. Dijkstra

Andrew Morton

maintainer

2013-06-18 20:27 UTC #11 lore raw

Quoted 7 lines · On Tue, 18 Jun 2013 14:50:55 -0400 J__rn Engel <[email protected]> wrote:

Quoted 3 lines · Joern Engel
Test program below is failing before these two patches and passing after.
Actually, do we have a place to stuff kernel tests? And if not, should we have one?

Yep, tools/testing/selftests/vm. It's pretty simple and stupid at present - it anything about the framework irritates you, please fix it!

General guidelines for tools/testing/selftests: the tool should execute quickly and shouldn't break the overall selftests run at either compile time or runtime if kernel features are absent, Kconfig is unexpected, etc.

It's more a "place to accumulate and maintain selftest programs" than a serious self-testing framework.

Jörn Engel

author

2013-06-18 21:34 UTC #12 lore raw

Quoted 4 lines · Andrew Morton

Quoted 2 lines · On Tue, 18 Jun 2013 14:50:55 -0400 J__rn Engel <[email protected]> wrote:
Quoted 1 line · Joern Engel

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Test program below is failing before these two patches and passing after.
Actually, do we have a place to stuff kernel tests? And if not, should we have one?
Yep, tools/testing/selftests/vm. It's pretty simple and stupid at present - it anything about the framework irritates you, please fix it!

Just did. :)

JA?rn

--
Maintenance in other professions and of other articles is concerned with the return of the item to its original state; in Software, maintenance is concerned with moving an item away from its original state. -- Les Belady