Skip to contentSkip to conversation

Test for memfd_create() syscall with MFD_HUGETLB flag.

PATCH Superseded Vishnu K 2 participants 3 messages Jul 20, 2018 – Jul 25, 2018

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Vishnu K

author
#0 lore raw

PATCH v2 1/1 Test for memfd_create() syscall with MFD_HUGETLB flag.

memfd_create03.c: Tests the syscall for the flag MFD_HUGETLB.

memfd_create04.c: Tests the syscall for the flags MFD_HUGE_2MB, MFD_HUGE_1GB,... used in conjunction with MFD_HUGETLB flag.

memfd_create_hugetlb.c: Contains helper functions for memfd_create03.c and memfd_create04.c.

-- major changes from v1 to v2 --

Restore the original number of hugepages in cleanup().

Modify check_mfd_new() from memfd_create_common and replace safe_huge_new() + safe_huge_x_new() from memfd_create_hugetlb with it.

Modify get_from_file() from memfd_create_hugetlb to use FILE_LINES_SCANF in code logic.

Move test dependent flags to include/lapi

---
 include/lapi/memfd.h                          |  34 ++++
 runtest/syscalls                              |   2 +
 .../kernel/syscalls/memfd_create/.gitignore   |   2 +
 .../kernel/syscalls/memfd_create/Makefile     |   4 +-
 .../syscalls/memfd_create/memfd_create03.c    | 165 +++++++++++++++++
 .../syscalls/memfd_create/memfd_create04.c    | 140 ++++++++++++++
 .../memfd_create/memfd_create_common.c        |  20 +-
 .../memfd_create/memfd_create_hugetlb.c       | 173 ++++++++++++++++++
 .../memfd_create/memfd_create_hugetlb.h       |  60 ++++++
 9 files changed, 594 insertions(+), 6 deletions(-)
 create mode 100644 testcases/kernel/syscalls/memfd_create/memfd_create03.c
 create mode 100644 testcases/kernel/syscalls/memfd_create/memfd_create04.c
 create mode 100644 testcases/kernel/syscalls/memfd_create/memfd_create_hugetlb.c
 create mode 100644 testcases/kernel/syscalls/memfd_create/memfd_create_hugetlb.h
include/lapi/memfd.h +34 -0
index 18ed40fc6..1f92a18aa 100644
@@ -24,4 +24,38 @@
 # define MFD_ALLOW_SEALING       0x0002U
 #endif
 
+/* flags for memfd_create(3) and memfd_create(4) */
+#ifndef MFD_HUGETLB
+#define MFD_HUGETLB 0x0004U
+#endif
+
+#ifndef MFD_HUGE_64KB
+#define MFD_HUGE_64KB (16 << 26)
+#endif
+#ifndef MFD_HUGE_512KB
+#define MFD_HUGE_512KB (19 << 26)
+#endif
+#ifndef MFD_HUGE_2MB
+#define MFD_HUGE_2MB (21 << 26)
+#endif
+#ifndef MFD_HUGE_8MB
+#define MFD_HUGE_8MB (23 << 26)
+#endif
+#ifndef MFD_HUGE_16MB
+#define MFD_HUGE_16MB (24 << 26)
+#endif
+#ifndef MFD_HUGE_256MB
+#define MFD_HUGE_256MB (28 << 26)
+#endif
+#ifndef MFD_HUGE_1GB
+#define MFD_HUGE_1GB (30 << 26)
+#endif
+#ifndef MFD_HUGE_2GB
+#define MFD_HUGE_2GB (31 << 26)
+#endif
+#ifndef MFD_HUGE_16GB
+#define MFD_HUGE_16GB (34 << 26)
+#endif
+
+
 #endif
runtest/syscalls +2 -0
index a9afecf57..c310b0106 100644
@@ -1479,5 +1479,7 @@ futex_wait_bitset02 futex_wait_bitset02
 
 memfd_create01 memfd_create01
 memfd_create02 memfd_create02
+memfd_create03 memfd_create03
+memfd_create04 memfd_create04
 
 copy_file_range01 copy_file_range01
testcases/kernel/syscalls/memfd_create/.gitignore +2 -0
index 9b20604ab..b79adfa65 100644
@@ -1,2 +1,4 @@
 /memfd_create01
 /memfd_create02
+/memfd_create03
+/memfd_create04
testcases/kernel/syscalls/memfd_create/Makefile +2 -2
index f23b8732c..ec83a5c40 100644
@@ -16,8 +16,8 @@ top_srcdir		?= ../../../..
 
 include $(top_srcdir)/include/mk/testcases.mk
 
-FILTER_OUT_MAKE_TARGETS         := memfd_create_common
+FILTER_OUT_MAKE_TARGETS         := memfd_create_common memfd_create_hugetlb
 
 include $(top_srcdir)/include/mk/generic_leaf_target.mk
 
-$(MAKE_TARGETS): %: %.o memfd_create_common.o
+$(MAKE_TARGETS): %: %.o memfd_create_common.o memfd_create_hugetlb.o
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +165 -0

165 lines not on this page · raw message

testcases/kernel/syscalls/memfd_create/memfd_create04.c new file +140 -0

140 lines not on this page · raw message

testcases/kernel/syscalls/memfd_create/memfd_create_common.c +16 -4
index f22e3d345..186eafa51 100644
@@ -144,21 +144,33 @@ int mfd_flags_available(const char *filename, const int lineno,
 }
 
 int check_mfd_new(const char *filename, const int lineno,
-			const char *name, loff_t sz, int flags)
+		  const char *name, loff_t sz, int flags)
 {
 	int fd;
 
 	fd = sys_memfd_create(name, flags);
 	if (fd < 0) {
+		if ((flags & MFD_HUGETLB) == MFD_HUGETLB)
+			goto end;
+
 		tst_brk_(filename, lineno, TBROK | TERRNO,
-			"memfd_create(%s, %d) failed", name, flags);
+			 "memfd_create(%s, %d) failed", name, flags);
+	}
+
+	if ((flags & MFD_HUGETLB) == MFD_HUGETLB) {
+		tst_res_(filename, lineno, TINFO,
+			 "memfd_create(%s, %d) succeeded",
+			 name, flags);
+		goto end;
 	}
 
-	tst_res_(filename, lineno, TPASS, "memfd_create(%s, %d) succeeded",
-		name, flags);
+	tst_res_(filename, lineno, TPASS,
+		 "memfd_create(%s, %d) succeeded",
+		 name, flags);
 
 	check_ftruncate(filename, lineno, fd, sz);
 
+ end:
 	return fd;
 }
 
testcases/kernel/syscalls/memfd_create/memfd_create_hugetlb.c new file +173 -0

173 lines not on this page · raw message

testcases/kernel/syscalls/memfd_create/memfd_create_hugetlb.h new file +60 -0

60 lines not on this page · raw message

Signature · 1 line
2.17.1
#1 lore raw

PATCH v2 1/1 Test for memfd_create() syscall with MFD_HUGETLB flag.

----- Original Message -----

Quoted 13 lines

memfd_create03.c: Tests the syscall for the flag MFD_HUGETLB.

memfd_create04.c: Tests the syscall for the flags MFD_HUGE_2MB, MFD_HUGE_1GB,... used in conjunction with MFD_HUGETLB flag.

memfd_create_hugetlb.c: Contains helper functions for memfd_create03.c and memfd_create04.c.

-- major changes from v1 to v2 --

Restore the original number of hugepages in cleanup().

Quoted 8 lines

Modify check_mfd_new() from memfd_create_common and replace safe_huge_new() + safe_huge_x_new() from memfd_create_hugetlb with it.

Modify get_from_file() from memfd_create_hugetlb to use FILE_LINES_SCANF in code logic.

Move test dependent flags to include/lapi

Hi,

<snip>

testcases/kernel/syscalls/memfd_create/memfd_create03.c line 53

Quoted 8 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +8 -0
+
+static void test_write_protect(int fd)
+{
+	if (CHECK_HUGE_NON_WRITEABLE(fd) == 0)
+		tst_res(TPASS, "write call failed as expected");
+	else
+		tst_res(TFAIL, "write call passed unexpectedly");
+}

This looks unnecessarily complicated.

You have macro, that calls functions, which is printing result text as TINFO, then returning retcode based on which you decide if to report PASS or FAIL.

Why not report PASS/FAIL directly? Or why not put the test right here, without all the indirection? Are you planning on using CHECK_HUGE_NON_WRITEABLE() in other tests?

Quoted 6 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +6 -0
+
+static void test_def_pagesize(int fd)
+{
+	int ret;
+	long hps;
+	void *mem;

testcases/kernel/syscalls/memfd_create/memfd_create03.c line 69

Quoted 10 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +10 -0
+
+	hps = GET_HUGE_DEF_PAGESIZE();
+	mem = CHECK_HUGE_MMAPABLE(fd, hps);
+
+	ret = CHECK_MUNMAP_LT_PAGE(mem, hps);
+	if (ret == 0)
+		tst_res(TPASS, "File created in def pagesize %ldkB", hps/1024);
+	else
+		tst_res(TFAIL, "File is not in default pagesize");
+}

Similar here, are you planning on using check_huge_mmapable() in other tests?

Side effect of CHECK_HUGE_MMAPABLE is that it mmaps a page. You're not freeing that page.

Quoted 54 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +54 -0
+
+static void test_max_hugepages(int fd)
+{
+	int res;
+	int new_fd;
+	int dummy_fd;
+	long hps;
+	long free_pages;
+	void *mem;
+
+	free_pages = GET_HUGE_FREE_PAGES();
+	hps = GET_HUGE_DEF_PAGESIZE();
+	mem = CHECK_HUGE_MMAPABLE(fd, free_pages * hps);
+
+	new_fd = CHECK_MFD_NEW("new_file", 0, MFD_HUGETLB);
+	if (fd < 0)
+		tst_brk(TBROK | TERRNO, "memfd_create() failed");
+	res = CHECK_HUGE_NON_MMAPABLE(new_fd, hps);
+	dummy_fd = new_fd;
+	SAFE_CLOSE(new_fd);
+	tst_res(TINFO, "close(%d) succeeded", dummy_fd);
+
+	SAFE_MUNMAP(mem, free_pages * hps);
+
+	if (res == 0)
+		tst_res(TPASS, "Hugepages creation is limited as expected");
+	else
+		tst_res(TFAIL, "Hugepages creation limit failed");
+}
+
+static const struct tcase {
+	void (*func)(int fd);
+	const char *desc;
+} tcases[] = {
+	{&test_write_protect, "--TESTING WRITE CALL IN HUGEPAGES--"},
+	{&test_def_pagesize, "--TESTING PAGE SIZE OF CREATED FILE--"},
+	{&test_max_hugepages, "--TESTING THE NUMBER OF HUGEPAGES--"},
+};
+
+static void memfd_huge_controller(unsigned int n)
+{
+	int fd;
+	int dummy_fd;
+	const struct tcase *tc;
+
+	tc = &tcases[n];
+
+	tst_res(TINFO, "%s", tc->desc);
+
+	fd = CHECK_MFD_NEW("test_file", 0, MFD_HUGETLB);
+	if (fd < 0)
+		tst_brk(TBROK | TERRNO, "memfd_create() failed");
+
+	tc->func(fd);

testcases/kernel/syscalls/memfd_create/memfd_create03.c line 133

Quoted 10 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +10 -0
+
+	dummy_fd = fd;
+	SAFE_CLOSE(fd);
+	tst_res(TINFO, "close(%d) succeeded", dummy_fd);
+}
+
+static void setup(void)
+{
+	int fd;
+
# ./memfd_create03 
tst_test.c:1018: INFO: Timeout per run is 0h 05m 00s
memfd_create03.c:134: BROK: Unable to get total number of hugepages

Summary:

passed   0
failed   0
skipped  0

warnings 0

Check if hugepages are supported is needed too, for example:

if (access(PATH_HUGEPAGES, F_OK))
tst_brk(TCONF, "Huge page is not supported.");

Review

Quoted 2 lines
(quoted diff) +2 -0
+	if (GET_HUGE_TOTAL_PAGES() > 0)
+		return;

I thought you need 4 huge pages for the test. This skips setup even for 1.

testcases/kernel/syscalls/memfd_create/memfd_create03.c line 138

Quoted 3 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +3 -0
+
+	fd = SAFE_OPEN("/proc/sys/vm/nr_hugepages", O_WRONLY);
+	SAFE_WRITE(1, fd, "4", 1);

I'd suggest normal write, then read back the value from /proc to check if test can proceed.

testcases/kernel/syscalls/memfd_create/memfd_create03.c line 143

Quoted 5 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +5 -0
+	hugepages_enabled = 1;
+	SAFE_CLOSE(fd);
+
+	if (GET_HUGE_TOTAL_PAGES() == 0)
+		tst_brk(TCONF, "Enable Hugepages manually");

If you need 4 huge pages for test, then shouldn't this check you have at least 4?

<snip>

Quoted 33 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +12 -3
index f22e3d345..186eafa51 100644
@@ -144,21 +144,33 @@ int mfd_flags_available(const char *filename, const int
 lineno,
 }
 
 int check_mfd_new(const char *filename, const int lineno,
-			const char *name, loff_t sz, int flags)
+		  const char *name, loff_t sz, int flags)
 {
 	int fd;
 
 	fd = sys_memfd_create(name, flags);
 	if (fd < 0) {
+		if ((flags & MFD_HUGETLB) == MFD_HUGETLB)
+			goto end;
+
 		tst_brk_(filename, lineno, TBROK | TERRNO,
-			"memfd_create(%s, %d) failed", name, flags);
+			 "memfd_create(%s, %d) failed", name, flags);
+	}
+
+	if ((flags & MFD_HUGETLB) == MFD_HUGETLB) {
+		tst_res_(filename, lineno, TINFO,
+			 "memfd_create(%s, %d) succeeded",
+			 name, flags);
+		goto end;
 	}
 
-	tst_res_(filename, lineno, TPASS, "memfd_create(%s, %d) succeeded",

testcases/kernel/syscalls/memfd_create/memfd_create_common.c int mfd_flags_available(const char *filename, const int · line 176

Quoted 10 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +4 -1
-		name, flags);
+	tst_res_(filename, lineno, TPASS,
+		 "memfd_create(%s, %d) succeeded",
+		 name, flags);
 
 	check_ftruncate(filename, lineno, fd, sz);
 
+ end:
 	return fd;
 }

This is ugly way of making it behave one way for your test, and other way for different test.

There are couple options:

1. two functions
mfd_new() - calls the syscall, and only returns a value check_mfd_new() - calls mfd_new(), checks returned value, calls tst_brk()

2. extra parameter, that specifies if failure should lead to tst_brk(), similar to 'strict' parameter in file_lines_scanf().

<snip>

Quoted 19 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +19 -0
+int check_huge_non_writeable(const char *filename, const int lineno, int fd)
+{
+	ssize_t ret;
+	char test_str[] = "data";
+
+	ret = write(fd, test_str, sizeof(test_str));
+
+	if (ret < 0) {
+		if (errno != EINVAL) {
+			tst_res_(filename, lineno, TINFO,
+				 "write(%d, \"%s\", %zu) didn't fail as expected",
+				 fd, test_str, strlen(test_str));
+
+			return -1;
+		}
+	} else {
+		tst_res_(filename, lineno, TINFO,
+			 "write(%d, \"%s\", %zu) succeeded unexpectedly",
+			 fd, test_str, strlen(test_str));

testcases/kernel/syscalls/memfd_create/memfd_create_common.c

Quoted 10 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +10 -0
+
+		return -1;
+	}
+
+	tst_res_(filename, lineno, TINFO,
+		 "write(%d, \"%s\", %zu) failed as expected",
+		 fd, test_str, strlen(test_str));
+
+	return 0;
+}

If this reported PASS/FAIL directly, instead of TINFO, you don't need return code and extra logic in callers.

<snip>

testcases/kernel/syscalls/memfd_create/memfd_create_common.c

Quoted 9 lines
testcases/kernel/syscalls/memfd_create/memfd_create_common.c +9 -0
+
+static void get_from_file(const char *filename, const int lineno,
+			  char *pattern, char *err_msg, long *val)
+{
+	int ret;
+
+	ret = FILE_LINES_SCANF(MEMINFO_PATH, pattern, val);
+	if (ret == 1)
+		tst_brk_(filename, lineno, TBROK, err_msg, NULL);

As I mentioned in v1 review, there's also SAFE_FILE_LINES_SCANF, that does exactly same thing.

--

memfd_create04.c:80: INFO: Attempt to create 1GB huge pages memfd_create04.c:82: INFO: memfd_create(tfile, 2013265924) succeeded memfd_create04.c:94: INFO: mmap((nil), 1073741824, 2, 2, 3, 0) failed memfd_create04.c:96: WARN: Contiguous memory unavailable to map memfd_create04.c:100: INFO: close(3) succeeded memfd_create04.c:102: PASS: Successfully created 1GB pages

This looks like a guaranteed warning on some systems.
What do you expect user do with this warning? Report bug? Change config?

Regards,
Jan

author
#2 lore raw

PATCH v2 1/1 Test for memfd_create() syscall withMFD_HUGETLBflag.

Hi,

Thanks for your code review.

Quoted 8 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +7 -0
+static void test_write_protect(int fd)
+{
+        if (CHECK_HUGE_NON_WRITEABLE(fd) == 0)
+                tst_res(TPASS, "write call failed as expected");
+        else
+                tst_res(TFAIL, "write call passed unexpectedly");
+}

Why not report PASS/FAIL directly? Or why not put the test

right here, without all the indirection? Are you planning on using CHECK_HUGE_NON_WRITEABLE() in other tests?

CHECK_HUGE_NON_WRITEABLE() is dedicated to this test alone. We will move the code logic from CHECK_HUGE_NON_WRITEABLE() to test_write_protect() avoiding indirection.

Quoted 6 lines
testcases/kernel/syscalls/memfd_create/memfd_create03.c new file +5 -0
+        hugepages_enabled = 1;
+        SAFE_CLOSE(fd);
+
+        if (GET_HUGE_TOTAL_PAGES() == 0)
+                tst_brk(TCONF, "Enable Hugepages manually");

I thought you need 4 huge pages for the test. This skips setup

even for 1.

Quoted 1 line

If you need 4 huge pages for test, then shouldn't this check you have at

least 4?

There is no such demand for particular number of hugepages. Tests will work for any number of hugepages greater than 0. 4 is actually a randomly choosen number.

We will modify setup in a way that:-
1. Check the number of free_hugepages.
2. if free_hugepages is 0, increase the total hugepage count by a number (choosen 4).
3. Again, if free_hugepages is 0, tst_brk -> TCONF.

In clean-up, restore the original value of total hugepage count.

memfd_create04.c:80: INFO: Attempt to create 1GB huge pages memfd_create04.c:82: INFO: memfd_create(tfile, 2013265924) succeeded memfd_create04.c:94: INFO: mmap((nil), 1073741824, 2, 2, 3, 0) failed memfd_create04.c:96: WARN: Contiguous memory unavailable to map memfd_create04.c:100: INFO: close(3) succeeded memfd_create04.c:102: PASS: Successfully created 1GB pages

Quoted 1 line

This looks like a guaranteed warning on some systems.

What do you expect user do with this warning? Report bug? Change config?

In memfd_create04:

1. The warning is to due lack of contiguous memory resource to map
hugepage and is nothing related to syscall failure.

Map checking here is an add-on, which is not compulsory. We will remove this checking as users have nothing to do with it.

2. Hence if no map checking, the test will work without set-up & clean-up.

Regards,
Vishnu K