Skip to contentSkip to conversation

madvise09: Add MADV_FREE test

PATCH New Cyril Hrubis 2 participants 5 messages Mar 13, 2017 – Mar 21, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 madvise09: Add MADV_FREE test

We test the madvise(MADV_FREE) by running a process in a memory cgroup with fairly small memory limits. The test process forks a child, moves it to the newly created cgroup. The child allocates memory, marks it MADV_FREE then forks a memory hungry child that allocates and faults memory in a loop. Due to undeterministic nature of the OOM (that kills the memory hungry child sooner or later) we have to retry at two levels. First problem is that rarely it gets too rogue and both processes in the cgroup gets killed. The second is that sometimes the memory hungry child is killed too fast (before the kernel has chance to free the pages), so we rerun it a (for a few times) if that happens.

The test expects memory cgroup mounted in the standard /sys/fs/cgroup/ path, which is OK since the functionality tested was added to kernel 4.5 and the test would be skipped on older distros anyway.

Also the test expects that the MADV_FREE pages will not be freed immediatelly hence the test will fail if the whole system is under memory pressure.

The memory limits were choosen to be 8MB and 16MB for memsw limit. I've tried to use to base these on usage_in_bytes but that turned out to be source of random errors. It worked fine and produces stable numbers 99% of the time but then the numbers have changed and caused the test to fail in fork() with ENOMEM.

Signature · 13 lines
Changes in v2:

- Limits bumped to 8MB and 16MB
- Dirty child sleeps between faulting pages and the sleep gradually
  increases with time
- We check if at least one page was freed in the good child
- Setup checks for SwapTotal: in /proc/meminfo and TCONFs if it's <= 0
---
 runtest/syscalls                              |   1 +
 testcases/kernel/syscalls/.gitignore          |   1 +
 testcases/kernel/syscalls/madvise/madvise09.c | 256 ++++++++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 testcases/kernel/syscalls/madvise/madvise09.c
runtest/syscalls +1 -0
index 931a354..a918c5e 100644
@@ -759,6 +759,7 @@ madvise05 madvise05
 madvise06 madvise06
 madvise07 madvise07
 madvise08 madvise08
+madvise09 madvise09
 
 newuname01 newuname01
 
testcases/kernel/syscalls/.gitignore +1 -0
index 9a4727c..5081992 100644
@@ -519,6 +519,7 @@
 /madvise/madvise06
 /madvise/madvise07
 /madvise/madvise08
+/madvise/madvise09
 /mallopt/mallopt01
 /mbind/mbind01
 /memcmp/memcmp01
testcases/kernel/syscalls/madvise/madvise09.c new file +256 -0

256 lines not on this page · raw message

Signature · 1 line
2.10.2
#1 lore raw

PATCH 1/1 madvise09: Add MADV_FREE test

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

Quoted 5 lines

From: "Cyril Hrubis" <[email protected]>
To: [email protected]
Cc: "Jan Stancek" <[email protected]>
Sent: Monday, 13 March, 2017 12:06:36 PM
Subject: [PATCH] madvise09: Add MADV_FREE test

Hi,

It worked fine for me with 4.10 and 64k pages on ppc64le. Couple comments inline:

<snip>

Quoted 4 lines
testcases/kernel/syscalls/madvise/madvise09.c new file +4 -0
+
+static void memory_pressure_child(void)
+{
+	size_t i, page_size = getpagesize();

testcases/kernel/syscalls/madvise/madvise09.c line 82

Quoted 10 lines
testcases/kernel/syscalls/madvise/madvise09.c new file +10 -0
+	char *ptr;
+	int sleep = 1;
+
+	for (;;) {
+		ptr = mmap(NULL, 500 * page_size, PROT_READ | PROT_WRITE,
+			   MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+		for (i = 0; i < 500; i++) {
+			ptr[i * page_size] = i % 100;
+			usleep(sleep);

What I was suggesting was delay between retries. "for" above won't likely ever finish with 64k pages - it will hit OOM sooner, so there won't be any delay in subsequent retries.

How about we make "int sleep" global and increase it before we enter "for" loop?

Quoted 71 lines
testcases/kernel/syscalls/madvise/madvise09.c new file +71 -0
+		}
+
+		sleep++;
+	}
+
+	abort();
+}
+
+static void setup_cgroup_paths(int pid)
+{
+	snprintf(cgroup_path, sizeof(cgroup_path),
+		 MEMCG_PATH "ltp_madvise09_%i/", pid);
+	snprintf(tasks_path, sizeof(tasks_path), "%s/tasks", cgroup_path);
+	snprintf(limit_in_bytes_path, sizeof(limit_in_bytes_path),
+		 "%s/memory.limit_in_bytes", cgroup_path);
+	snprintf(memsw_limit_in_bytes_path, sizeof(memsw_limit_in_bytes_path),
+		 "%s/memory.memsw.limit_in_bytes", cgroup_path);
+}
+
+static int count_freed(char *ptr)
+{
+	int i, ret = 0;
+
+	for (i = 0; i < PAGES; i++) {
+		if (!ptr[i * page_size])
+			ret++;
+	}
+
+	return ret;
+}
+
+static void child(void)
+{
+	size_t i;
+	char *ptr;
+	unsigned int usage, old_limit, old_memsw_limit;
+	int status, pid, retries = 0;
+
+	SAFE_MKDIR(cgroup_path, 0777);
+	SAFE_FILE_PRINTF(tasks_path, "%i", getpid());
+
+	ptr = SAFE_MMAP(NULL, PAGES * page_size, PROT_READ | PROT_WRITE,
+	                 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+
+	for (i = 0; i < PAGES * page_size; i++)
+		ptr[i] = 'a';
+
+	if (madvise(ptr, PAGES * page_size, MADV_FREE)) {
+		if (errno == EINVAL)
+			tst_brk(TCONF | TERRNO, "MADV_FREE is not supported");
+
+		tst_brk(TBROK | TERRNO, "MADV_FREE failed");
+	}
+
+	if (ptr[page_size] != 'a')
+		tst_res(TFAIL, "MADV_FREE pages were freed immediatelly");
+	else
+		tst_res(TPASS, "MADV_FREE pages were not freed immediatelly");
+
+	ptr[0] = 'b';
+	ptr[10 * page_size] = 'b';
+
+	usage = 8 * 1024 * 1024;
+	tst_res(TINFO, "Setting memory limits to %u %u", usage, 2 * usage);
+
+	SAFE_FILE_SCANF(limit_in_bytes_path, "%u", &old_limit);
+	SAFE_FILE_SCANF(memsw_limit_in_bytes_path, "%u", &old_memsw_limit);
+	SAFE_FILE_PRINTF(limit_in_bytes_path, "%u", usage);
+	SAFE_FILE_PRINTF(memsw_limit_in_bytes_path, "%u", 2 * usage);
+
+	do {

testcases/kernel/syscalls/madvise/madvise09.c line 163

Quoted 10 lines
testcases/kernel/syscalls/madvise/madvise09.c new file +10 -0
+		pid = SAFE_FORK();
+		if (!pid)
+			memory_pressure_child();
+
+		tst_res(TINFO, "Memory hungry child %i started, try %i", pid, retries);
+
+		SAFE_WAIT(&status);
+	} while (retries++ < 10 && count_freed(ptr) == 0);
+
+	if (ptr[0] == 0 || ptr[10 * page_size] == 0)

Would it make sense to make this more strict by checking also expected value? Just in case we get some non-zero garbage.

if (ptr[0] == 'b' || ptr[10 * page_size] == 'b')
PASS
else
FAIL

testcases/kernel/syscalls/madvise/madvise09.c line 172

Quoted 9 lines
testcases/kernel/syscalls/madvise/madvise09.c new file +9 -0
+		tst_res(TFAIL, "Page modified after MADV_FREE was freed");
+	else
+		tst_res(TPASS, "Page modified after MADV_FREE was not freed");
+
+	char map[PAGES+1];
+	unsigned int freed = 0;
+
+	for (i = 0; i < PAGES; i++) {
+		if (ptr[i * page_size]) {

Same here, should we check that entire array contains only [ab0]? Otherwise FAIL.

Quoted 12 lines
testcases/kernel/syscalls/madvise/madvise09.c new file +12 -0
+			map[i] = 'p';
+		} else {
+			map[i] = '_';
+			freed++;
+		}
+	}
+	map[PAGES] = '\0';
+
+	tst_res(TINFO, "Memory map: %s", map);
+
+	if (freed)
+		tst_res(TPASS, "Pages MADV_FREE were freed on low memory");

testcases/kernel/syscalls/madvise/madvise09.c line 194

Quoted 10 lines
testcases/kernel/syscalls/madvise/madvise09.c new file +10 -0
+	else
+		tst_res(TFAIL, "No MADV_FREE page was freed on low memory");
+
+	SAFE_FILE_PRINTF(memsw_limit_in_bytes_path, "%u", old_memsw_limit);
+	SAFE_FILE_PRINTF(limit_in_bytes_path, "%u", old_limit);
+
+	SAFE_MUNMAP(ptr, PAGES);
+
+	exit(0);
+}

Otherwise it looks good to me.

Regards,
Jan

#2 lore raw

PATCH 1/1 madvise09: Add MADV_FREE test

Hi!
Ok to commit with these changes against v2?

testcases/kernel/syscalls/madvise/madvise09.c +71 -11
index 0339086..45a14a0 100644
@@ -52,6 +52,7 @@
 #include <signal.h>
 #include <errno.h>
 #include <stdio.h>
+#include <ctype.h>
 
 #include "tst_test.h"
 #include "lapi/mmap.h"
@@ -64,14 +65,16 @@ static char limit_in_bytes_path[PATH_MAX];
 static char memsw_limit_in_bytes_path[PATH_MAX];
 
 static size_t page_size;
+static int sleep_between_faults;
 
 #define PAGES 32
+#define TOUCHED_PAGE1 0
+#define TOUCHED_PAGE2 10
 
 static void memory_pressure_child(void)
 {
 	size_t i, page_size = getpagesize();
 	char *ptr;
-	int sleep = 1;
 
 	for (;;) {
 		ptr = mmap(NULL, 500 * page_size, PROT_READ | PROT_WRITE,
@@ -79,10 +82,8 @@ static void memory_pressure_child(void)
 
 		for (i = 0; i < 500; i++) {
 			ptr[i * page_size] = i % 100;
-			usleep(sleep);
+			usleep(sleep_between_faults);
 		}
-
-		sleep++;
 	}
 
 	abort();
@@ -111,6 +112,46 @@ static int count_freed(char *ptr)
 	return ret;
 }
 
+static int check_page_baaa(char *ptr)
+{
+	unsigned int i;
+
+	if (ptr[0] != 'b') {
+		tst_res(TINFO, "%p unexpected %c (%i) at 0 expected 'b'",
+			ptr, isprint(ptr[0]) ? ptr[0] : ' ', ptr[0]);
+		return 1;
+	}
+
+	for (i = 1; i < page_size; i++) {
+		if (ptr[i] != 'a') {
+			tst_res(TINFO,
+				"%p unexpected %c (%i) at %i expected 'a'",
+				ptr, isprint(ptr[i]) ? ptr[i] : ' ',
+				ptr[i], i);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+static int check_page(char *ptr, char val)
+{
+	unsigned int i;
+
+	for (i = 0; i < page_size; i++) {
+		if (ptr[i] != val) {
+			tst_res(TINFO,
+				"%p unexpected %c (%i)@%i expected %c (%i)",
+				ptr, isprint(ptr[i]) ? ptr[i] : ' ', ptr[i], i,
+				isprint(val) ? val : ' ', val);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 static void child(void)
 {
 	size_t i;
@@ -139,8 +180,8 @@ static void child(void)
 	else
 		tst_res(TPASS, "MADV_FREE pages were not freed immediatelly");
 
-	ptr[0] = 'b';
-	ptr[10 * page_size] = 'b';
+	ptr[TOUCHED_PAGE1 * page_size] = 'b';
+	ptr[TOUCHED_PAGE2 * page_size] = 'b';
 
 	usage = 8 * 1024 * 1024;
 	tst_res(TINFO, "Setting memory limits to %u %u", usage, 2 * usage);
@@ -151,6 +192,8 @@ static void child(void)
 	SAFE_FILE_PRINTF(memsw_limit_in_bytes_path, "%u", 2 * usage);
 
 	do {
+		sleep_between_faults++;
+
 		pid = SAFE_FORK();
 		if (!pid)
 			memory_pressure_child();
@@ -160,21 +203,33 @@ static void child(void)
 		SAFE_WAIT(&status);
 	} while (retries++ < 10 && count_freed(ptr) == 0);
 
-	if (ptr[0] == 0 || ptr[10 * page_size] == 0)
-		tst_res(TFAIL, "Page modified after MADV_FREE was freed");
-	else
-		tst_res(TPASS, "Page modified after MADV_FREE was not freed");
-
 	char map[PAGES+1];
 	unsigned int freed = 0;
+	unsigned int corrupted = 0;
 
 	for (i = 0; i < PAGES; i++) {
+		char exp_val;
+
 		if (ptr[i * page_size]) {
+			exp_val = 'a';
 			map[i] = 'p';
 		} else {
+			exp_val = 0;
 			map[i] = '_';
 			freed++;
 		}
+
+		if (i != TOUCHED_PAGE1 && i != TOUCHED_PAGE2) {
+			if (check_page(ptr + i * page_size, exp_val)) {
+				map[i] = '?';
+				corrupted++;
+			}
+		} else {
+			if (check_page_baaa(ptr + i * page_size)) {
+				map[i] = '?';
+				corrupted++;
+			}
+		}
 	}
 	map[PAGES] = '\0';
 
@@ -185,6 +240,11 @@ static void child(void)
 	else
 		tst_res(TFAIL, "No MADV_FREE page was freed on low memory");
 
+	if (corrupted)
+		tst_res(TFAIL, "Found corrupted page");
+	else
+		tst_res(TPASS, "All pages have expected content");
+
 	SAFE_FILE_PRINTF(memsw_limit_in_bytes_path, "%u", old_memsw_limit);
 	SAFE_FILE_PRINTF(limit_in_bytes_path, "%u", old_limit);
 
Signature · 2 lines
Cyril Hrubis
[email protected]
#3 lore raw

PATCH 1/1 madvise09: Add MADV_FREE test

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

Quoted 8 lines

From: "Cyril Hrubis" <[email protected]>
To: "Jan Stancek" <[email protected]>
Cc: [email protected]
Sent: Tuesday, 21 March, 2017 2:03:13 PM
Subject: Re: [PATCH] madvise09: Add MADV_FREE test

Hi!
Ok to commit with these changes against v2?

Yes, thanks for adding those corruption checks.

Regards,
Jan

#4 lore raw

PATCH 1/1 madvise09: Add MADV_FREE test

Hi!

Quoted 3 lines
Quoted 1 line

Ok to commit with these changes against v2?

Yes, thanks for adding those corruption checks.

Pushed, thanks for the review :-)

Signature · 2 lines
Cyril Hrubis
[email protected]