[PATCH v2 0/8] selftests/mm fixes for arm64

[PATCH v2 0/8] selftests/mm fixes for arm64

[Ryan Roberts]

Hi All,

This is v2 of my series to clean up mm selftests so that they run correctly on
arm64. See [1] for full explanation.


Changes Since v1 [1]
--------------------

  - Patch 1: Explicitly set line buffer mode in ksft_print_header()
  - Dropped v1 patch 2 (set execute permissions): Andrew has taken this into his
    branch separately.
  - Patch 2: Don't compile `soft-dirty` suite for arm64 instead of skipping it
    at runtime.
  - Patch 2: Declare fewer tests and skip all of test_softdirty() if soft-dirty
    is not supported, rather than conditionally marking each check as skipped.
  - Added Reviewed-by tags: thanks DavidH!
  - Patch 8: Clarified commit message.


[1] https://lore.kernel.org/linux-mm/20230713135440.3651409-1-ryan.roberts@arm.com/

Thanks,
Ryan


Ryan Roberts (8):
  selftests: Line buffer test program's stdout
  selftests/mm: Skip soft-dirty tests on arm64
  selftests/mm: Enable mrelease_test for arm64
  selftests/mm: Fix thuge-gen test bugs
  selftests/mm: va_high_addr_switch should skip unsupported arm64
    configs
  selftests/mm: Make migration test robust to failure
  selftests/mm: Optionally pass duration to transhuge-stress
  selftests/mm: Run all tests from run_vmtests.sh

 tools/testing/selftests/kselftest.h           |  9 ++
 tools/testing/selftests/kselftest/runner.sh   |  7 +-
 tools/testing/selftests/mm/Makefile           | 82 ++++++++++---------
 tools/testing/selftests/mm/madv_populate.c    | 26 +++++-
 tools/testing/selftests/mm/migration.c        | 14 +++-
 tools/testing/selftests/mm/mrelease_test.c    |  1 +
 tools/testing/selftests/mm/run_vmtests.sh     | 28 ++++++-
 tools/testing/selftests/mm/settings           |  2 +-
 tools/testing/selftests/mm/thuge-gen.c        |  4 +-
 tools/testing/selftests/mm/transhuge-stress.c | 12 ++-
 .../selftests/mm/va_high_addr_switch.c        |  2 +-
 11 files changed, 133 insertions(+), 54 deletions(-)

--
2.25.1

[PATCH v2 1/8] selftests: Line buffer test program's stdout

[Ryan Roberts]

The selftests runner pipes the test program's stdout to tap_prefix. The
presence of the pipe means that the test program sets its stdout to be
fully buffered (as aposed to line buffered when directly connected to
the terminal). The block buffering means that there is often content in
the buffer at fork() time, which causes the output to end up duplicated.
This was causing problems for mm:cow where test results were duplicated
20-30x.

Solve this by using `stdbuf`, when available to force the test program
to use line buffered mode. This means previously printf'ed results are
flushed out of the program before any fork().

Additionally, explicitly set line buffer mode in ksft_print_header(),
which means that all test programs that use the ksft framework will
benefit even if stdbuf is not present on the system.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/kselftest.h         | 9 +++++++++
 tools/testing/selftests/kselftest/runner.sh | 7 +++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 829be379545a..529d29a35900 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -113,6 +113,15 @@ static inline int ksft_get_error_cnt(void) { return ksft_cnt.ksft_error; }
 
 static inline void ksft_print_header(void)
 {
+	/*
+	 * Force line buffering; If stdout is not connected to a terminal, it
+	 * will otherwise default to fully buffered, which can cause output
+	 * duplication if there is content in the buffer when fork()ing. If
+	 * there is a crash, line buffering also means the most recent output
+	 * line will be visible.
+	 */
+	setvbuf(stdout, NULL, _IOLBF, 0);
+
 	if (!(getenv("KSFT_TAP_LEVEL")))
 		printf("TAP version 13\n");
 }
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index 1c952d1401d4..261c73cab41b 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -105,15 +105,18 @@ run_one()
 		echo "# Warning: file $TEST is missing!"
 		echo "not ok $test_num $TEST_HDR_MSG"
 	else
+		if [ -x /usr/bin/stdbuf ]; then
+			stdbuf="/usr/bin/stdbuf --output=L "
+		fi
 		eval kselftest_cmd_args="\$${kselftest_cmd_args_ref:-}"
-		cmd="./$BASENAME_TEST $kselftest_cmd_args"
+		cmd="$stdbuf ./$BASENAME_TEST $kselftest_cmd_args"
 		if [ ! -x "$TEST" ]; then
 			echo "# Warning: file $TEST is not executable"
 
 			if [ $(head -n 1 "$TEST" | cut -c -2) = "#!" ]
 			then
 				interpreter=$(head -n 1 "$TEST" | cut -c 3-)
-				cmd="$interpreter ./$BASENAME_TEST"
+				cmd="$stdbuf $interpreter ./$BASENAME_TEST"
 			else
 				echo "not ok $test_num $TEST_HDR_MSG"
 				return
-- 
2.25.1

Re: [PATCH v2 1/8] selftests: Line buffer test program's stdout

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 401 bytes --]

On Mon, Jul 17, 2023 at 11:31:45AM +0100, Ryan Roberts wrote:
> The selftests runner pipes the test program's stdout to tap_prefix. The
> presence of the pipe means that the test program sets its stdout to be
> fully buffered (as aposed to line buffered when directly connected to
> the terminal). The block buffering means that there is often content in

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/8] selftests/mm: Skip soft-dirty tests on arm64

[Ryan Roberts]

arm64 does not support the soft-dirty PTE bit. However, the `soft-dirty`
test suite is currently run unconditionally and therefore generates
spurious test failures on arm64. There are also some tests in
`madv_populate` which assume it is supported.

For `soft-dirty` lets disable the whole suite for arm64; it is no longer
built and run_vmtests.sh will skip it if its not present.

For `madv_populate`, we need a runtime mechanism so that the remaining
tests continue to be run. Unfortunately, the only way to determine if
the soft-dirty dirty bit is supported is to write to a page, then see if
the bit is set in /proc/self/pagemap. But the tests that we want to
conditionally execute are testing precicesly this. So if we introduced
this feature check, we could accedentally turn a real failure (on a
system that claims to support soft-dirty) into a skip. So instead, do
the check based on architecture; for arm64, we report that soft-dirty is
not supported.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/mm/Makefile        |  5 ++++-
 tools/testing/selftests/mm/madv_populate.c | 26 ++++++++++++++++++++--
 tools/testing/selftests/mm/run_vmtests.sh  |  5 ++++-
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 66d7c07dc177..3514697fc2db 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -63,12 +63,15 @@ TEST_GEN_PROGS += thuge-gen
 TEST_GEN_PROGS += transhuge-stress
 TEST_GEN_PROGS += uffd-stress
 TEST_GEN_PROGS += uffd-unit-tests
-TEST_GEN_PROGS += soft-dirty
 TEST_GEN_PROGS += split_huge_page_test
 TEST_GEN_PROGS += ksm_tests
 TEST_GEN_PROGS += ksm_functional_tests
 TEST_GEN_PROGS += mdwe_test
 
+ifneq ($(ARCH),arm64)
+TEST_GEN_PROGS += soft-dirty
+endif
+
 ifeq ($(ARCH),x86_64)
 CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32)
 CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_64bit_program.c)
diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c
index 60547245e479..17bcb07f19f3 100644
--- a/tools/testing/selftests/mm/madv_populate.c
+++ b/tools/testing/selftests/mm/madv_populate.c
@@ -264,14 +264,35 @@ static void test_softdirty(void)
 	munmap(addr, SIZE);
 }
 
+static int system_has_softdirty(void)
+{
+	/*
+	 * There is no way to check if the kernel supports soft-dirty, other
+	 * than by writing to a page and seeing if the bit was set. But the
+	 * tests are intended to check that the bit gets set when it should, so
+	 * doing that check would turn a potentially legitimate fail into a
+	 * skip. Fortunately, we know for sure that arm64 does not support
+	 * soft-dirty. So for now, let's just use the arch as a corse guide.
+	 */
+#if defined(__aarch64__)
+	return 0;
+#else
+	return 1;
+#endif
+}
+
 int main(int argc, char **argv)
 {
+	int nr_tests = 16;
 	int err;
 
 	pagesize = getpagesize();
 
+	if (system_has_softdirty())
+		nr_tests += 5;
+
 	ksft_print_header();
-	ksft_set_plan(21);
+	ksft_set_plan(nr_tests);
 
 	sense_support();
 	test_prot_read();
@@ -279,7 +300,8 @@ int main(int argc, char **argv)
 	test_holes();
 	test_populate_read();
 	test_populate_write();
-	test_softdirty();
+	if (system_has_softdirty())
+		test_softdirty();
 
 	err = ksft_get_fail_cnt();
 	if (err)
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
index 3f26f6e15b2a..9e4338aa5e09 100755
--- a/tools/testing/selftests/mm/run_vmtests.sh
+++ b/tools/testing/selftests/mm/run_vmtests.sh
@@ -290,7 +290,10 @@ then
 	CATEGORY="pkey" run_test ./protection_keys_64
 fi
 
-CATEGORY="soft_dirty" run_test ./soft-dirty
+if [ -x ./soft-dirty ]
+then
+	CATEGORY="soft_dirty" run_test ./soft-dirty
+fi
 
 # COW tests
 CATEGORY="cow" run_test ./cow
-- 
2.25.1

Re: [PATCH v2 2/8] selftests/mm: Skip soft-dirty tests on arm64

[David Hildenbrand]

On 17.07.23 12:31, Ryan Roberts wrote:
> arm64 does not support the soft-dirty PTE bit. However, the `soft-dirty`
> test suite is currently run unconditionally and therefore generates
> spurious test failures on arm64. There are also some tests in
> `madv_populate` which assume it is supported.
> 
> For `soft-dirty` lets disable the whole suite for arm64; it is no longer
> built and run_vmtests.sh will skip it if its not present.
> 
> For `madv_populate`, we need a runtime mechanism so that the remaining
> tests continue to be run. Unfortunately, the only way to determine if
> the soft-dirty dirty bit is supported is to write to a page, then see if
> the bit is set in /proc/self/pagemap. But the tests that we want to
> conditionally execute are testing precicesly this. So if we introduced
> this feature check, we could accedentally turn a real failure (on a
> system that claims to support soft-dirty) into a skip. So instead, do
> the check based on architecture; for arm64, we report that soft-dirty is
> not supported.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   tools/testing/selftests/mm/Makefile        |  5 ++++-
>   tools/testing/selftests/mm/madv_populate.c | 26 ++++++++++++++++++++--
>   tools/testing/selftests/mm/run_vmtests.sh  |  5 ++++-
>   3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index 66d7c07dc177..3514697fc2db 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -63,12 +63,15 @@ TEST_GEN_PROGS += thuge-gen
>   TEST_GEN_PROGS += transhuge-stress
>   TEST_GEN_PROGS += uffd-stress
>   TEST_GEN_PROGS += uffd-unit-tests
> -TEST_GEN_PROGS += soft-dirty
>   TEST_GEN_PROGS += split_huge_page_test
>   TEST_GEN_PROGS += ksm_tests
>   TEST_GEN_PROGS += ksm_functional_tests
>   TEST_GEN_PROGS += mdwe_test
>   
> +ifneq ($(ARCH),arm64)
> +TEST_GEN_PROGS += soft-dirty
> +endif
> +
>   ifeq ($(ARCH),x86_64)
>   CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32)
>   CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_64bit_program.c)
> diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c
> index 60547245e479..17bcb07f19f3 100644
> --- a/tools/testing/selftests/mm/madv_populate.c
> +++ b/tools/testing/selftests/mm/madv_populate.c
> @@ -264,14 +264,35 @@ static void test_softdirty(void)
>   	munmap(addr, SIZE);
>   }
>   
> +static int system_has_softdirty(void)
> +{
> +	/*
> +	 * There is no way to check if the kernel supports soft-dirty, other
> +	 * than by writing to a page and seeing if the bit was set. But the
> +	 * tests are intended to check that the bit gets set when it should, so
> +	 * doing that check would turn a potentially legitimate fail into a
> +	 * skip. Fortunately, we know for sure that arm64 does not support
> +	 * soft-dirty. So for now, let's just use the arch as a corse guide.
> +	 */
> +#if defined(__aarch64__)
> +	return 0;
> +#else
> +	return 1;
> +#endif
> +}

I guess that will also make the compiler remove any traces of 
test_softdirty()( from the binary.

Acked-by: David Hildenbrand <david@redhat.com>


-- 
Cheers,

David / dhildenb

[PATCH v2 3/8] selftests/mm: Enable mrelease_test for arm64

[Ryan Roberts]

mrelease_test defaults to defining __NR_pidfd_open and
__NR_process_mrelease syscall numbers to -1, if they are not defined
anywhere else, and the suite would then be marked as skipped as a
result.

arm64 (at least the stock debian toolchain that I'm using) requires
including <sys/syscall.h> to pull in the defines for these syscalls. So
let's add this header. With this in place, the test is passing on arm64.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 tools/testing/selftests/mm/mrelease_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/mm/mrelease_test.c b/tools/testing/selftests/mm/mrelease_test.c
index dca21042b679..d822004a374e 100644
--- a/tools/testing/selftests/mm/mrelease_test.c
+++ b/tools/testing/selftests/mm/mrelease_test.c
@@ -7,6 +7,7 @@
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <sys/syscall.h>
 #include <sys/wait.h>
 #include <unistd.h>
 #include <asm-generic/unistd.h>
-- 
2.25.1

[PATCH v2 4/8] selftests/mm: Fix thuge-gen test bugs

[Ryan Roberts]

thuge-gen was previously only munmapping part of the mmapped buffer,
which caused us to run out of 1G huge pages for a later part of the
test. Fix this by munmapping the whole buffer. Based on the code, it
looks like a typo rather than an intention to keep some of the buffer
mapped.

thuge-gen was also calling mmap with SHM_HUGETLB flag (bit 11 set),
which is actually MAP_DENYWRITE in mmap context. The man page says this
flag is ignored in modern kernels. I'm pretty sure from the context that
the author intended to pass the MAP_HUGETLB flag so I've fixed that up
too.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/mm/thuge-gen.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mm/thuge-gen.c b/tools/testing/selftests/mm/thuge-gen.c
index 380ab5f0a534..16ed4dfa7359 100644
--- a/tools/testing/selftests/mm/thuge-gen.c
+++ b/tools/testing/selftests/mm/thuge-gen.c
@@ -139,7 +139,7 @@ void test_mmap(unsigned long size, unsigned flags)
 		before, after, before - after, size);
 	assert(size == getpagesize() || (before - after) == NUM_PAGES);
 	show(size);
-	err = munmap(map, size);
+	err = munmap(map, size * NUM_PAGES);
 	assert(!err);
 }
 
@@ -222,7 +222,7 @@ int main(void)
 		test_mmap(ps, MAP_HUGETLB | arg);
 	}
 	printf("Testing default huge mmap\n");
-	test_mmap(default_hps, SHM_HUGETLB);
+	test_mmap(default_hps, MAP_HUGETLB);
 
 	puts("Testing non-huge shmget");
 	test_shmget(getpagesize(), 0);
-- 
2.25.1

Re: [PATCH v2 4/8] selftests/mm: Fix thuge-gen test bugs

[David Hildenbrand]

On 17.07.23 12:31, Ryan Roberts wrote:
> thuge-gen was previously only munmapping part of the mmapped buffer,
> which caused us to run out of 1G huge pages for a later part of the
> test. Fix this by munmapping the whole buffer. Based on the code, it
> looks like a typo rather than an intention to keep some of the buffer
> mapped.
> 
> thuge-gen was also calling mmap with SHM_HUGETLB flag (bit 11 set),
> which is actually MAP_DENYWRITE in mmap context. The man page says this
> flag is ignored in modern kernels. I'm pretty sure from the context that
> the author intended to pass the MAP_HUGETLB flag so I've fixed that up
> too.

Makes sense to me.

> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   tools/testing/selftests/mm/thuge-gen.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/thuge-gen.c b/tools/testing/selftests/mm/thuge-gen.c
> index 380ab5f0a534..16ed4dfa7359 100644
> --- a/tools/testing/selftests/mm/thuge-gen.c
> +++ b/tools/testing/selftests/mm/thuge-gen.c
> @@ -139,7 +139,7 @@ void test_mmap(unsigned long size, unsigned flags)
>   		before, after, before - after, size);
>   	assert(size == getpagesize() || (before - after) == NUM_PAGES);
>   	show(size);
> -	err = munmap(map, size);
> +	err = munmap(map, size * NUM_PAGES);
>   	assert(!err);
>   }
>   
> @@ -222,7 +222,7 @@ int main(void)
>   		test_mmap(ps, MAP_HUGETLB | arg);
>   	}
>   	printf("Testing default huge mmap\n");
> -	test_mmap(default_hps, SHM_HUGETLB);
> +	test_mmap(default_hps, MAP_HUGETLB);
>   
>   	puts("Testing non-huge shmget");
>   	test_shmget(getpagesize(), 0);

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

[PATCH v2 5/8] selftests/mm: va_high_addr_switch should skip unsupported arm64 configs

[Ryan Roberts]

va_high_addr_switch has a mechanism to determine if the tests should be
run or skipped (supported_arch()). This currently returns
unconditionally true for arm64. However, va_high_addr_switch also
requires a large virtual address space for the tests to run, otherwise
they spuriously fail.

Since arm64 can only support VA > 48 bits when the page size is 64K,
let's decide whether we should skip the test suite based on the page
size. This reduces noise when running on 4K and 16K kernels.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 tools/testing/selftests/mm/va_high_addr_switch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/va_high_addr_switch.c b/tools/testing/selftests/mm/va_high_addr_switch.c
index 7cfaf4a74c57..cfbc501290d3 100644
--- a/tools/testing/selftests/mm/va_high_addr_switch.c
+++ b/tools/testing/selftests/mm/va_high_addr_switch.c
@@ -292,7 +292,7 @@ static int supported_arch(void)
 #elif defined(__x86_64__)
 	return 1;
 #elif defined(__aarch64__)
-	return 1;
+	return getpagesize() == PAGE_SIZE;
 #else
 	return 0;
 #endif
-- 
2.25.1

[PATCH v2 6/8] selftests/mm: Make migration test robust to failure

[Ryan Roberts]

The `migration` test currently has a number of robustness problems that
cause it to hang and leak resources.

Timeout: There are 3 tests, which each previously ran for 60 seconds.
However, the timeout in mm/settings for a single test binary was set to
45 seconds. So when run using run_kselftest.sh, the top level timeout
would trigger before the test binary was finished. Solve this by meeting
in the middle; each of the 3 tests now runs for 20 seconds (for a total
of 60), and the top level timeout is set to 90 seconds.

Leaking child processes: the `shared_anon` test fork()s some children
but then an ASSERT() fires before the test kills those children. The
assert causes immediate exit of the parent and leaking of the children.
Furthermore, if run using the run_kselftest.sh wrapper, the wrapper
would get stuck waiting for those children to exit, which never happens.
Solve this by deferring any asserts until after the children are killed.
The same pattern is used for the threaded tests for uniformity.

With these changes, the test binary now runs to completion on arm64,
with 2 tests passing and the `shared_anon` test failing.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/mm/migration.c | 14 ++++++++++----
 tools/testing/selftests/mm/settings    |  2 +-
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c
index 379581567f27..189d7d9070e8 100644
--- a/tools/testing/selftests/mm/migration.c
+++ b/tools/testing/selftests/mm/migration.c
@@ -15,7 +15,7 @@
 #include <time.h>
 
 #define TWOMEG (2<<20)
-#define RUNTIME (60)
+#define RUNTIME (20)
 
 #define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1)))
 
@@ -118,6 +118,7 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
 {
 	uint64_t *ptr;
 	int i;
+	int ret;
 
 	if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
 		SKIP(return, "Not enough threads or NUMA nodes available");
@@ -131,9 +132,10 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
 		if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
 			perror("Couldn't create thread");
 
-	ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
+	ret = migrate(ptr, self->n1, self->n2);
 	for (i = 0; i < self->nthreads - 1; i++)
 		ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
+	ASSERT_EQ(ret, 0);
 }
 
 /*
@@ -144,6 +146,7 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
 	pid_t pid;
 	uint64_t *ptr;
 	int i;
+	int ret;
 
 	if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
 		SKIP(return, "Not enough threads or NUMA nodes available");
@@ -161,9 +164,10 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
 			self->pids[i] = pid;
 	}
 
-	ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
+	ret = migrate(ptr, self->n1, self->n2);
 	for (i = 0; i < self->nthreads - 1; i++)
 		ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
+	ASSERT_EQ(ret, 0);
 }
 
 /*
@@ -173,6 +177,7 @@ TEST_F_TIMEOUT(migration, private_anon_thp, 2*RUNTIME)
 {
 	uint64_t *ptr;
 	int i;
+	int ret;
 
 	if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
 		SKIP(return, "Not enough threads or NUMA nodes available");
@@ -188,9 +193,10 @@ TEST_F_TIMEOUT(migration, private_anon_thp, 2*RUNTIME)
 		if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
 			perror("Couldn't create thread");
 
-	ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
+	ret = migrate(ptr, self->n1, self->n2);
 	for (i = 0; i < self->nthreads - 1; i++)
 		ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
+	ASSERT_EQ(ret, 0);
 }
 
 TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/mm/settings b/tools/testing/selftests/mm/settings
index 9abfc60e9e6f..ba4d85f74cd6 100644
--- a/tools/testing/selftests/mm/settings
+++ b/tools/testing/selftests/mm/settings
@@ -1 +1 @@
-timeout=45
+timeout=90
-- 
2.25.1

Re: [PATCH v2 6/8] selftests/mm: Make migration test robust to failure

[David Hildenbrand]

On 17.07.23 12:31, Ryan Roberts wrote:
> The `migration` test currently has a number of robustness problems that
> cause it to hang and leak resources.
> 
> Timeout: There are 3 tests, which each previously ran for 60 seconds.
> However, the timeout in mm/settings for a single test binary was set to
> 45 seconds. So when run using run_kselftest.sh, the top level timeout
> would trigger before the test binary was finished. Solve this by meeting
> in the middle; each of the 3 tests now runs for 20 seconds (for a total
> of 60), and the top level timeout is set to 90 seconds.
> 
> Leaking child processes: the `shared_anon` test fork()s some children
> but then an ASSERT() fires before the test kills those children. The
> assert causes immediate exit of the parent and leaking of the children.
> Furthermore, if run using the run_kselftest.sh wrapper, the wrapper
> would get stuck waiting for those children to exit, which never happens.
> Solve this by deferring any asserts until after the children are killed.
> The same pattern is used for the threaded tests for uniformity.
> 
> With these changes, the test binary now runs to completion on arm64,
> with 2 tests passing and the `shared_anon` test failing.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   tools/testing/selftests/mm/migration.c | 14 ++++++++++----
>   tools/testing/selftests/mm/settings    |  2 +-
>   2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/migration.c b/tools/testing/selftests/mm/migration.c
> index 379581567f27..189d7d9070e8 100644
> --- a/tools/testing/selftests/mm/migration.c
> +++ b/tools/testing/selftests/mm/migration.c
> @@ -15,7 +15,7 @@
>   #include <time.h>
>   
>   #define TWOMEG (2<<20)
> -#define RUNTIME (60)
> +#define RUNTIME (20)
>   
>   #define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1)))
>   
> @@ -118,6 +118,7 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>   {
>   	uint64_t *ptr;
>   	int i;
> +	int ret;
>   
>   	if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>   		SKIP(return, "Not enough threads or NUMA nodes available");
> @@ -131,9 +132,10 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>   		if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
>   			perror("Couldn't create thread");
>   
> -	ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
> +	ret = migrate(ptr, self->n1, self->n2);
>   	for (i = 0; i < self->nthreads - 1; i++)
>   		ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
> +	ASSERT_EQ(ret, 0);

Why is that required? This does not involve fork.

>   }
>   
>   /*
> @@ -144,6 +146,7 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>   	pid_t pid;
>   	uint64_t *ptr;
>   	int i;
> +	int ret;
>   
>   	if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>   		SKIP(return, "Not enough threads or NUMA nodes available");
> @@ -161,9 +164,10 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>   			self->pids[i] = pid;
>   	}
>   
> -	ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
> +	ret = migrate(ptr, self->n1, self->n2);
>   	for (i = 0; i < self->nthreads - 1; i++)
>   		ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
> +	ASSERT_EQ(ret, 0);


Might be cleaner to also:

diff --git a/tools/testing/selftests/mm/migration.c 
b/tools/testing/selftests/mm/migration.c
index 379581567f27..b3f12b9847ec 100644
--- a/tools/testing/selftests/mm/migration.c
+++ b/tools/testing/selftests/mm/migration.c
@@ -11,6 +11,7 @@
  #include <numaif.h>
  #include <sys/mman.h>
  #include <sys/types.h>
+#include <sys/prctl.h>
  #include <signal.h>
  #include <time.h>

@@ -155,10 +156,12 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
         memset(ptr, 0xde, TWOMEG);
         for (i = 0; i < self->nthreads - 1; i++) {
                 pid = fork();
-               if (!pid)
+               if (!pid) {
+                       prctl(PR_SET_PDEATHSIG, SIGHUP);
                         access_mem(ptr);
-               else
+               } else {
                         self->pids[i] = pid;
+               }
         }

         ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);


Then, whenever the parent dies, all child processes get zapped.


-- 
Cheers,

David / dhildenb

Re: [PATCH v2 6/8] selftests/mm: Make migration test robust to failure

[Ryan Roberts]

On 17/07/2023 18:40, David Hildenbrand wrote:
> On 17.07.23 12:31, Ryan Roberts wrote:
>> The `migration` test currently has a number of robustness problems that
>> cause it to hang and leak resources.
>>
>> Timeout: There are 3 tests, which each previously ran for 60 seconds.
>> However, the timeout in mm/settings for a single test binary was set to
>> 45 seconds. So when run using run_kselftest.sh, the top level timeout
>> would trigger before the test binary was finished. Solve this by meeting
>> in the middle; each of the 3 tests now runs for 20 seconds (for a total
>> of 60), and the top level timeout is set to 90 seconds.
>>
>> Leaking child processes: the `shared_anon` test fork()s some children
>> but then an ASSERT() fires before the test kills those children. The
>> assert causes immediate exit of the parent and leaking of the children.
>> Furthermore, if run using the run_kselftest.sh wrapper, the wrapper
>> would get stuck waiting for those children to exit, which never happens.
>> Solve this by deferring any asserts until after the children are killed.
>> The same pattern is used for the threaded tests for uniformity.
>>
>> With these changes, the test binary now runs to completion on arm64,
>> with 2 tests passing and the `shared_anon` test failing.
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
>>   tools/testing/selftests/mm/migration.c | 14 ++++++++++----
>>   tools/testing/selftests/mm/settings    |  2 +-
>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/migration.c
>> b/tools/testing/selftests/mm/migration.c
>> index 379581567f27..189d7d9070e8 100644
>> --- a/tools/testing/selftests/mm/migration.c
>> +++ b/tools/testing/selftests/mm/migration.c
>> @@ -15,7 +15,7 @@
>>   #include <time.h>
>>     #define TWOMEG (2<<20)
>> -#define RUNTIME (60)
>> +#define RUNTIME (20)
>>     #define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1)))
>>   @@ -118,6 +118,7 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>   {
>>       uint64_t *ptr;
>>       int i;
>> +    int ret;
>>         if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>           SKIP(return, "Not enough threads or NUMA nodes available");
>> @@ -131,9 +132,10 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>           if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
>>               perror("Couldn't create thread");
>>   -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>> +    ret = migrate(ptr, self->n1, self->n2);
>>       for (i = 0; i < self->nthreads - 1; i++)
>>           ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
>> +    ASSERT_EQ(ret, 0);
> 
> Why is that required? This does not involve fork.

It's not required. I was just trying to keep everything aligned to the same pattern.

> 
>>   }
>>     /*
>> @@ -144,6 +146,7 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>       pid_t pid;
>>       uint64_t *ptr;
>>       int i;
>> +    int ret;
>>         if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>           SKIP(return, "Not enough threads or NUMA nodes available");
>> @@ -161,9 +164,10 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>               self->pids[i] = pid;
>>       }
>>   -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>> +    ret = migrate(ptr, self->n1, self->n2);
>>       for (i = 0; i < self->nthreads - 1; i++)
>>           ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
>> +    ASSERT_EQ(ret, 0);
> 
> 
> Might be cleaner to also:

Or instead of? I agree this is neater, so will undo the moving of the ASSERT()
and rely on this prctl.

> 
> diff --git a/tools/testing/selftests/mm/migration.c
> b/tools/testing/selftests/mm/migration.c
> index 379581567f27..b3f12b9847ec 100644
> --- a/tools/testing/selftests/mm/migration.c
> +++ b/tools/testing/selftests/mm/migration.c
> @@ -11,6 +11,7 @@
>  #include <numaif.h>
>  #include <sys/mman.h>
>  #include <sys/types.h>
> +#include <sys/prctl.h>
>  #include <signal.h>
>  #include <time.h>
> 
> @@ -155,10 +156,12 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>         memset(ptr, 0xde, TWOMEG);
>         for (i = 0; i < self->nthreads - 1; i++) {
>                 pid = fork();
> -               if (!pid)
> +               if (!pid) {
> +                       prctl(PR_SET_PDEATHSIG, SIGHUP);
>                         access_mem(ptr);
> -               else
> +               } else {
>                         self->pids[i] = pid;
> +               }
>         }
> 
>         ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
> 
> 
> Then, whenever the parent dies, all child processes get zapped.
> 
> 

Re: [PATCH v2 6/8] selftests/mm: Make migration test robust to failure

[David Hildenbrand]

On 18.07.23 12:49, Ryan Roberts wrote:
> On 17/07/2023 18:40, David Hildenbrand wrote:
>> On 17.07.23 12:31, Ryan Roberts wrote:
>>> The `migration` test currently has a number of robustness problems that
>>> cause it to hang and leak resources.
>>>
>>> Timeout: There are 3 tests, which each previously ran for 60 seconds.
>>> However, the timeout in mm/settings for a single test binary was set to
>>> 45 seconds. So when run using run_kselftest.sh, the top level timeout
>>> would trigger before the test binary was finished. Solve this by meeting
>>> in the middle; each of the 3 tests now runs for 20 seconds (for a total
>>> of 60), and the top level timeout is set to 90 seconds.
>>>
>>> Leaking child processes: the `shared_anon` test fork()s some children
>>> but then an ASSERT() fires before the test kills those children. The
>>> assert causes immediate exit of the parent and leaking of the children.
>>> Furthermore, if run using the run_kselftest.sh wrapper, the wrapper
>>> would get stuck waiting for those children to exit, which never happens.
>>> Solve this by deferring any asserts until after the children are killed.
>>> The same pattern is used for the threaded tests for uniformity.
>>>
>>> With these changes, the test binary now runs to completion on arm64,
>>> with 2 tests passing and the `shared_anon` test failing.
>>>
>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>>    tools/testing/selftests/mm/migration.c | 14 ++++++++++----
>>>    tools/testing/selftests/mm/settings    |  2 +-
>>>    2 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/mm/migration.c
>>> b/tools/testing/selftests/mm/migration.c
>>> index 379581567f27..189d7d9070e8 100644
>>> --- a/tools/testing/selftests/mm/migration.c
>>> +++ b/tools/testing/selftests/mm/migration.c
>>> @@ -15,7 +15,7 @@
>>>    #include <time.h>
>>>      #define TWOMEG (2<<20)
>>> -#define RUNTIME (60)
>>> +#define RUNTIME (20)
>>>      #define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1)))
>>>    @@ -118,6 +118,7 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>>    {
>>>        uint64_t *ptr;
>>>        int i;
>>> +    int ret;
>>>          if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>>            SKIP(return, "Not enough threads or NUMA nodes available");
>>> @@ -131,9 +132,10 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>>            if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
>>>                perror("Couldn't create thread");
>>>    -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>>> +    ret = migrate(ptr, self->n1, self->n2);
>>>        for (i = 0; i < self->nthreads - 1; i++)
>>>            ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
>>> +    ASSERT_EQ(ret, 0);
>>
>> Why is that required? This does not involve fork.
> 
> It's not required. I was just trying to keep everything aligned to the same pattern.
> 
>>
>>>    }
>>>      /*
>>> @@ -144,6 +146,7 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>>        pid_t pid;
>>>        uint64_t *ptr;
>>>        int i;
>>> +    int ret;
>>>          if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>>            SKIP(return, "Not enough threads or NUMA nodes available");
>>> @@ -161,9 +164,10 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>>                self->pids[i] = pid;
>>>        }
>>>    -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>>> +    ret = migrate(ptr, self->n1, self->n2);
>>>        for (i = 0; i < self->nthreads - 1; i++)
>>>            ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
>>> +    ASSERT_EQ(ret, 0);
>>
>>
>> Might be cleaner to also:
> 
> Or instead of? I agree this is neater, so will undo the moving of the ASSERT()
> and rely on this prctl.

I was thinking about possible races when our parent process already 
quits before our child managed to set the prctl. prctl() won't do 
anything in that case, hmmmm.

But similarly, existing code might already trigger the migrate() + kill 
before the child processes even started to access_mem().

Racy :)

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 6/8] selftests/mm: Make migration test robust to failure

[David Hildenbrand]

On 18.07.23 13:23, David Hildenbrand wrote:
> On 18.07.23 12:49, Ryan Roberts wrote:
>> On 17/07/2023 18:40, David Hildenbrand wrote:
>>> On 17.07.23 12:31, Ryan Roberts wrote:
>>>> The `migration` test currently has a number of robustness problems that
>>>> cause it to hang and leak resources.
>>>>
>>>> Timeout: There are 3 tests, which each previously ran for 60 seconds.
>>>> However, the timeout in mm/settings for a single test binary was set to
>>>> 45 seconds. So when run using run_kselftest.sh, the top level timeout
>>>> would trigger before the test binary was finished. Solve this by meeting
>>>> in the middle; each of the 3 tests now runs for 20 seconds (for a total
>>>> of 60), and the top level timeout is set to 90 seconds.
>>>>
>>>> Leaking child processes: the `shared_anon` test fork()s some children
>>>> but then an ASSERT() fires before the test kills those children. The
>>>> assert causes immediate exit of the parent and leaking of the children.
>>>> Furthermore, if run using the run_kselftest.sh wrapper, the wrapper
>>>> would get stuck waiting for those children to exit, which never happens.
>>>> Solve this by deferring any asserts until after the children are killed.
>>>> The same pattern is used for the threaded tests for uniformity.
>>>>
>>>> With these changes, the test binary now runs to completion on arm64,
>>>> with 2 tests passing and the `shared_anon` test failing.
>>>>
>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>> ---
>>>>     tools/testing/selftests/mm/migration.c | 14 ++++++++++----
>>>>     tools/testing/selftests/mm/settings    |  2 +-
>>>>     2 files changed, 11 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/mm/migration.c
>>>> b/tools/testing/selftests/mm/migration.c
>>>> index 379581567f27..189d7d9070e8 100644
>>>> --- a/tools/testing/selftests/mm/migration.c
>>>> +++ b/tools/testing/selftests/mm/migration.c
>>>> @@ -15,7 +15,7 @@
>>>>     #include <time.h>
>>>>       #define TWOMEG (2<<20)
>>>> -#define RUNTIME (60)
>>>> +#define RUNTIME (20)
>>>>       #define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1)))
>>>>     @@ -118,6 +118,7 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>>>     {
>>>>         uint64_t *ptr;
>>>>         int i;
>>>> +    int ret;
>>>>           if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>>>             SKIP(return, "Not enough threads or NUMA nodes available");
>>>> @@ -131,9 +132,10 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>>>             if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
>>>>                 perror("Couldn't create thread");
>>>>     -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>>>> +    ret = migrate(ptr, self->n1, self->n2);
>>>>         for (i = 0; i < self->nthreads - 1; i++)
>>>>             ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
>>>> +    ASSERT_EQ(ret, 0);
>>>
>>> Why is that required? This does not involve fork.
>>
>> It's not required. I was just trying to keep everything aligned to the same pattern.
>>
>>>
>>>>     }
>>>>       /*
>>>> @@ -144,6 +146,7 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>>>         pid_t pid;
>>>>         uint64_t *ptr;
>>>>         int i;
>>>> +    int ret;
>>>>           if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>>>             SKIP(return, "Not enough threads or NUMA nodes available");
>>>> @@ -161,9 +164,10 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>>>                 self->pids[i] = pid;
>>>>         }
>>>>     -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>>>> +    ret = migrate(ptr, self->n1, self->n2);
>>>>         for (i = 0; i < self->nthreads - 1; i++)
>>>>             ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
>>>> +    ASSERT_EQ(ret, 0);
>>>
>>>
>>> Might be cleaner to also:
>>
>> Or instead of? I agree this is neater, so will undo the moving of the ASSERT()
>> and rely on this prctl.
> 
> I was thinking about possible races when our parent process already
> quits before our child managed to set the prctl. prctl() won't do
> anything in that case, hmmmm.
> 
> But similarly, existing code might already trigger the migrate() + kill
> before the child processes even started to access_mem().
> 
> Racy :)
> 

Maybe what would work, is checking after the prctl() in the child if the 
parent is already gone.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 6/8] selftests/mm: Make migration test robust to failure

[Ryan Roberts]

On 18/07/2023 12:24, David Hildenbrand wrote:
> On 18.07.23 13:23, David Hildenbrand wrote:
>> On 18.07.23 12:49, Ryan Roberts wrote:
>>> On 17/07/2023 18:40, David Hildenbrand wrote:
>>>> On 17.07.23 12:31, Ryan Roberts wrote:
>>>>> The `migration` test currently has a number of robustness problems that
>>>>> cause it to hang and leak resources.
>>>>>
>>>>> Timeout: There are 3 tests, which each previously ran for 60 seconds.
>>>>> However, the timeout in mm/settings for a single test binary was set to
>>>>> 45 seconds. So when run using run_kselftest.sh, the top level timeout
>>>>> would trigger before the test binary was finished. Solve this by meeting
>>>>> in the middle; each of the 3 tests now runs for 20 seconds (for a total
>>>>> of 60), and the top level timeout is set to 90 seconds.
>>>>>
>>>>> Leaking child processes: the `shared_anon` test fork()s some children
>>>>> but then an ASSERT() fires before the test kills those children. The
>>>>> assert causes immediate exit of the parent and leaking of the children.
>>>>> Furthermore, if run using the run_kselftest.sh wrapper, the wrapper
>>>>> would get stuck waiting for those children to exit, which never happens.
>>>>> Solve this by deferring any asserts until after the children are killed.
>>>>> The same pattern is used for the threaded tests for uniformity.
>>>>>
>>>>> With these changes, the test binary now runs to completion on arm64,
>>>>> with 2 tests passing and the `shared_anon` test failing.
>>>>>
>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>> ---
>>>>>     tools/testing/selftests/mm/migration.c | 14 ++++++++++----
>>>>>     tools/testing/selftests/mm/settings    |  2 +-
>>>>>     2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/mm/migration.c
>>>>> b/tools/testing/selftests/mm/migration.c
>>>>> index 379581567f27..189d7d9070e8 100644
>>>>> --- a/tools/testing/selftests/mm/migration.c
>>>>> +++ b/tools/testing/selftests/mm/migration.c
>>>>> @@ -15,7 +15,7 @@
>>>>>     #include <time.h>
>>>>>       #define TWOMEG (2<<20)
>>>>> -#define RUNTIME (60)
>>>>> +#define RUNTIME (20)
>>>>>       #define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1)))
>>>>>     @@ -118,6 +118,7 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>>>>     {
>>>>>         uint64_t *ptr;
>>>>>         int i;
>>>>> +    int ret;
>>>>>           if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>>>>             SKIP(return, "Not enough threads or NUMA nodes available");
>>>>> @@ -131,9 +132,10 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>>>>             if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
>>>>>                 perror("Couldn't create thread");
>>>>>     -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>>>>> +    ret = migrate(ptr, self->n1, self->n2);
>>>>>         for (i = 0; i < self->nthreads - 1; i++)
>>>>>             ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
>>>>> +    ASSERT_EQ(ret, 0);
>>>>
>>>> Why is that required? This does not involve fork.
>>>
>>> It's not required. I was just trying to keep everything aligned to the same
>>> pattern.
>>>
>>>>
>>>>>     }
>>>>>       /*
>>>>> @@ -144,6 +146,7 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>>>>         pid_t pid;
>>>>>         uint64_t *ptr;
>>>>>         int i;
>>>>> +    int ret;
>>>>>           if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>>>>             SKIP(return, "Not enough threads or NUMA nodes available");
>>>>> @@ -161,9 +164,10 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>>>>                 self->pids[i] = pid;
>>>>>         }
>>>>>     -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>>>>> +    ret = migrate(ptr, self->n1, self->n2);
>>>>>         for (i = 0; i < self->nthreads - 1; i++)
>>>>>             ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
>>>>> +    ASSERT_EQ(ret, 0);
>>>>
>>>>
>>>> Might be cleaner to also:
>>>
>>> Or instead of? I agree this is neater, so will undo the moving of the ASSERT()
>>> and rely on this prctl.
>>
>> I was thinking about possible races when our parent process already
>> quits before our child managed to set the prctl. prctl() won't do
>> anything in that case, hmmmm.
>>
>> But similarly, existing code might already trigger the migrate() + kill
>> before the child processes even started to access_mem().
>>
>> Racy :)
>>
> 
> Maybe what would work, is checking after the prctl() in the child if the parent
> is already gone.


Like this?

	if (!pid) {
		prctl(PR_SET_PDEATHSIG, SIGHUP);
		/* Parent may have died before prctl so check now. */
		if (getppid() == 1)
			kill(getpid(), SIGHUP);
		access_mem(ptr);
	}

> 

Re: [PATCH v2 6/8] selftests/mm: Make migration test robust to failure

[David Hildenbrand]

On 18.07.23 14:42, Ryan Roberts wrote:
> On 18/07/2023 12:24, David Hildenbrand wrote:
>> On 18.07.23 13:23, David Hildenbrand wrote:
>>> On 18.07.23 12:49, Ryan Roberts wrote:
>>>> On 17/07/2023 18:40, David Hildenbrand wrote:
>>>>> On 17.07.23 12:31, Ryan Roberts wrote:
>>>>>> The `migration` test currently has a number of robustness problems that
>>>>>> cause it to hang and leak resources.
>>>>>>
>>>>>> Timeout: There are 3 tests, which each previously ran for 60 seconds.
>>>>>> However, the timeout in mm/settings for a single test binary was set to
>>>>>> 45 seconds. So when run using run_kselftest.sh, the top level timeout
>>>>>> would trigger before the test binary was finished. Solve this by meeting
>>>>>> in the middle; each of the 3 tests now runs for 20 seconds (for a total
>>>>>> of 60), and the top level timeout is set to 90 seconds.
>>>>>>
>>>>>> Leaking child processes: the `shared_anon` test fork()s some children
>>>>>> but then an ASSERT() fires before the test kills those children. The
>>>>>> assert causes immediate exit of the parent and leaking of the children.
>>>>>> Furthermore, if run using the run_kselftest.sh wrapper, the wrapper
>>>>>> would get stuck waiting for those children to exit, which never happens.
>>>>>> Solve this by deferring any asserts until after the children are killed.
>>>>>> The same pattern is used for the threaded tests for uniformity.
>>>>>>
>>>>>> With these changes, the test binary now runs to completion on arm64,
>>>>>> with 2 tests passing and the `shared_anon` test failing.
>>>>>>
>>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>>> ---
>>>>>>      tools/testing/selftests/mm/migration.c | 14 ++++++++++----
>>>>>>      tools/testing/selftests/mm/settings    |  2 +-
>>>>>>      2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/mm/migration.c
>>>>>> b/tools/testing/selftests/mm/migration.c
>>>>>> index 379581567f27..189d7d9070e8 100644
>>>>>> --- a/tools/testing/selftests/mm/migration.c
>>>>>> +++ b/tools/testing/selftests/mm/migration.c
>>>>>> @@ -15,7 +15,7 @@
>>>>>>      #include <time.h>
>>>>>>        #define TWOMEG (2<<20)
>>>>>> -#define RUNTIME (60)
>>>>>> +#define RUNTIME (20)
>>>>>>        #define ALIGN(x, a) (((x) + (a - 1)) & (~((a) - 1)))
>>>>>>      @@ -118,6 +118,7 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>>>>>      {
>>>>>>          uint64_t *ptr;
>>>>>>          int i;
>>>>>> +    int ret;
>>>>>>            if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>>>>>              SKIP(return, "Not enough threads or NUMA nodes available");
>>>>>> @@ -131,9 +132,10 @@ TEST_F_TIMEOUT(migration, private_anon, 2*RUNTIME)
>>>>>>              if (pthread_create(&self->threads[i], NULL, access_mem, ptr))
>>>>>>                  perror("Couldn't create thread");
>>>>>>      -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>>>>>> +    ret = migrate(ptr, self->n1, self->n2);
>>>>>>          for (i = 0; i < self->nthreads - 1; i++)
>>>>>>              ASSERT_EQ(pthread_cancel(self->threads[i]), 0);
>>>>>> +    ASSERT_EQ(ret, 0);
>>>>>
>>>>> Why is that required? This does not involve fork.
>>>>
>>>> It's not required. I was just trying to keep everything aligned to the same
>>>> pattern.
>>>>
>>>>>
>>>>>>      }
>>>>>>        /*
>>>>>> @@ -144,6 +146,7 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>>>>>          pid_t pid;
>>>>>>          uint64_t *ptr;
>>>>>>          int i;
>>>>>> +    int ret;
>>>>>>            if (self->nthreads < 2 || self->n1 < 0 || self->n2 < 0)
>>>>>>              SKIP(return, "Not enough threads or NUMA nodes available");
>>>>>> @@ -161,9 +164,10 @@ TEST_F_TIMEOUT(migration, shared_anon, 2*RUNTIME)
>>>>>>                  self->pids[i] = pid;
>>>>>>          }
>>>>>>      -    ASSERT_EQ(migrate(ptr, self->n1, self->n2), 0);
>>>>>> +    ret = migrate(ptr, self->n1, self->n2);
>>>>>>          for (i = 0; i < self->nthreads - 1; i++)
>>>>>>              ASSERT_EQ(kill(self->pids[i], SIGTERM), 0);
>>>>>> +    ASSERT_EQ(ret, 0);
>>>>>
>>>>>
>>>>> Might be cleaner to also:
>>>>
>>>> Or instead of? I agree this is neater, so will undo the moving of the ASSERT()
>>>> and rely on this prctl.
>>>
>>> I was thinking about possible races when our parent process already
>>> quits before our child managed to set the prctl. prctl() won't do
>>> anything in that case, hmmmm.
>>>
>>> But similarly, existing code might already trigger the migrate() + kill
>>> before the child processes even started to access_mem().
>>>
>>> Racy :)
>>>
>>
>> Maybe what would work, is checking after the prctl() in the child if the parent
>> is already gone.
> 
> 
> Like this?
> 
> 	if (!pid) {
> 		prctl(PR_SET_PDEATHSIG, SIGHUP);
> 		/* Parent may have died before prctl so check now. */
> 		if (getppid() == 1)
> 			kill(getpid(), SIGHUP);
> 		access_mem(ptr);
> 	}

Staring at forget_original_parent(), that order should work.

I do wonder if there is a nicer way to handle that, but maybe that 
already is the "nice" way.

-- 
Cheers,

David / dhildenb

[PATCH v2 7/8] selftests/mm: Optionally pass duration to transhuge-stress

[Ryan Roberts]

Until now, transhuge-stress runs until its explicitly killed, so when
invoked by run_kselftest.sh, it would run until the test timeout, then
it would be killed and the test would be marked as failed.

Add a new, optional command line parameter that allows the user to
specify the duration in seconds that the program should run. The program
exits after this duration with a success (0) exit code. If the argument
is omitted the old behacvior remains.

On it's own, this doesn't quite solve our problem because
run_kselftest.sh does not allow passing parameters to the program under
test. But we will shortly move this to run_vmtests.sh, which does allow
parameter passing.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/mm/transhuge-stress.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mm/transhuge-stress.c b/tools/testing/selftests/mm/transhuge-stress.c
index ba9d37ad3a89..c61fb9350b8c 100644
--- a/tools/testing/selftests/mm/transhuge-stress.c
+++ b/tools/testing/selftests/mm/transhuge-stress.c
@@ -25,13 +25,14 @@ int main(int argc, char **argv)
 {
 	size_t ram, len;
 	void *ptr, *p;
-	struct timespec a, b;
+	struct timespec start, a, b;
 	int i = 0;
 	char *name = NULL;
 	double s;
 	uint8_t *map;
 	size_t map_len;
 	int pagemap_fd;
+	int duration = 0;
 
 	ram = sysconf(_SC_PHYS_PAGES);
 	if (ram > SIZE_MAX / psize() / 4)
@@ -42,9 +43,11 @@ int main(int argc, char **argv)
 
 	while (++i < argc) {
 		if (!strcmp(argv[i], "-h"))
-			errx(1, "usage: %s [size in MiB]", argv[0]);
+			errx(1, "usage: %s [-f <filename>] [-d <duration>] [size in MiB]", argv[0]);
 		else if (!strcmp(argv[i], "-f"))
 			name = argv[++i];
+		else if (!strcmp(argv[i], "-d"))
+			duration = atoi(argv[++i]);
 		else
 			len = atoll(argv[i]) << 20;
 	}
@@ -78,6 +81,8 @@ int main(int argc, char **argv)
 	if (!map)
 		errx(2, "map malloc");
 
+	clock_gettime(CLOCK_MONOTONIC, &start);
+
 	while (1) {
 		int nr_succeed = 0, nr_failed = 0, nr_pages = 0;
 
@@ -118,5 +123,8 @@ int main(int argc, char **argv)
 		      "%4d succeed, %4d failed, %4d different pages",
 		      s, s * 1000 / (len >> HPAGE_SHIFT), len / s / (1 << 20),
 		      nr_succeed, nr_failed, nr_pages);
+
+		if (duration > 0 && b.tv_sec - start.tv_sec >= duration)
+			return 0;
 	}
 }
-- 
2.25.1

Re: [PATCH v2 7/8] selftests/mm: Optionally pass duration to transhuge-stress

[David Hildenbrand]

On 17.07.23 12:31, Ryan Roberts wrote:
> Until now, transhuge-stress runs until its explicitly killed, so when
> invoked by run_kselftest.sh, it would run until the test timeout, then
> it would be killed and the test would be marked as failed.
> 
> Add a new, optional command line parameter that allows the user to
> specify the duration in seconds that the program should run. The program
> exits after this duration with a success (0) exit code. If the argument
> is omitted the old behacvior remains.
> 
> On it's own, this doesn't quite solve our problem because
> run_kselftest.sh does not allow passing parameters to the program under
> test. But we will shortly move this to run_vmtests.sh, which does allow
> parameter passing.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   tools/testing/selftests/mm/transhuge-stress.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/transhuge-stress.c b/tools/testing/selftests/mm/transhuge-stress.c
> index ba9d37ad3a89..c61fb9350b8c 100644
> --- a/tools/testing/selftests/mm/transhuge-stress.c
> +++ b/tools/testing/selftests/mm/transhuge-stress.c
> @@ -25,13 +25,14 @@ int main(int argc, char **argv)
>   {
>   	size_t ram, len;
>   	void *ptr, *p;
> -	struct timespec a, b;
> +	struct timespec start, a, b;
>   	int i = 0;
>   	char *name = NULL;
>   	double s;
>   	uint8_t *map;
>   	size_t map_len;
>   	int pagemap_fd;
> +	int duration = 0;
>   
>   	ram = sysconf(_SC_PHYS_PAGES);
>   	if (ram > SIZE_MAX / psize() / 4)
> @@ -42,9 +43,11 @@ int main(int argc, char **argv)
>   
>   	while (++i < argc) {
>   		if (!strcmp(argv[i], "-h"))
> -			errx(1, "usage: %s [size in MiB]", argv[0]);
> +			errx(1, "usage: %s [-f <filename>] [-d <duration>] [size in MiB]", argv[0]);
>   		else if (!strcmp(argv[i], "-f"))
>   			name = argv[++i];
> +		else if (!strcmp(argv[i], "-d"))
> +			duration = atoi(argv[++i]);
>   		else
>   			len = atoll(argv[i]) << 20;
>   	}
> @@ -78,6 +81,8 @@ int main(int argc, char **argv)
>   	if (!map)
>   		errx(2, "map malloc");
>   
> +	clock_gettime(CLOCK_MONOTONIC, &start);
> +
>   	while (1) {
>   		int nr_succeed = 0, nr_failed = 0, nr_pages = 0;
>   
> @@ -118,5 +123,8 @@ int main(int argc, char **argv)
>   		      "%4d succeed, %4d failed, %4d different pages",
>   		      s, s * 1000 / (len >> HPAGE_SHIFT), len / s / (1 << 20),
>   		      nr_succeed, nr_failed, nr_pages);
> +
> +		if (duration > 0 && b.tv_sec - start.tv_sec >= duration)
> +			return 0;
>   	}
>   }

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

[PATCH v2 8/8] selftests/mm: Run all tests from run_vmtests.sh

[Ryan Roberts]

It is very unclear to me how one is supposed to run all the mm selftests
consistently and get clear results.

Most of the test programs are launched by both run_vmtests.sh and
run_kselftest.sh:

  hugepage-mmap
  hugepage-shm
  map_hugetlb
  hugepage-mremap
  hugepage-vmemmap
  hugetlb-madvise
  map_fixed_noreplace
  gup_test
  gup_longterm
  uffd-unit-tests
  uffd-stress
  compaction_test
  on-fault-limit
  map_populate
  mlock-random-test
  mlock2-tests
  mrelease_test
  mremap_test
  thuge-gen
  virtual_address_range
  va_high_addr_switch
  mremap_dontunmap
  hmm-tests
  madv_populate
  memfd_secret
  ksm_tests
  ksm_functional_tests
  soft-dirty
  cow

However, of this set, when launched by run_vmtests.sh, some of the
programs are invoked multiple times with different arguments. When
invoked by run_kselftest.sh, they are invoked without arguments (and as
a consequence, some fail immediately).

Some test programs are only launched by run_vmtests.sh:

  test_vmalloc.sh

And some test programs and only launched by run_kselftest.sh:

  khugepaged
  migration
  mkdirty
  transhuge-stress
  split_huge_page_test
  mdwe_test
  write_to_hugetlbfs

Furthermore, run_vmtests.sh is invoked by run_kselftest.sh, so in this
case all the test programs invoked by both scripts are run twice!

Needless to say, this is a bit of a mess. In the absence of fully
understanding the history here, it looks to me like the best solution is
to launch ALL test programs from run_vmtests.sh, and ONLY invoke
run_vmtests.sh from run_kselftest.sh. This way, we get full control over
the parameters, each program is only invoked the intended number of
times, and regardless of which script is used, the same tests get run in
the same way.

The only drawback is that if using run_kselftest.sh, it's top-level tap
result reporting reports only a single test and it fails if any of the
contained tests fail. I don't see this as a big deal though since we
still see all the nested reporting from multiple layers. The other issue
with this is that all of run_vmtests.sh must execute within a single
kselftest timeout period, so let's increase that to something more
suitable.

In the Makefile, TEST_GEN_PROGS will compile and install the tests and
will add them to the list of tests that run_kselftest.sh will run.
TEST_GEN_FILES will compile and install the tests but will not add them
to the test list. So let's move all the programs from TEST_GEN_PROGS to
TEST_GEN_FILES so that they are built but not executed by
run_kselftest.sh. Note that run_vmtests.sh is added to TEST_PROGS, which
means it ends up in the test list. (the lack of "_GEN" means it won't be
compiled, but simply copied).

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/mm/Makefile       | 79 ++++++++++++-----------
 tools/testing/selftests/mm/run_vmtests.sh | 23 +++++++
 tools/testing/selftests/mm/settings       |  2 +-
 3 files changed, 64 insertions(+), 40 deletions(-)

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 3514697fc2db..f39ba27d15fa 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -35,41 +35,41 @@ MAKEFLAGS += --no-builtin-rules
 CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
 LDLIBS = -lrt -lpthread
 
-TEST_GEN_PROGS = cow
-TEST_GEN_PROGS += compaction_test
-TEST_GEN_PROGS += gup_longterm
-TEST_GEN_PROGS += gup_test
-TEST_GEN_PROGS += hmm-tests
-TEST_GEN_PROGS += hugetlb-madvise
-TEST_GEN_PROGS += hugepage-mmap
-TEST_GEN_PROGS += hugepage-mremap
-TEST_GEN_PROGS += hugepage-shm
-TEST_GEN_PROGS += hugepage-vmemmap
-TEST_GEN_PROGS += khugepaged
-TEST_GEN_PROGS += madv_populate
-TEST_GEN_PROGS += map_fixed_noreplace
-TEST_GEN_PROGS += map_hugetlb
-TEST_GEN_PROGS += map_populate
-TEST_GEN_PROGS += memfd_secret
-TEST_GEN_PROGS += migration
-TEST_GEN_PROGS += mkdirty
-TEST_GEN_PROGS += mlock-random-test
-TEST_GEN_PROGS += mlock2-tests
-TEST_GEN_PROGS += mrelease_test
-TEST_GEN_PROGS += mremap_dontunmap
-TEST_GEN_PROGS += mremap_test
-TEST_GEN_PROGS += on-fault-limit
-TEST_GEN_PROGS += thuge-gen
-TEST_GEN_PROGS += transhuge-stress
-TEST_GEN_PROGS += uffd-stress
-TEST_GEN_PROGS += uffd-unit-tests
-TEST_GEN_PROGS += split_huge_page_test
-TEST_GEN_PROGS += ksm_tests
-TEST_GEN_PROGS += ksm_functional_tests
-TEST_GEN_PROGS += mdwe_test
+TEST_GEN_FILES = cow
+TEST_GEN_FILES += compaction_test
+TEST_GEN_FILES += gup_longterm
+TEST_GEN_FILES += gup_test
+TEST_GEN_FILES += hmm-tests
+TEST_GEN_FILES += hugetlb-madvise
+TEST_GEN_FILES += hugepage-mmap
+TEST_GEN_FILES += hugepage-mremap
+TEST_GEN_FILES += hugepage-shm
+TEST_GEN_FILES += hugepage-vmemmap
+TEST_GEN_FILES += khugepaged
+TEST_GEN_FILES += madv_populate
+TEST_GEN_FILES += map_fixed_noreplace
+TEST_GEN_FILES += map_hugetlb
+TEST_GEN_FILES += map_populate
+TEST_GEN_FILES += memfd_secret
+TEST_GEN_FILES += migration
+TEST_GEN_FILES += mkdirty
+TEST_GEN_FILES += mlock-random-test
+TEST_GEN_FILES += mlock2-tests
+TEST_GEN_FILES += mrelease_test
+TEST_GEN_FILES += mremap_dontunmap
+TEST_GEN_FILES += mremap_test
+TEST_GEN_FILES += on-fault-limit
+TEST_GEN_FILES += thuge-gen
+TEST_GEN_FILES += transhuge-stress
+TEST_GEN_FILES += uffd-stress
+TEST_GEN_FILES += uffd-unit-tests
+TEST_GEN_FILES += split_huge_page_test
+TEST_GEN_FILES += ksm_tests
+TEST_GEN_FILES += ksm_functional_tests
+TEST_GEN_FILES += mdwe_test
 
 ifneq ($(ARCH),arm64)
-TEST_GEN_PROGS += soft-dirty
+TEST_GEN_FILES += soft-dirty
 endif
 
 ifeq ($(ARCH),x86_64)
@@ -86,24 +86,24 @@ CFLAGS += -no-pie
 endif
 
 ifeq ($(CAN_BUILD_I386),1)
-TEST_GEN_PROGS += $(BINARIES_32)
+TEST_GEN_FILES += $(BINARIES_32)
 endif
 
 ifeq ($(CAN_BUILD_X86_64),1)
-TEST_GEN_PROGS += $(BINARIES_64)
+TEST_GEN_FILES += $(BINARIES_64)
 endif
 else
 
 ifneq (,$(findstring $(ARCH),ppc64))
-TEST_GEN_PROGS += protection_keys
+TEST_GEN_FILES += protection_keys
 endif
 
 endif
 
 ifneq (,$(filter $(ARCH),arm64 ia64 mips64 parisc64 ppc64 riscv64 s390x sparc64 x86_64))
-TEST_GEN_PROGS += va_high_addr_switch
-TEST_GEN_PROGS += virtual_address_range
-TEST_GEN_PROGS += write_to_hugetlbfs
+TEST_GEN_FILES += va_high_addr_switch
+TEST_GEN_FILES += virtual_address_range
+TEST_GEN_FILES += write_to_hugetlbfs
 endif
 
 TEST_PROGS := run_vmtests.sh
@@ -115,6 +115,7 @@ TEST_FILES += va_high_addr_switch.sh
 include ../lib.mk
 
 $(TEST_GEN_PROGS): vm_util.c
+$(TEST_GEN_FILES): vm_util.c
 
 $(OUTPUT)/uffd-stress: uffd-common.c
 $(OUTPUT)/uffd-unit-tests: uffd-common.c
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
index 9e4338aa5e09..cc2cbc4405ff 100755
--- a/tools/testing/selftests/mm/run_vmtests.sh
+++ b/tools/testing/selftests/mm/run_vmtests.sh
@@ -55,6 +55,17 @@ separated by spaces:
 	test soft dirty page bit semantics
 - cow
 	test copy-on-write semantics
+- thp
+	test transparent huge pages
+- migration
+	invoke move_pages(2) to exercise the migration entry code
+	paths in the kernel
+- mkdirty
+	test handling of code that might set PTE/PMD dirty in
+	read-only VMAs
+- mdwe
+	test prctl(PR_SET_MDWE, ...)
+
 example: ./run_vmtests.sh -t "hmm mmap ksm"
 EOF
 	exit 0
@@ -298,6 +309,18 @@ fi
 # COW tests
 CATEGORY="cow" run_test ./cow
 
+CATEGORY="thp" run_test ./khugepaged
+
+CATEGORY="thp" run_test ./transhuge-stress -d 20
+
+CATEGORY="thp" run_test ./split_huge_page_test
+
+CATEGORY="migration" run_test ./migration
+
+CATEGORY="mkdirty" run_test ./mkdirty
+
+CATEGORY="mdwe" run_test ./mdwe_test
+
 echo "SUMMARY: PASS=${count_pass} SKIP=${count_skip} FAIL=${count_fail}"
 
 exit $exitcode
diff --git a/tools/testing/selftests/mm/settings b/tools/testing/selftests/mm/settings
index ba4d85f74cd6..a953c96aa16e 100644
--- a/tools/testing/selftests/mm/settings
+++ b/tools/testing/selftests/mm/settings
@@ -1 +1 @@
-timeout=90
+timeout=180
-- 
2.25.1

Re: [PATCH v2 8/8] selftests/mm: Run all tests from run_vmtests.sh

[David Hildenbrand]

On 17.07.23 12:31, Ryan Roberts wrote:
> It is very unclear to me how one is supposed to run all the mm selftests
> consistently and get clear results.
> 
> Most of the test programs are launched by both run_vmtests.sh and
> run_kselftest.sh:
> 
>    hugepage-mmap
>    hugepage-shm
>    map_hugetlb
>    hugepage-mremap
>    hugepage-vmemmap
>    hugetlb-madvise
>    map_fixed_noreplace
>    gup_test
>    gup_longterm
>    uffd-unit-tests
>    uffd-stress
>    compaction_test
>    on-fault-limit
>    map_populate
>    mlock-random-test
>    mlock2-tests
>    mrelease_test
>    mremap_test
>    thuge-gen
>    virtual_address_range
>    va_high_addr_switch
>    mremap_dontunmap
>    hmm-tests
>    madv_populate
>    memfd_secret
>    ksm_tests
>    ksm_functional_tests
>    soft-dirty
>    cow
> 
> However, of this set, when launched by run_vmtests.sh, some of the
> programs are invoked multiple times with different arguments. When
> invoked by run_kselftest.sh, they are invoked without arguments (and as
> a consequence, some fail immediately).
> 
> Some test programs are only launched by run_vmtests.sh:
> 
>    test_vmalloc.sh
> 
> And some test programs and only launched by run_kselftest.sh:
> 
>    khugepaged
>    migration
>    mkdirty
>    transhuge-stress
>    split_huge_page_test
>    mdwe_test
>    write_to_hugetlbfs
> 
> Furthermore, run_vmtests.sh is invoked by run_kselftest.sh, so in this
> case all the test programs invoked by both scripts are run twice!
> 
> Needless to say, this is a bit of a mess. In the absence of fully
> understanding the history here, it looks to me like the best solution is
> to launch ALL test programs from run_vmtests.sh, and ONLY invoke
> run_vmtests.sh from run_kselftest.sh. This way, we get full control over
> the parameters, each program is only invoked the intended number of
> times, and regardless of which script is used, the same tests get run in
> the same way.
> 
> The only drawback is that if using run_kselftest.sh, it's top-level tap
> result reporting reports only a single test and it fails if any of the
> contained tests fail. I don't see this as a big deal though since we
> still see all the nested reporting from multiple layers. The other issue
> with this is that all of run_vmtests.sh must execute within a single
> kselftest timeout period, so let's increase that to something more
> suitable.
> 
> In the Makefile, TEST_GEN_PROGS will compile and install the tests and
> will add them to the list of tests that run_kselftest.sh will run.
> TEST_GEN_FILES will compile and install the tests but will not add them
> to the test list. So let's move all the programs from TEST_GEN_PROGS to
> TEST_GEN_FILES so that they are built but not executed by
> run_kselftest.sh. Note that run_vmtests.sh is added to TEST_PROGS, which
> means it ends up in the test list. (the lack of "_GEN" means it won't be
> compiled, but simply copied).
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 8/8] selftests/mm: Run all tests from run_vmtests.sh

[Peter Xu]

On Mon, Jul 17, 2023 at 07:27:13PM +0200, David Hildenbrand wrote:
> On 17.07.23 12:31, Ryan Roberts wrote:
> > It is very unclear to me how one is supposed to run all the mm selftests
> > consistently and get clear results.
> > 
> > Most of the test programs are launched by both run_vmtests.sh and
> > run_kselftest.sh:
> > 
> >    hugepage-mmap
> >    hugepage-shm
> >    map_hugetlb
> >    hugepage-mremap
> >    hugepage-vmemmap
> >    hugetlb-madvise
> >    map_fixed_noreplace
> >    gup_test
> >    gup_longterm
> >    uffd-unit-tests
> >    uffd-stress
> >    compaction_test
> >    on-fault-limit
> >    map_populate
> >    mlock-random-test
> >    mlock2-tests
> >    mrelease_test
> >    mremap_test
> >    thuge-gen
> >    virtual_address_range
> >    va_high_addr_switch
> >    mremap_dontunmap
> >    hmm-tests
> >    madv_populate
> >    memfd_secret
> >    ksm_tests
> >    ksm_functional_tests
> >    soft-dirty
> >    cow
> > 
> > However, of this set, when launched by run_vmtests.sh, some of the
> > programs are invoked multiple times with different arguments. When
> > invoked by run_kselftest.sh, they are invoked without arguments (and as
> > a consequence, some fail immediately).
> > 
> > Some test programs are only launched by run_vmtests.sh:
> > 
> >    test_vmalloc.sh
> > 
> > And some test programs and only launched by run_kselftest.sh:
> > 
> >    khugepaged
> >    migration
> >    mkdirty
> >    transhuge-stress
> >    split_huge_page_test
> >    mdwe_test
> >    write_to_hugetlbfs
> > 
> > Furthermore, run_vmtests.sh is invoked by run_kselftest.sh, so in this
> > case all the test programs invoked by both scripts are run twice!
> > 
> > Needless to say, this is a bit of a mess. In the absence of fully
> > understanding the history here, it looks to me like the best solution is
> > to launch ALL test programs from run_vmtests.sh, and ONLY invoke
> > run_vmtests.sh from run_kselftest.sh. This way, we get full control over
> > the parameters, each program is only invoked the intended number of
> > times, and regardless of which script is used, the same tests get run in
> > the same way.
> > 
> > The only drawback is that if using run_kselftest.sh, it's top-level tap
> > result reporting reports only a single test and it fails if any of the
> > contained tests fail. I don't see this as a big deal though since we
> > still see all the nested reporting from multiple layers. The other issue
> > with this is that all of run_vmtests.sh must execute within a single
> > kselftest timeout period, so let's increase that to something more
> > suitable.
> > 
> > In the Makefile, TEST_GEN_PROGS will compile and install the tests and
> > will add them to the list of tests that run_kselftest.sh will run.
> > TEST_GEN_FILES will compile and install the tests but will not add them
> > to the test list. So let's move all the programs from TEST_GEN_PROGS to
> > TEST_GEN_FILES so that they are built but not executed by
> > run_kselftest.sh. Note that run_vmtests.sh is added to TEST_PROGS, which
> > means it ends up in the test list. (the lack of "_GEN" means it won't be
> > compiled, but simply copied).
> > 
> > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> > ---
> 
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks for letting me know, David.  Sorry for the late response, still
catching up things.

I used to justify that from mm/ itself that everything should be PROG, but
I see that from higher level where TEST_GEN_FILE|PROG is really used this
makes sense.  As long as vm_utils.o will be properly linked I'll be happy
enough..

Acked-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

Re: [PATCH v2 8/8] selftests/mm: Run all tests from run_vmtests.sh

[Ryan Roberts]

On 19/07/2023 21:45, Peter Xu wrote:
> On Mon, Jul 17, 2023 at 07:27:13PM +0200, David Hildenbrand wrote:
>> On 17.07.23 12:31, Ryan Roberts wrote:
>>> It is very unclear to me how one is supposed to run all the mm selftests
>>> consistently and get clear results.
>>>
>>> Most of the test programs are launched by both run_vmtests.sh and
>>> run_kselftest.sh:
>>>
>>>    hugepage-mmap
>>>    hugepage-shm
>>>    map_hugetlb
>>>    hugepage-mremap
>>>    hugepage-vmemmap
>>>    hugetlb-madvise
>>>    map_fixed_noreplace
>>>    gup_test
>>>    gup_longterm
>>>    uffd-unit-tests
>>>    uffd-stress
>>>    compaction_test
>>>    on-fault-limit
>>>    map_populate
>>>    mlock-random-test
>>>    mlock2-tests
>>>    mrelease_test
>>>    mremap_test
>>>    thuge-gen
>>>    virtual_address_range
>>>    va_high_addr_switch
>>>    mremap_dontunmap
>>>    hmm-tests
>>>    madv_populate
>>>    memfd_secret
>>>    ksm_tests
>>>    ksm_functional_tests
>>>    soft-dirty
>>>    cow
>>>
>>> However, of this set, when launched by run_vmtests.sh, some of the
>>> programs are invoked multiple times with different arguments. When
>>> invoked by run_kselftest.sh, they are invoked without arguments (and as
>>> a consequence, some fail immediately).
>>>
>>> Some test programs are only launched by run_vmtests.sh:
>>>
>>>    test_vmalloc.sh
>>>
>>> And some test programs and only launched by run_kselftest.sh:
>>>
>>>    khugepaged
>>>    migration
>>>    mkdirty
>>>    transhuge-stress
>>>    split_huge_page_test
>>>    mdwe_test
>>>    write_to_hugetlbfs
>>>
>>> Furthermore, run_vmtests.sh is invoked by run_kselftest.sh, so in this
>>> case all the test programs invoked by both scripts are run twice!
>>>
>>> Needless to say, this is a bit of a mess. In the absence of fully
>>> understanding the history here, it looks to me like the best solution is
>>> to launch ALL test programs from run_vmtests.sh, and ONLY invoke
>>> run_vmtests.sh from run_kselftest.sh. This way, we get full control over
>>> the parameters, each program is only invoked the intended number of
>>> times, and regardless of which script is used, the same tests get run in
>>> the same way.
>>>
>>> The only drawback is that if using run_kselftest.sh, it's top-level tap
>>> result reporting reports only a single test and it fails if any of the
>>> contained tests fail. I don't see this as a big deal though since we
>>> still see all the nested reporting from multiple layers. The other issue
>>> with this is that all of run_vmtests.sh must execute within a single
>>> kselftest timeout period, so let's increase that to something more
>>> suitable.
>>>
>>> In the Makefile, TEST_GEN_PROGS will compile and install the tests and
>>> will add them to the list of tests that run_kselftest.sh will run.
>>> TEST_GEN_FILES will compile and install the tests but will not add them
>>> to the test list. So let's move all the programs from TEST_GEN_PROGS to
>>> TEST_GEN_FILES so that they are built but not executed by
>>> run_kselftest.sh. Note that run_vmtests.sh is added to TEST_PROGS, which
>>> means it ends up in the test list. (the lack of "_GEN" means it won't be
>>> compiled, but simply copied).
>>>
>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
> 
> Thanks for letting me know, David.  Sorry for the late response, still
> catching up things.
> 
> I used to justify that from mm/ itself that everything should be PROG, but
> I see that from higher level where TEST_GEN_FILE|PROG is really used this
> makes sense.  As long as vm_utils.o will be properly linked I'll be happy
> enough..

Yep that's the case; I've set it up so that vm_utils.o is linked in for both
TEST_GEN_FILE and TEST_GEN_PROG binaries.

> 
> Acked-by: Peter Xu <peterx@redhat.com>

Thanks!

> 
> Thanks,
>