[LTP] [PATCH 1/2] [COMMITTED] syscalls/mmap12: Convert to the new library

[LTP] [PATCH 1/2] [COMMITTED] syscalls/mmap12: Convert to the new library

[Cyril Hrubis]

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 testcases/kernel/syscalls/mmap/mmap12.c | 103 +++++++++++---------------------
 1 file changed, 36 insertions(+), 67 deletions(-)

diff --git a/testcases/kernel/syscalls/mmap/mmap12.c b/testcases/kernel/syscalls/mmap/mmap12.c
index 2b9c8664e..32cbe4431 100644
--- a/testcases/kernel/syscalls/mmap/mmap12.c
+++ b/testcases/kernel/syscalls/mmap/mmap12.c
@@ -40,57 +40,17 @@
 #include <sys/mman.h>
 #include <sys/shm.h>
 
-#include "test.h"
+#include "tst_test.h"
 
 #define TEMPFILE        "mmapfile"
 #define PATHLEN         256
 #define MMAPSIZE        (1UL<<20)
 
-char *TCID = "mmap12";
-int TST_TOTAL = 1;
-
 static int fildes;
 static char *addr;
 
-static int page_check(void);
-static void setup(void);
-static void cleanup(void);
-
-int main(int argc, char *argv[])
-{
-	int lc;
-
-	tst_parse_opts(argc, argv, NULL, NULL);
-
-	setup();
-
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-
-		addr = mmap(NULL, MMAPSIZE, PROT_READ | PROT_WRITE,
-			    MAP_PRIVATE | MAP_POPULATE, fildes, 0);
-
-		if (addr == MAP_FAILED) {
-			tst_resm(TFAIL | TERRNO, "mmap of %s failed", TEMPFILE);
-			continue;
-		}
-
-		if (page_check())
-			tst_resm(TFAIL, "Not all pages are present");
-		else
-			tst_resm(TPASS, "Functionality of mmap() "
-						"successful");
-		if (munmap(addr, MMAPSIZE) != 0)
-			tst_brkm(TFAIL | TERRNO, NULL, "munmap failed");
-	}
-
-	cleanup();
-	tst_exit();
-}
-
 static int page_check(void)
 {
-	int ret;
 	int i = 1;
 	int flag = 0;
 	int pm;
@@ -110,28 +70,26 @@ static int page_check(void)
 	pm = open("/proc/self/pagemap", O_RDONLY);
 	if (pm == -1) {
 		if ((errno == EPERM) && (geteuid() != 0)) {
-			tst_brkm(TCONF | TERRNO, NULL,
+			tst_brk(TCONF | TERRNO,
 				"don't have permission to open dev pagemap");
 		} else {
-			tst_brkm(TFAIL | TERRNO, NULL,
-				"Open dev pagemap failed");
+			tst_brk(TFAIL | TERRNO, "pen dev pagemap failed");
 		}
 	}
 
-	offset = lseek(pm, index, SEEK_SET);
+	offset = SAFE_LSEEK(pm, index, SEEK_SET);
 	if (offset != index)
-		tst_brkm(TFAIL | TERRNO, NULL, "Reposition offset failed");
+		tst_brk(TFAIL | TERRNO, "Reposition offset failed");
 
 	while (i <= num_pages) {
-		ret = read(pm, &pagemap, sizeof(uint64_t));
-		if (ret < 0)
-			tst_brkm(TFAIL | TERRNO, NULL, "Read pagemap failed");
+		SAFE_READ(1, pm, &pagemap, sizeof(uint64_t));
+
 		/*
 		 * Check if the page is present.
 		 */
 		if (!(pagemap & (1ULL<<63))) {
-			tst_resm(TINFO, "The %dth page addressed at %lX is not "
-					"present", i, vmstart + i * page_sz);
+			tst_res(TINFO, "The %dth page addressed@%lX is not "
+				       "present", i, vmstart + i * page_sz);
 			flag = 1;
 		}
 
@@ -146,30 +104,41 @@ static int page_check(void)
 	return 0;
 }
 
-static void setup(void)
+void verify_mmap(void)
 {
-	tst_sig(FORK, DEF_HANDLER, cleanup);
-
-	if ((tst_kvercmp(2, 6, 25)) < 0)
-		tst_brkm(TCONF, NULL,
-			"This test can only run on kernels that are 2.6.25 and "
-			"higher");
+	addr = mmap(NULL, MMAPSIZE, PROT_READ | PROT_WRITE,
+		    MAP_PRIVATE | MAP_POPULATE, fildes, 0);
 
-	TEST_PAUSE;
+	if (addr == MAP_FAILED) {
+		tst_res(TFAIL | TERRNO, "mmap of %s failed", TEMPFILE);
+		return;
+	}
 
-	tst_tmpdir();
+	if (page_check())
+		tst_res(TFAIL, "Not all pages are present");
+	else
+		tst_res(TPASS, "Functionality of mmap() successful");
 
-	fildes = open(TEMPFILE, O_RDWR | O_CREAT, 0766);
-	if (fildes < 0)
-		tst_brkm(TFAIL, cleanup, "opening %s failed", TEMPFILE);
+	SAFE_MUNMAP(addr, MMAPSIZE);
+}
 
-	if (ftruncate(fildes, MMAPSIZE) < 0)
-		tst_brkm(TFAIL | TERRNO, cleanup, "ftruncate file failed");
+static void setup(void)
+{
+	fildes = SAFE_OPEN(TEMPFILE, O_RDWR | O_CREAT, 0766);
 
+	SAFE_FTRUNCATE(fildes, MMAPSIZE);
 }
 
 static void cleanup(void)
 {
-	close(fildes);
-	tst_rmdir();
+	if (fildes > 0)
+		SAFE_CLOSE(fildes);
 }
+
+static struct tst_test test = {
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = verify_mmap,
+	.needs_tmpdir = 1,
+	.min_kver = "2.6.25",
+};
-- 
2.13.0

[LTP] [PATCH 2/2] [RFC] syscalls/mmap12: Do not fail on non-present pages

[Cyril Hrubis]

There are two problems with the testcase:

1. MAP_POPULATE is best effort operation.

   To quote Michal Hocko:

   "The semantic of MAP_POPULATE is rather vague and allows for nuances
   in future."

2. There is no guarantee that the pages will be present even if
   MAP_POPULATE caused read-ahead.

   To quote Michal again:

   "Ptes can be made !present, reclaimed or who knows what else in
   future yet that won't qualify as a regression. I find such a test
   questionable at best."

I still think that calling mmap() with MAP_POPULATE and checking that
the mapping is OK is a valid testcase itself, so instead of passing the
test on present pages we simply check that the mapping is zero-filled
(since we mapped empty file). I kept the page-present check still there
but it now produces only INFO messages, it could be removed though if
everyone agrees that it has no real value.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
CC: Michal Hocko <mhocko@suse.com>
---
 testcases/kernel/syscalls/mmap/mmap12.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/testcases/kernel/syscalls/mmap/mmap12.c b/testcases/kernel/syscalls/mmap/mmap12.c
index 32cbe4431..05ace6a0c 100644
--- a/testcases/kernel/syscalls/mmap/mmap12.c
+++ b/testcases/kernel/syscalls/mmap/mmap12.c
@@ -49,7 +49,7 @@
 static int fildes;
 static char *addr;
 
-static int page_check(void)
+static void page_check(void)
 {
 	int i = 1;
 	int flag = 0;
@@ -70,8 +70,9 @@ static int page_check(void)
 	pm = open("/proc/self/pagemap", O_RDONLY);
 	if (pm == -1) {
 		if ((errno == EPERM) && (geteuid() != 0)) {
-			tst_brk(TCONF | TERRNO,
+			tst_res(TCONF | TERRNO,
 				"don't have permission to open dev pagemap");
+			return;
 		} else {
 			tst_brk(TFAIL | TERRNO, "pen dev pagemap failed");
 		}
@@ -98,14 +99,14 @@ static int page_check(void)
 
 	close(pm);
 
-	if (flag)
-		return 1;
-
-	return 0;
+	if (!flag)
+		tst_res(TINFO, "All pages are present");
 }
 
 void verify_mmap(void)
 {
+	unsigned int i;
+
 	addr = mmap(NULL, MMAPSIZE, PROT_READ | PROT_WRITE,
 		    MAP_PRIVATE | MAP_POPULATE, fildes, 0);
 
@@ -114,11 +115,18 @@ void verify_mmap(void)
 		return;
 	}
 
-	if (page_check())
-		tst_res(TFAIL, "Not all pages are present");
-	else
-		tst_res(TPASS, "Functionality of mmap() successful");
+	page_check();
+
+	for (i = 0; i < MMAPSIZE; i++) {
+		if (addr[i]) {
+			tst_res(TFAIL, "Non-zero byte@offset %i", i);
+			goto unmap;
+		}
+	}
+
+	tst_res(TPASS, "File mapped properly");
 
+unmap:
 	SAFE_MUNMAP(addr, MMAPSIZE);
 }
 
-- 
2.13.0

[LTP] [PATCH 2/2] [RFC] syscalls/mmap12: Do not fail on non-present pages

[Jan Stancek]

----- Original Message -----
> There are two problems with the testcase:
> 
> 1. MAP_POPULATE is best effort operation.
> 
>    To quote Michal Hocko:
> 
>    "The semantic of MAP_POPULATE is rather vague and allows for nuances
>    in future."
> 
> 2. There is no guarantee that the pages will be present even if
>    MAP_POPULATE caused read-ahead.
> 
>    To quote Michal again:
> 
>    "Ptes can be made !present, reclaimed or who knows what else in
>    future yet that won't qualify as a regression. I find such a test
>    questionable at best."

Hi,

I agree with points 1 and 2, the test relies on likely
scenario that pages were present. We are in similar
situation when it comes to readahead test.

@Michal: Can you think of alternative way to differentiate between:
"MAP_POPULATE did its best" vs. "MAP_POPULATE is broken"?

> 
> I still think that calling mmap() with MAP_POPULATE and checking that
> the mapping is OK is a valid testcase itself, so instead of passing the
> test on present pages we simply check that the mapping is zero-filled
> (since we mapped empty file). I kept the page-present check still there
> but it now produces only INFO messages, it could be removed though if
> everyone agrees that it has no real value.

Agreed, we should have at least test that uses the flag - so we can
tell it didn't have negative impact (Oops/non-zero values mapped/etc).

Regards,
Jan

[LTP] [PATCH 2/2] [RFC] syscalls/mmap12: Do not fail on non-present pages

[Michal Hocko]

On Fri 18-08-17 06:00:54, Jan Stancek wrote:
[...]
> @Michal: Can you think of alternative way to differentiate between:
> "MAP_POPULATE did its best" vs. "MAP_POPULATE is broken"?

What about reading /proc/self/smaps for the range and check for RSS
field? Seeing 0 could be reported as an unexpected _warning_ but I am
not really sure there is any correctness test we could do here. Maybe
just compare that MAP_POPULATE will give you the same content as the
manual fault in.

-- 
Michal Hocko
SUSE Labs

[LTP] [PATCH 2/2] [RFC] syscalls/mmap12: Do not fail on non-present pages

[Cyril Hrubis]

Hi!
> > @Michal: Can you think of alternative way to differentiate between:
> > "MAP_POPULATE did its best" vs. "MAP_POPULATE is broken"?
> 
> What about reading /proc/self/smaps for the range and check for RSS
> field? Seeing 0 could be reported as an unexpected _warning_ but I am
> not really sure there is any correctness test we could do here. Maybe
> just compare that MAP_POPULATE will give you the same content as the
> manual fault in.

Jan will you try to implement something along these lines or should we
merge the original patch before the release and look into this later in
a case we decide to?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] [RFC] syscalls/mmap12: Do not fail on non-present pages

[Jan Stancek]

----- Original Message -----
> Hi!
> > > @Michal: Can you think of alternative way to differentiate between:
> > > "MAP_POPULATE did its best" vs. "MAP_POPULATE is broken"?
> > 
> > What about reading /proc/self/smaps for the range and check for RSS
> > field? Seeing 0 could be reported as an unexpected _warning_ but I am
> > not really sure there is any correctness test we could do here. Maybe
> > just compare that MAP_POPULATE will give you the same content as the
> > manual fault in.
> 
> Jan will you try to implement something along these lines or should we
> merge the original patch before the release and look into this later in
> a case we decide to?

Hi,

Let's go with original for now.

I think RSS != 0 is likely to work, but if we can't guarantee that,
I'd prefer simpler test over sporadic/rare false positives.

Regards,
Jan

[LTP] [PATCH 2/2] [RFC] syscalls/mmap12: Do not fail on non-present pages

[Cyril Hrubis]

Hi!
> Let's go with original for now.

Pushed with your ack, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz