[PATCH v1 0/9] selftests/mm fixes for arm64

[PATCH v1 0/9] selftests/mm fixes for arm64

[Ryan Roberts]

Hi All,

Given my on-going work on large anon folios and contpte mappings, I decided it
would be a good idea to start running mm selftests to help guard against
regressions. However, it soon became clear that I couldn't get the suite to run
cleanly on arm64 with a vanilla v6.5-rc1 kernel (perhaps I'm just doing it
wrong??), so got stuck in a rabbit hole trying to debug and fix all the issues.
Some were down to misconfigurations, but I also found a number of issues with
the tests and even a couple of issues with the kernel.

This series aims to fix (most of) the test issues. It applies on top of
v6.5-rc1.


Reproducing
-----------

What follows is a write up of how I'm running the tests and the results I see
with this series applied. I don't yet have a concrete understanding of all of
the remaining failures. So if anyone has any comments on my setup or reasons for
the test failures it would be great to hear.

Source: v6.5-rc1 + this series + [1] + [2]. [1] is a patch from Florent Revest to
fix mdwe mmap_FIXED tests. [2] is a fix for a regression in the kernel that I
found by running `mlock-random-test` and `mlock2-tests`.

Compile the kernel (on arm64 system):

$ make defconfig
$ ./scripts/config --enable CONFIG_SQUASHFS_LZ4
$ ./scripts/config --enable CONFIG_SQUASHFS_LZO
$ ./scripts/config --enable CONFIG_SQUASHFS_XZ
$ ./scripts/config --enable CONFIG_SQUASHFS_ZSTD
$ ./scripts/config --enable CONFIG_XFS_FS
$ ./scripts/config --enable CONFIG_SYSVIPC
$ ./scripts/config --enable CONFIG_USERFAULTFD
$ ./scripts/config --enable CONFIG_TEST_VMALLOC
$ ./scripts/config --enable CONFIG_GUP_TEST
$ ./scripts/config --enable CONFIG_TRANSPARENT_HUGEPAGE
$ ./scripts/config --enable CONFIG_MEM_SOFT_DIRTY
$ make olddefconfig
$ make -s -j`nproc` Image

(In the above case, I'm building/testing a 4K kernel).

Note that it turns out that arm64 doesn't really support ZONE_DEVICE; Although
it defines ARCH_HAS_PTE_DEVMAP, it can't allocate `struct page`s for arbitrary
physical addresses. This means that the TEST_HMM module causes warnings to be
emitted when initializing because it tries to reserve arbitrary PA range then
requests struct page's for them. I haven't fully investigated this yet, but for
now, I'm just deliverately excluding ZONE_DEVICE, (which TEST_HMM depends upon).
This means that the `hmm-tests` selftest gets skipped at runtime.

Compile the tests:

$ make -j`nproc` headers_install
$ make -C tools/testing/selftests TARGETS=mm install INSTALL_PATH=<path/to/install>

Start a VM running the kernel we just compiled:

$ taskset -c 8-15 qemu-system-aarch64							\
	-object memory-backend-file,id=mem0,size=6G,mem-path=/hugetlbfs,merge=off,prealloc=on,host-nodes=0,policy=bind,align=1G \
	-object memory-backend-file,id=mem1,size=6G,mem-path=/hugetlbfs,merge=off,prealloc=on,host-nodes=0,policy=bind,align=1G \
	-nographic -enable-kvm -machine virt,gic-version=3 -cpu max			\
	-smp 8 -m 12G									\
	-numa node,memdev=mem0,cpus=0-3,nodeid=0					\
	-numa node,memdev=mem1,cpus=4-7,nodeid=1					\
	-drive if=virtio,format=raw,file=ubuntu-22.04.xfs				\
	-object rng-random,filename=/dev/urandom,id=rng0				\
	-device virtio-scsi-pci,id=scsi0						\
	-netdev user,id=net0,hostfwd=tcp::8022-:22					\
	-device virtio-rng-pci,rng=rng0							\
	-device virtio-net-pci,netdev=net0						\
	-kernel arch/arm64/boot/Image							\
	-append "earlycon root=/dev/vda2 secretmem.enable hugepagesz=1G hugepages=0:2,1:2 hugepagesz=32M hugepages=0:2,1:2 default_hugepagesz=2M hugepages=0:64,1:64 hugepagesz=64K hugepages=0:2,1:2"

This starts a VM with 2 numa nodes (needed by ksm and migration tests), with 6G
of memory and 4 CPUs on each node. The kernel command line enables secretmem
(needed for `memfd_secret` test), and preallocates a bunch of huge pages
(divined by reading the comments and source for a bunch of tests that require
huge pages). 128M of the default huge page size, and 4 pages of each of the
other sizes appear to be sufficient. I'm allocating half on each numa node.

Once booted, copy the selftests we just compiled onto it.

On the VM, run the tests:

$ cd path/to/selftests
$ sudo ./run_kselftest.sh

or alternatively:

$ cd path/to/selftests/mm
$ sudo ./run_vmtests.sh


Test Results
------------

TOP-LEVEL SUMMARY: PASS=42 SKIP=4 FAIL=2

Only showing nested tests if they are skipped or failed.

[PASS] hugepage-mmap
[PASS] hugepage-shm
[PASS] map_hugetlb
[PASS] hugepage-mremap
[PASS] hugepage-vmemmap
[PASS] hugetlb-madvise
[PASS] map_fixed_noreplace
[PASS] gup_test -u
[PASS] gup_test -a
[PASS] gup_test -ct -F 0x1 0 19 0x1000
[PASS] gup_longterm
[PASS] uffd-unit-tests
[PASS] uffd-stress anon 20 16
[PASS] uffd-stress hugetlb 128 32
[PASS] uffd-stress hugetlb-private 128 32
[PASS] uffd-stress shmem 20 16
[PASS] uffd-stress shmem-private 20 16
[PASS] compaction_test
[PASS] on-fault-limit
[PASS] map_populate
[PASS] mlock-random-test
[PASS] mlock2-tests
[PASS] mrelease_test
[PASS] mremap_test
[PASS] thuge-gen
[PASS] virtual_address_range
[SKIP] va_high_addr_switch.sh
	# 4K kernel does not support big enough VA space for test
[SKIP] test_vmalloc.sh smoke
	# Test requires test_vmalloc kernel module which isn't present
[PASS] mremap_dontunmap
[SKIP] test_hmm.sh smoke
	# Test requires test_hmm kernel module - see ZONE_DEVICE issue above
[PASS] madv_populate
	[PASS] test_softdirty
		[SKIP] range is not softdirty
		[SKIP] MADV_POPULATE_READ
		[SKIP] range is not softdirty
		[SKIP] MADV_POPULATE_WRITE
		[SKIP] range is softdirty
		# All skipped because arm64 does not support soft-dirty
[PASS] memfd_secret
[PASS] ksm_tests -M -p 10
[PASS] ksm_tests -U
[PASS] ksm_tests -Z -p 10 -z 0
[PASS] ksm_tests -Z -p 10 -z 1
[PASS] ksm_tests -N -m 1
[PASS] ksm_tests -N -m 0
[PASS] ksm_functional_tests
	[SKIP] test_unmerge_uffd_wp
		# UFFD_FEATURE_PAGEFAULT_FLAG_WP not available on arm64
[PASS] ksm_functional_tests
	[SKIP] test_unmerge_uffd_wp
		# UFFD_FEATURE_PAGEFAULT_FLAG_WP not available on arm64
[SKIP] soft-dirty
	# Skipped because arm64 does not support soft-dirty
[FAIL] cow
	[FAIL] vmsplice() + unmap in child ... with hugetlb
	[FAIL] vmsplice() + unmap in child with mprotect() optimization ... with hugetlb
	[FAIL] vmsplice() before fork(), unmap in parent after fork() ... with hugetlb
	[FAIL] vmsplice() + unmap in parent after fork() ... with hugetlb
		# Above are known issues for vmsplice + hugetlb
		# Reproduces on x86
	[SKIP] Basic COW after fork() ... with swapped-out, PTE-mapped THP
	[SKIP] Basic COW after fork() with mprotect() optimization ... with swapped-out, PTE-mapped THP
	[SKIP] vmsplice() + unmap in child ... with swapped-out, PTE-mapped THP
	[SKIP] vmsplice() + unmap in child with mprotect() optimization ... with swapped-out, PTE-mapped THP
	[SKIP] vmsplice() before fork(), unmap in parent after fork() ... with swapped-out, PTE-mapped THP
	[SKIP] vmsplice() + unmap in parent after fork() ... with swapped-out, PTE-mapped THP
	[SKIP] R/O-mapping a page registered as iouring fixed buffer ... with swapped-out, PTE-mapped THP
	[SKIP] fork() with an iouring fixed buffer ... with swapped-out, PTE-mapped THP
	[SKIP] R/O GUP pin on R/O-mapped shared page ... with swapped-out, PTE-mapped THP
	[SKIP] R/O GUP-fast pin on R/O-mapped shared page ... with swapped-out, PTE-mapped THP
	[SKIP] R/O GUP pin on R/O-mapped previously-shared page ... with swapped-out, PTE-mapped THP
	[SKIP] R/O GUP-fast pin on R/O-mapped previously-shared page ... with swapped-out, PTE-mapped THP
	[SKIP] R/O GUP pin on R/O-mapped exclusive page ... with swapped-out, PTE-mapped THP
	[SKIP] R/O GUP-fast pin on R/O-mapped exclusive page ... with swapped-out, PTE-mapped THP
		# Above all skipped due to "MADV_PAGEOUT did not work, is swap enabled?"
		# swap is enabled though
		# Reproduces on x86
	[SKIP] Basic COW after fork() when collapsing after fork() (fully shared)
		# MADV_COLLAPSE failed: Invalid argument
[PASS] khugepaged
[PASS] transhuge-stress -d 20
[PASS] split_huge_page_test
[FAIL] migration
	[FAIL] migration.shared_anon
		# move_pages() reports that the requested page was not migrated
		# after a few iterations.
[PASS] mkdirty
[PASS] mdwe_test


[1] https://lore.kernel.org/lkml/20230704153630.1591122-3-revest@chromium.org/
[2] https://lore.kernel.org/linux-mm/20230711175020.4091336-1-Liam.Howlett@oracle.com/

Thanks,
Ryan


Ryan Roberts (9):
  selftests: Line buffer test program's stdout
  selftests/mm: Give scripts execute permission
  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/runner.sh   |  5 +-
 tools/testing/selftests/mm/Makefile           | 79 ++++++++++---------
 .../selftests/mm/charge_reserved_hugetlb.sh   |  0
 tools/testing/selftests/mm/check_config.sh    |  0
 .../selftests/mm/hugetlb_reparenting_test.sh  |  0
 tools/testing/selftests/mm/madv_populate.c    | 18 +++--
 tools/testing/selftests/mm/migration.c        | 14 +++-
 tools/testing/selftests/mm/mrelease_test.c    |  1 +
 tools/testing/selftests/mm/run_vmtests.sh     | 23 ++++++
 tools/testing/selftests/mm/settings           |  2 +-
 tools/testing/selftests/mm/soft-dirty.c       |  3 +
 tools/testing/selftests/mm/test_hmm.sh        |  0
 tools/testing/selftests/mm/test_vmalloc.sh    |  0
 tools/testing/selftests/mm/thuge-gen.c        |  4 +-
 tools/testing/selftests/mm/transhuge-stress.c | 12 ++-
 .../selftests/mm/va_high_addr_switch.c        |  3 +-
 .../selftests/mm/va_high_addr_switch.sh       |  0
 tools/testing/selftests/mm/vm_util.c          | 17 ++++
 tools/testing/selftests/mm/vm_util.h          |  1 +
 .../selftests/mm/write_hugetlb_memory.sh      |  0
 20 files changed, 127 insertions(+), 55 deletions(-)
 mode change 100644 => 100755 tools/testing/selftests/mm/charge_reserved_hugetlb.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/check_config.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/hugetlb_reparenting_test.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/test_hmm.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/test_vmalloc.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/va_high_addr_switch.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/write_hugetlb_memory.sh

--
2.25.1

[PATCH v1 1/9] 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().

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/kselftest/runner.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index 1c952d1401d4..cb2b395ae296 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -105,8 +105,11 @@ 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"
 
-- 
2.25.1

[PATCH v1 2/9] selftests/mm: Give scripts execute permission

[Ryan Roberts]

When run under run_vmtests.sh, test scripts were failing to run with
"permission denied" due to the scripts not being executable.

It is also annoying not to be able to directly invoke run_vmtests.sh,
which is solved by giving also it the execute permission.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/mm/charge_reserved_hugetlb.sh  | 0
 tools/testing/selftests/mm/check_config.sh             | 0
 tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 0
 tools/testing/selftests/mm/run_vmtests.sh              | 0
 tools/testing/selftests/mm/test_hmm.sh                 | 0
 tools/testing/selftests/mm/test_vmalloc.sh             | 0
 tools/testing/selftests/mm/va_high_addr_switch.sh      | 0
 tools/testing/selftests/mm/write_hugetlb_memory.sh     | 0
 8 files changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 tools/testing/selftests/mm/charge_reserved_hugetlb.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/check_config.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/hugetlb_reparenting_test.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/test_hmm.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/test_vmalloc.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/va_high_addr_switch.sh
 mode change 100644 => 100755 tools/testing/selftests/mm/write_hugetlb_memory.sh

diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
old mode 100644
new mode 100755
diff --git a/tools/testing/selftests/mm/check_config.sh b/tools/testing/selftests/mm/check_config.sh
old mode 100644
new mode 100755
diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
old mode 100644
new mode 100755
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
old mode 100644
new mode 100755
diff --git a/tools/testing/selftests/mm/test_hmm.sh b/tools/testing/selftests/mm/test_hmm.sh
old mode 100644
new mode 100755
diff --git a/tools/testing/selftests/mm/test_vmalloc.sh b/tools/testing/selftests/mm/test_vmalloc.sh
old mode 100644
new mode 100755
diff --git a/tools/testing/selftests/mm/va_high_addr_switch.sh b/tools/testing/selftests/mm/va_high_addr_switch.sh
old mode 100644
new mode 100755
diff --git a/tools/testing/selftests/mm/write_hugetlb_memory.sh b/tools/testing/selftests/mm/write_hugetlb_memory.sh
old mode 100644
new mode 100755
-- 
2.25.1

[PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[Ryan Roberts]

arm64 does not support the soft-dirty PTE bit. However there are tests
in `madv_populate` and `soft-dirty` which assume it is supported and
cause spurious failures to be reported when preferred behaviour would be
to mark the tests as skipped.

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. This is wrapped up into a utility
function `system_has_softdirty()`, which is used to skip the whole
`soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate`
suite as skipped.

Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
 tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++-----
 tools/testing/selftests/mm/soft-dirty.c    |  3 +++
 tools/testing/selftests/mm/vm_util.c       | 17 +++++++++++++++++
 tools/testing/selftests/mm/vm_util.h       |  1 +
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c
index 60547245e479..5a8c176d7fec 100644
--- a/tools/testing/selftests/mm/madv_populate.c
+++ b/tools/testing/selftests/mm/madv_populate.c
@@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t size)
 	return ret;
 }

+#define ksft_test_result_if_softdirty(cond, ...)	\
+do {							\
+	if (system_has_softdirty())			\
+		ksft_test_result(cond, __VA_ARGS__);	\
+	else						\
+		ksft_test_result_skip(__VA_ARGS__);	\
+} while (0)
+
 static void test_softdirty(void)
 {
 	char *addr;
@@ -246,19 +254,19 @@ static void test_softdirty(void)

 	/* Clear any softdirty bits. */
 	clear_softdirty();
-	ksft_test_result(range_is_not_softdirty(addr, SIZE),
+	ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
 			 "range is not softdirty\n");

 	/* Populating READ should set softdirty. */
 	ret = madvise(addr, SIZE, MADV_POPULATE_READ);
-	ksft_test_result(!ret, "MADV_POPULATE_READ\n");
-	ksft_test_result(range_is_not_softdirty(addr, SIZE),
+	ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n");
+	ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
 			 "range is not softdirty\n");

 	/* Populating WRITE should set softdirty. */
 	ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
-	ksft_test_result(!ret, "MADV_POPULATE_WRITE\n");
-	ksft_test_result(range_is_softdirty(addr, SIZE),
+	ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n");
+	ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE),
 			 "range is softdirty\n");

 	munmap(addr, SIZE);
diff --git a/tools/testing/selftests/mm/soft-dirty.c b/tools/testing/selftests/mm/soft-dirty.c
index cc5f144430d4..8a2cd161ec4d 100644
--- a/tools/testing/selftests/mm/soft-dirty.c
+++ b/tools/testing/selftests/mm/soft-dirty.c
@@ -192,6 +192,9 @@ int main(int argc, char **argv)
 	int pagemap_fd;
 	int pagesize;

+	if (!system_has_softdirty())
+		return KSFT_SKIP;
+
 	ksft_print_header();
 	ksft_set_plan(15);

diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index 558c9cd8901c..f014fafbd2d3 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -53,6 +53,23 @@ unsigned long pagemap_get_pfn(int fd, char *start)
 	return -1ul;
 }

+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
+}
+
 void clear_softdirty(void)
 {
 	int ret;
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index c7fa61f0dff8..5a57314d428a 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -36,6 +36,7 @@ bool pagemap_is_softdirty(int fd, char *start);
 bool pagemap_is_swapped(int fd, char *start);
 bool pagemap_is_populated(int fd, char *start);
 unsigned long pagemap_get_pfn(int fd, char *start);
+int system_has_softdirty(void);
 void clear_softdirty(void);
 bool check_for_pattern(FILE *fp, const char *pattern, char *buf, size_t len);
 uint64_t read_pmd_pagesize(void);
--
2.25.1

[PATCH v1 4/9] 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>
---
 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 v1 5/9] 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

[PATCH v1 6/9] 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>
---
 tools/testing/selftests/mm/va_high_addr_switch.c | 3 ++-
 1 file changed, 2 insertions(+), 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..4b6f62c69a9d 100644
--- a/tools/testing/selftests/mm/va_high_addr_switch.c
+++ b/tools/testing/selftests/mm/va_high_addr_switch.c
@@ -292,7 +292,8 @@ static int supported_arch(void)
 #elif defined(__x86_64__)
 	return 1;
 #elif defined(__aarch64__)
-	return 1;
+	size_t page_size = getpagesize();
+	return page_size == PAGE_SIZE;
 #else
 	return 0;
 #endif
-- 
2.25.1

[PATCH v1 7/9] 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

[PATCH v1 8/9] 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

[PATCH v1 9/9] 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.

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 66d7c07dc177..881ed96d96fd 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -35,39 +35,39 @@ 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 += 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
+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 += soft-dirty
+TEST_GEN_FILES += split_huge_page_test
+TEST_GEN_FILES += ksm_tests
+TEST_GEN_FILES += ksm_functional_tests
+TEST_GEN_FILES += mdwe_test
 
 ifeq ($(ARCH),x86_64)
 CAN_BUILD_I386 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_32bit_program.c -m32)
@@ -83,24 +83,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
@@ -112,6 +112,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 3f26f6e15b2a..55fe1d309355 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
@@ -295,6 +306,18 @@ CATEGORY="soft_dirty" run_test ./soft-dirty
 # 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 v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[David Hildenbrand]

On 13.07.23 15:54, Ryan Roberts wrote:
> arm64 does not support the soft-dirty PTE bit. However there are tests
> in `madv_populate` and `soft-dirty` which assume it is supported and
> cause spurious failures to be reported when preferred behaviour would be
> to mark the tests as skipped.
> 
> 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. This is wrapped up into a utility
> function `system_has_softdirty()`, which is used to skip the whole
> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate`
> suite as skipped.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++-----
>   tools/testing/selftests/mm/soft-dirty.c    |  3 +++
>   tools/testing/selftests/mm/vm_util.c       | 17 +++++++++++++++++
>   tools/testing/selftests/mm/vm_util.h       |  1 +
>   4 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c
> index 60547245e479..5a8c176d7fec 100644
> --- a/tools/testing/selftests/mm/madv_populate.c
> +++ b/tools/testing/selftests/mm/madv_populate.c
> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t size)
>   	return ret;
>   }
> 
> +#define ksft_test_result_if_softdirty(cond, ...)	\
> +do {							\
> +	if (system_has_softdirty())			\
> +		ksft_test_result(cond, __VA_ARGS__);	\
> +	else						\
> +		ksft_test_result_skip(__VA_ARGS__);	\
> +} while (0)
> +
>   static void test_softdirty(void)
>   {
>   	char *addr;
> @@ -246,19 +254,19 @@ static void test_softdirty(void)
> 
>   	/* Clear any softdirty bits. */
>   	clear_softdirty();
> -	ksft_test_result(range_is_not_softdirty(addr, SIZE),
> +	ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>   			 "range is not softdirty\n");
> 
>   	/* Populating READ should set softdirty. */
>   	ret = madvise(addr, SIZE, MADV_POPULATE_READ);
> -	ksft_test_result(!ret, "MADV_POPULATE_READ\n");
> -	ksft_test_result(range_is_not_softdirty(addr, SIZE),
> +	ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n");
> +	ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>   			 "range is not softdirty\n");
> 
>   	/* Populating WRITE should set softdirty. */
>   	ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
> -	ksft_test_result(!ret, "MADV_POPULATE_WRITE\n");
> -	ksft_test_result(range_is_softdirty(addr, SIZE),
> +	ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n");
> +	ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE),
>   			 "range is softdirty\n");

We probably want to skip the whole test_*softdirty* test instead of 
adding this (IMHO suboptimal) ksft_test_result_if_softdirty.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[Ryan Roberts]

On 13/07/2023 14:56, David Hildenbrand wrote:
> On 13.07.23 15:54, Ryan Roberts wrote:
>> arm64 does not support the soft-dirty PTE bit. However there are tests
>> in `madv_populate` and `soft-dirty` which assume it is supported and
>> cause spurious failures to be reported when preferred behaviour would be
>> to mark the tests as skipped.
>>
>> 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. This is wrapped up into a utility
>> function `system_has_softdirty()`, which is used to skip the whole
>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate`
>> suite as skipped.
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>> ---
>>   tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++-----
>>   tools/testing/selftests/mm/soft-dirty.c    |  3 +++
>>   tools/testing/selftests/mm/vm_util.c       | 17 +++++++++++++++++
>>   tools/testing/selftests/mm/vm_util.h       |  1 +
>>   4 files changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/madv_populate.c
>> b/tools/testing/selftests/mm/madv_populate.c
>> index 60547245e479..5a8c176d7fec 100644
>> --- a/tools/testing/selftests/mm/madv_populate.c
>> +++ b/tools/testing/selftests/mm/madv_populate.c
>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t
>> size)
>>       return ret;
>>   }
>>
>> +#define ksft_test_result_if_softdirty(cond, ...)    \
>> +do {                            \
>> +    if (system_has_softdirty())            \
>> +        ksft_test_result(cond, __VA_ARGS__);    \
>> +    else                        \
>> +        ksft_test_result_skip(__VA_ARGS__);    \
>> +} while (0)
>> +
>>   static void test_softdirty(void)
>>   {
>>       char *addr;
>> @@ -246,19 +254,19 @@ static void test_softdirty(void)
>>
>>       /* Clear any softdirty bits. */
>>       clear_softdirty();
>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>                "range is not softdirty\n");
>>
>>       /* Populating READ should set softdirty. */
>>       ret = madvise(addr, SIZE, MADV_POPULATE_READ);
>> -    ksft_test_result(!ret, "MADV_POPULATE_READ\n");
>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n");
>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>                "range is not softdirty\n");
>>
>>       /* Populating WRITE should set softdirty. */
>>       ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
>> -    ksft_test_result(!ret, "MADV_POPULATE_WRITE\n");
>> -    ksft_test_result(range_is_softdirty(addr, SIZE),
>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n");
>> +    ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE),
>>                "range is softdirty\n");
> 
> We probably want to skip the whole test_*softdirty* test instead of adding this
> (IMHO suboptimal) ksft_test_result_if_softdirty.

Yeah I thought about doing it that way, but then the output just looks like
there were fewer tests and they all passed. But thinking about it now, I guess
the TAP header outputs the number of planned tests and the number of tests
executed are fewer, so a machine parser would still notice. I just don't like
that it outputs skipped:0.

But it a lightly held view. Happy to just do:

	if (system_has_softdirty())
		test_softdirty()

If you insist. ;-)

> 

Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[David Hildenbrand]

On 13.07.23 16:03, Ryan Roberts wrote:
> On 13/07/2023 14:56, David Hildenbrand wrote:
>> On 13.07.23 15:54, Ryan Roberts wrote:
>>> arm64 does not support the soft-dirty PTE bit. However there are tests
>>> in `madv_populate` and `soft-dirty` which assume it is supported and
>>> cause spurious failures to be reported when preferred behaviour would be
>>> to mark the tests as skipped.
>>>
>>> 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. This is wrapped up into a utility
>>> function `system_has_softdirty()`, which is used to skip the whole
>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate`
>>> suite as skipped.
>>>
>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>>    tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++-----
>>>    tools/testing/selftests/mm/soft-dirty.c    |  3 +++
>>>    tools/testing/selftests/mm/vm_util.c       | 17 +++++++++++++++++
>>>    tools/testing/selftests/mm/vm_util.h       |  1 +
>>>    4 files changed, 34 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/mm/madv_populate.c
>>> b/tools/testing/selftests/mm/madv_populate.c
>>> index 60547245e479..5a8c176d7fec 100644
>>> --- a/tools/testing/selftests/mm/madv_populate.c
>>> +++ b/tools/testing/selftests/mm/madv_populate.c
>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t
>>> size)
>>>        return ret;
>>>    }
>>>
>>> +#define ksft_test_result_if_softdirty(cond, ...)    \
>>> +do {                            \
>>> +    if (system_has_softdirty())            \
>>> +        ksft_test_result(cond, __VA_ARGS__);    \
>>> +    else                        \
>>> +        ksft_test_result_skip(__VA_ARGS__);    \
>>> +} while (0)
>>> +
>>>    static void test_softdirty(void)
>>>    {
>>>        char *addr;
>>> @@ -246,19 +254,19 @@ static void test_softdirty(void)
>>>
>>>        /* Clear any softdirty bits. */
>>>        clear_softdirty();
>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>                 "range is not softdirty\n");
>>>
>>>        /* Populating READ should set softdirty. */
>>>        ret = madvise(addr, SIZE, MADV_POPULATE_READ);
>>> -    ksft_test_result(!ret, "MADV_POPULATE_READ\n");
>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n");
>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>                 "range is not softdirty\n");
>>>
>>>        /* Populating WRITE should set softdirty. */
>>>        ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
>>> -    ksft_test_result(!ret, "MADV_POPULATE_WRITE\n");
>>> -    ksft_test_result(range_is_softdirty(addr, SIZE),
>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n");
>>> +    ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE),
>>>                 "range is softdirty\n");
>>
>> We probably want to skip the whole test_*softdirty* test instead of adding this
>> (IMHO suboptimal) ksft_test_result_if_softdirty.
> 
> Yeah I thought about doing it that way, but then the output just looks like
> there were fewer tests and they all passed. But thinking about it now, I guess
> the TAP header outputs the number of planned tests and the number of tests
> executed are fewer, so a machine parser would still notice. I just don't like
> that it outputs skipped:0.
> 
> But it a lightly held view. Happy to just do:
> 
> 	if (system_has_softdirty())
> 		test_softdirty()
> 
> If you insist. ;-)

diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c
index 60547245e479..33fda0337b32 100644
--- a/tools/testing/selftests/mm/madv_populate.c
+++ b/tools/testing/selftests/mm/madv_populate.c
@@ -266,12 +266,16 @@ static void test_softdirty(void)
  
  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 +283,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)


-- 
Cheers,

David / dhildenb

Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[David Hildenbrand]

On 13.07.23 16:09, David Hildenbrand wrote:
> On 13.07.23 16:03, Ryan Roberts wrote:
>> On 13/07/2023 14:56, David Hildenbrand wrote:
>>> On 13.07.23 15:54, Ryan Roberts wrote:
>>>> arm64 does not support the soft-dirty PTE bit. However there are tests
>>>> in `madv_populate` and `soft-dirty` which assume it is supported and
>>>> cause spurious failures to be reported when preferred behaviour would be
>>>> to mark the tests as skipped.
>>>>
>>>> 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. This is wrapped up into a utility
>>>> function `system_has_softdirty()`, which is used to skip the whole
>>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate`
>>>> suite as skipped.
>>>>
>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>> ---
>>>>     tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++-----
>>>>     tools/testing/selftests/mm/soft-dirty.c    |  3 +++
>>>>     tools/testing/selftests/mm/vm_util.c       | 17 +++++++++++++++++
>>>>     tools/testing/selftests/mm/vm_util.h       |  1 +
>>>>     4 files changed, 34 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/mm/madv_populate.c
>>>> b/tools/testing/selftests/mm/madv_populate.c
>>>> index 60547245e479..5a8c176d7fec 100644
>>>> --- a/tools/testing/selftests/mm/madv_populate.c
>>>> +++ b/tools/testing/selftests/mm/madv_populate.c
>>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t
>>>> size)
>>>>         return ret;
>>>>     }
>>>>
>>>> +#define ksft_test_result_if_softdirty(cond, ...)    \
>>>> +do {                            \
>>>> +    if (system_has_softdirty())            \
>>>> +        ksft_test_result(cond, __VA_ARGS__);    \
>>>> +    else                        \
>>>> +        ksft_test_result_skip(__VA_ARGS__);    \
>>>> +} while (0)
>>>> +
>>>>     static void test_softdirty(void)
>>>>     {
>>>>         char *addr;
>>>> @@ -246,19 +254,19 @@ static void test_softdirty(void)
>>>>
>>>>         /* Clear any softdirty bits. */
>>>>         clear_softdirty();
>>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>>                  "range is not softdirty\n");
>>>>
>>>>         /* Populating READ should set softdirty. */
>>>>         ret = madvise(addr, SIZE, MADV_POPULATE_READ);
>>>> -    ksft_test_result(!ret, "MADV_POPULATE_READ\n");
>>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n");
>>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>>                  "range is not softdirty\n");
>>>>
>>>>         /* Populating WRITE should set softdirty. */
>>>>         ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
>>>> -    ksft_test_result(!ret, "MADV_POPULATE_WRITE\n");
>>>> -    ksft_test_result(range_is_softdirty(addr, SIZE),
>>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n");
>>>> +    ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE),
>>>>                  "range is softdirty\n");
>>>
>>> We probably want to skip the whole test_*softdirty* test instead of adding this
>>> (IMHO suboptimal) ksft_test_result_if_softdirty.
>>
>> Yeah I thought about doing it that way, but then the output just looks like
>> there were fewer tests and they all passed. But thinking about it now, I guess
>> the TAP header outputs the number of planned tests and the number of tests
>> executed are fewer, so a machine parser would still notice. I just don't like
>> that it outputs skipped:0.
>>
>> But it a lightly held view. Happy to just do:
>>
>> 	if (system_has_softdirty())
>> 		test_softdirty()
>>
>> If you insist. ;-)
> 
> diff --git a/tools/testing/selftests/mm/madv_populate.c b/tools/testing/selftests/mm/madv_populate.c
> index 60547245e479..33fda0337b32 100644
> --- a/tools/testing/selftests/mm/madv_populate.c
> +++ b/tools/testing/selftests/mm/madv_populate.c
> @@ -266,12 +266,16 @@ static void test_softdirty(void)
>    
>    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 +283,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)
> 
> 

Oh, and if you want to have the skip, then you can think about 
converting test_softdirty() to only perform a single ksft_test_result(), 
and have a single skip on top.

All cleaner IMHO than ksft_test_result_if_softdirty ;)

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[Ryan Roberts]

On 13/07/2023 15:09, David Hildenbrand wrote:
> On 13.07.23 16:03, Ryan Roberts wrote:
>> On 13/07/2023 14:56, David Hildenbrand wrote:
>>> On 13.07.23 15:54, Ryan Roberts wrote:
>>>> arm64 does not support the soft-dirty PTE bit. However there are tests
>>>> in `madv_populate` and `soft-dirty` which assume it is supported and
>>>> cause spurious failures to be reported when preferred behaviour would be
>>>> to mark the tests as skipped.
>>>>
>>>> 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. This is wrapped up into a utility
>>>> function `system_has_softdirty()`, which is used to skip the whole
>>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate`
>>>> suite as skipped.
>>>>
>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>> ---
>>>>    tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++-----
>>>>    tools/testing/selftests/mm/soft-dirty.c    |  3 +++
>>>>    tools/testing/selftests/mm/vm_util.c       | 17 +++++++++++++++++
>>>>    tools/testing/selftests/mm/vm_util.h       |  1 +
>>>>    4 files changed, 34 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/mm/madv_populate.c
>>>> b/tools/testing/selftests/mm/madv_populate.c
>>>> index 60547245e479..5a8c176d7fec 100644
>>>> --- a/tools/testing/selftests/mm/madv_populate.c
>>>> +++ b/tools/testing/selftests/mm/madv_populate.c
>>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t
>>>> size)
>>>>        return ret;
>>>>    }
>>>>
>>>> +#define ksft_test_result_if_softdirty(cond, ...)    \
>>>> +do {                            \
>>>> +    if (system_has_softdirty())            \
>>>> +        ksft_test_result(cond, __VA_ARGS__);    \
>>>> +    else                        \
>>>> +        ksft_test_result_skip(__VA_ARGS__);    \
>>>> +} while (0)
>>>> +
>>>>    static void test_softdirty(void)
>>>>    {
>>>>        char *addr;
>>>> @@ -246,19 +254,19 @@ static void test_softdirty(void)
>>>>
>>>>        /* Clear any softdirty bits. */
>>>>        clear_softdirty();
>>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>>                 "range is not softdirty\n");
>>>>
>>>>        /* Populating READ should set softdirty. */
>>>>        ret = madvise(addr, SIZE, MADV_POPULATE_READ);
>>>> -    ksft_test_result(!ret, "MADV_POPULATE_READ\n");
>>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n");
>>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>>                 "range is not softdirty\n");
>>>>
>>>>        /* Populating WRITE should set softdirty. */
>>>>        ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
>>>> -    ksft_test_result(!ret, "MADV_POPULATE_WRITE\n");
>>>> -    ksft_test_result(range_is_softdirty(addr, SIZE),
>>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n");
>>>> +    ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE),
>>>>                 "range is softdirty\n");
>>>
>>> We probably want to skip the whole test_*softdirty* test instead of adding this
>>> (IMHO suboptimal) ksft_test_result_if_softdirty.
>>
>> Yeah I thought about doing it that way, but then the output just looks like
>> there were fewer tests and they all passed. But thinking about it now, I guess
>> the TAP header outputs the number of planned tests and the number of tests
>> executed are fewer, so a machine parser would still notice. I just don't like
>> that it outputs skipped:0.
>>
>> But it a lightly held view. Happy to just do:
>>
>>     if (system_has_softdirty())
>>         test_softdirty()
>>
>> If you insist. ;-)
> 
> diff --git a/tools/testing/selftests/mm/madv_populate.c
> b/tools/testing/selftests/mm/madv_populate.c
> index 60547245e479..33fda0337b32 100644
> --- a/tools/testing/selftests/mm/madv_populate.c
> +++ b/tools/testing/selftests/mm/madv_populate.c
> @@ -266,12 +266,16 @@ static void test_softdirty(void)
>  
>  int main(int argc, char **argv)
>  {
> +       int nr_tests = 16;
>         int err;
>  
>         pagesize = getpagesize();
>  
> +       if (system_has_softdirty())
> +               nr_tests += 5;

This is the opposite of the point I was trying to make; If there are 21 tests in
a suite, I'd like to know that there are 21 tests, 16 of which passed and 5 of
which were skipped. This will hide the 5 from the test report.

> +
>         ksft_print_header();
> -       ksft_set_plan(21);
> +       ksft_set_plan(nr_tests);
>  
>         sense_support();
>         test_prot_read();
> @@ -279,7 +283,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)
> 
> 

Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[Ryan Roberts]

On 13/07/2023 15:12, David Hildenbrand wrote:
> On 13.07.23 16:09, David Hildenbrand wrote:
>> On 13.07.23 16:03, Ryan Roberts wrote:
>>> On 13/07/2023 14:56, David Hildenbrand wrote:
>>>> On 13.07.23 15:54, Ryan Roberts wrote:
>>>>> arm64 does not support the soft-dirty PTE bit. However there are tests
>>>>> in `madv_populate` and `soft-dirty` which assume it is supported and
>>>>> cause spurious failures to be reported when preferred behaviour would be
>>>>> to mark the tests as skipped.
>>>>>
>>>>> 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. This is wrapped up into a utility
>>>>> function `system_has_softdirty()`, which is used to skip the whole
>>>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate`
>>>>> suite as skipped.
>>>>>
>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>> ---
>>>>>     tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++-----
>>>>>     tools/testing/selftests/mm/soft-dirty.c    |  3 +++
>>>>>     tools/testing/selftests/mm/vm_util.c       | 17 +++++++++++++++++
>>>>>     tools/testing/selftests/mm/vm_util.h       |  1 +
>>>>>     4 files changed, 34 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/mm/madv_populate.c
>>>>> b/tools/testing/selftests/mm/madv_populate.c
>>>>> index 60547245e479..5a8c176d7fec 100644
>>>>> --- a/tools/testing/selftests/mm/madv_populate.c
>>>>> +++ b/tools/testing/selftests/mm/madv_populate.c
>>>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t
>>>>> size)
>>>>>         return ret;
>>>>>     }
>>>>>
>>>>> +#define ksft_test_result_if_softdirty(cond, ...)    \
>>>>> +do {                            \
>>>>> +    if (system_has_softdirty())            \
>>>>> +        ksft_test_result(cond, __VA_ARGS__);    \
>>>>> +    else                        \
>>>>> +        ksft_test_result_skip(__VA_ARGS__);    \
>>>>> +} while (0)
>>>>> +
>>>>>     static void test_softdirty(void)
>>>>>     {
>>>>>         char *addr;
>>>>> @@ -246,19 +254,19 @@ static void test_softdirty(void)
>>>>>
>>>>>         /* Clear any softdirty bits. */
>>>>>         clear_softdirty();
>>>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>>>                  "range is not softdirty\n");
>>>>>
>>>>>         /* Populating READ should set softdirty. */
>>>>>         ret = madvise(addr, SIZE, MADV_POPULATE_READ);
>>>>> -    ksft_test_result(!ret, "MADV_POPULATE_READ\n");
>>>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n");
>>>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>>>                  "range is not softdirty\n");
>>>>>
>>>>>         /* Populating WRITE should set softdirty. */
>>>>>         ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
>>>>> -    ksft_test_result(!ret, "MADV_POPULATE_WRITE\n");
>>>>> -    ksft_test_result(range_is_softdirty(addr, SIZE),
>>>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n");
>>>>> +    ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE),
>>>>>                  "range is softdirty\n");
>>>>
>>>> We probably want to skip the whole test_*softdirty* test instead of adding this
>>>> (IMHO suboptimal) ksft_test_result_if_softdirty.
>>>
>>> Yeah I thought about doing it that way, but then the output just looks like
>>> there were fewer tests and they all passed. But thinking about it now, I guess
>>> the TAP header outputs the number of planned tests and the number of tests
>>> executed are fewer, so a machine parser would still notice. I just don't like
>>> that it outputs skipped:0.
>>>
>>> But it a lightly held view. Happy to just do:
>>>
>>>     if (system_has_softdirty())
>>>         test_softdirty()
>>>
>>> If you insist. ;-)
>>
>> diff --git a/tools/testing/selftests/mm/madv_populate.c
>> b/tools/testing/selftests/mm/madv_populate.c
>> index 60547245e479..33fda0337b32 100644
>> --- a/tools/testing/selftests/mm/madv_populate.c
>> +++ b/tools/testing/selftests/mm/madv_populate.c
>> @@ -266,12 +266,16 @@ static void test_softdirty(void)
>>       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 +283,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)
>>
>>
> 
> Oh, and if you want to have the skip, then you can think about converting
> test_softdirty() to only perform a single ksft_test_result(), and have a single
> skip on top.
> 
> All cleaner IMHO than ksft_test_result_if_softdirty ;)

I'll do it the way you recommend above. Like I said, its a lightly held opinion,
and your way looks like less effort. ;-)


> 

Re: [PATCH v1 1/9] selftests: Line buffer test program's stdout

[Mark Brown]

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

On Thu, Jul 13, 2023 at 02:54:32PM +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
> 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().

This is going to be useful in general since not all selftests use the
kselftest helpers but it'd probably also be good to make
ksft_print_header() also make the output unbuffered so that if setbuf
isn't installed on the target system or the tests are run standalone we
don't run into issues there.  Even if the test isn't corrupting data
having things unbuffered is going to be good for making sure we don't
drop any output if the test dies.

> +		if [ -x /usr/bin/stdbuf ]; then
> +			stdbuf="/usr/bin/stdbuf --output=L "
> +		fi

Might be more robust to use type -p to find stdbuf in case it's in /bin
or something?

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

Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[David Hildenbrand]

On 13.07.23 16:14, Ryan Roberts wrote:
> On 13/07/2023 15:09, David Hildenbrand wrote:
>> On 13.07.23 16:03, Ryan Roberts wrote:
>>> On 13/07/2023 14:56, David Hildenbrand wrote:
>>>> On 13.07.23 15:54, Ryan Roberts wrote:
>>>>> arm64 does not support the soft-dirty PTE bit. However there are tests
>>>>> in `madv_populate` and `soft-dirty` which assume it is supported and
>>>>> cause spurious failures to be reported when preferred behaviour would be
>>>>> to mark the tests as skipped.
>>>>>
>>>>> 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. This is wrapped up into a utility
>>>>> function `system_has_softdirty()`, which is used to skip the whole
>>>>> `soft-dirty` suite, and mark the soft-dirty tests in the `madv_populate`
>>>>> suite as skipped.
>>>>>
>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>> ---
>>>>>     tools/testing/selftests/mm/madv_populate.c | 18 +++++++++++++-----
>>>>>     tools/testing/selftests/mm/soft-dirty.c    |  3 +++
>>>>>     tools/testing/selftests/mm/vm_util.c       | 17 +++++++++++++++++
>>>>>     tools/testing/selftests/mm/vm_util.h       |  1 +
>>>>>     4 files changed, 34 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/tools/testing/selftests/mm/madv_populate.c
>>>>> b/tools/testing/selftests/mm/madv_populate.c
>>>>> index 60547245e479..5a8c176d7fec 100644
>>>>> --- a/tools/testing/selftests/mm/madv_populate.c
>>>>> +++ b/tools/testing/selftests/mm/madv_populate.c
>>>>> @@ -232,6 +232,14 @@ static bool range_is_not_softdirty(char *start, ssize_t
>>>>> size)
>>>>>         return ret;
>>>>>     }
>>>>>
>>>>> +#define ksft_test_result_if_softdirty(cond, ...)    \
>>>>> +do {                            \
>>>>> +    if (system_has_softdirty())            \
>>>>> +        ksft_test_result(cond, __VA_ARGS__);    \
>>>>> +    else                        \
>>>>> +        ksft_test_result_skip(__VA_ARGS__);    \
>>>>> +} while (0)
>>>>> +
>>>>>     static void test_softdirty(void)
>>>>>     {
>>>>>         char *addr;
>>>>> @@ -246,19 +254,19 @@ static void test_softdirty(void)
>>>>>
>>>>>         /* Clear any softdirty bits. */
>>>>>         clear_softdirty();
>>>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>>>                  "range is not softdirty\n");
>>>>>
>>>>>         /* Populating READ should set softdirty. */
>>>>>         ret = madvise(addr, SIZE, MADV_POPULATE_READ);
>>>>> -    ksft_test_result(!ret, "MADV_POPULATE_READ\n");
>>>>> -    ksft_test_result(range_is_not_softdirty(addr, SIZE),
>>>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_READ\n");
>>>>> +    ksft_test_result_if_softdirty(range_is_not_softdirty(addr, SIZE),
>>>>>                  "range is not softdirty\n");
>>>>>
>>>>>         /* Populating WRITE should set softdirty. */
>>>>>         ret = madvise(addr, SIZE, MADV_POPULATE_WRITE);
>>>>> -    ksft_test_result(!ret, "MADV_POPULATE_WRITE\n");
>>>>> -    ksft_test_result(range_is_softdirty(addr, SIZE),
>>>>> +    ksft_test_result_if_softdirty(!ret, "MADV_POPULATE_WRITE\n");
>>>>> +    ksft_test_result_if_softdirty(range_is_softdirty(addr, SIZE),
>>>>>                  "range is softdirty\n");
>>>>
>>>> We probably want to skip the whole test_*softdirty* test instead of adding this
>>>> (IMHO suboptimal) ksft_test_result_if_softdirty.
>>>
>>> Yeah I thought about doing it that way, but then the output just looks like
>>> there were fewer tests and they all passed. But thinking about it now, I guess
>>> the TAP header outputs the number of planned tests and the number of tests
>>> executed are fewer, so a machine parser would still notice. I just don't like
>>> that it outputs skipped:0.
>>>
>>> But it a lightly held view. Happy to just do:
>>>
>>>      if (system_has_softdirty())
>>>          test_softdirty()
>>>
>>> If you insist. ;-)
>>
>> diff --git a/tools/testing/selftests/mm/madv_populate.c
>> b/tools/testing/selftests/mm/madv_populate.c
>> index 60547245e479..33fda0337b32 100644
>> --- a/tools/testing/selftests/mm/madv_populate.c
>> +++ b/tools/testing/selftests/mm/madv_populate.c
>> @@ -266,12 +266,16 @@ static void test_softdirty(void)
>>   
>>   int main(int argc, char **argv)
>>   {
>> +       int nr_tests = 16;
>>          int err;
>>   
>>          pagesize = getpagesize();
>>   
>> +       if (system_has_softdirty())
>> +               nr_tests += 5;
> 
> This is the opposite of the point I was trying to make; If there are 21 tests in
> a suite, I'd like to know that there are 21 tests, 16 of which passed and 5 of
> which were skipped. This will hide the 5 from the test report.

Well, these test are impossible on that architecture, which is IMHO 
different to some kind of "impossible in the configuration" like "no 
swap", "no hugetlb", "no THP available".

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/9] selftests: Line buffer test program's stdout

[Ryan Roberts]

On 13/07/2023 15:16, Mark Brown wrote:
> On Thu, Jul 13, 2023 at 02:54:32PM +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
>> 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().
> 
> This is going to be useful in general since not all selftests use the
> kselftest helpers but it'd probably also be good to make
> ksft_print_header() also make the output unbuffered 

Yeah sounds reasonable.

> so that if setbuf
> isn't installed on the target system or the tests are run standalone we
> don't run into issues there.  Even if the test isn't corrupting data
> having things unbuffered is going to be good for making sure we don't
> drop any output if the test dies.

Note that currently I've set stdbuf to encourage line buffering rather than no
buffering. Are you saying no buffering is preferred? I took the view that line
buffering is a good middle ground, and and aligns with what people see when
developing and running the program manually in the terminal.

> 
>> +		if [ -x /usr/bin/stdbuf ]; then
>> +			stdbuf="/usr/bin/stdbuf --output=L "
>> +		fi
> 
> Might be more robust to use type -p to find stdbuf in case it's in /bin
> or something?

Yep good idea.

Re: [PATCH v1 4/9] selftests/mm: Enable mrelease_test for arm64

[David Hildenbrand]

On 13.07.23 15:54, Ryan Roberts wrote:
> 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>
> ---
>   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>

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

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission

[David Hildenbrand]

On 13.07.23 15:54, Ryan Roberts wrote:
> When run under run_vmtests.sh, test scripts were failing to run with
> "permission denied" due to the scripts not being executable.
> 
> It is also annoying not to be able to directly invoke run_vmtests.sh,
> which is solved by giving also it the execute permission.
> 
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>   tools/testing/selftests/mm/charge_reserved_hugetlb.sh  | 0
>   tools/testing/selftests/mm/check_config.sh             | 0
>   tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 0
>   tools/testing/selftests/mm/run_vmtests.sh              | 0
>   tools/testing/selftests/mm/test_hmm.sh                 | 0
>   tools/testing/selftests/mm/test_vmalloc.sh             | 0
>   tools/testing/selftests/mm/va_high_addr_switch.sh      | 0
>   tools/testing/selftests/mm/write_hugetlb_memory.sh     | 0
>   8 files changed, 0 insertions(+), 0 deletions(-)
>   mode change 100644 => 100755 tools/testing/selftests/mm/charge_reserved_hugetlb.sh
>   mode change 100644 => 100755 tools/testing/selftests/mm/check_config.sh
>   mode change 100644 => 100755 tools/testing/selftests/mm/hugetlb_reparenting_test.sh
>   mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
>   mode change 100644 => 100755 tools/testing/selftests/mm/test_hmm.sh
>   mode change 100644 => 100755 tools/testing/selftests/mm/test_vmalloc.sh
>   mode change 100644 => 100755 tools/testing/selftests/mm/va_high_addr_switch.sh
>   mode change 100644 => 100755 tools/testing/selftests/mm/write_hugetlb_memory.sh
> 
> diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> old mode 100644
> new mode 100755
> diff --git a/tools/testing/selftests/mm/check_config.sh b/tools/testing/selftests/mm/check_config.sh
> old mode 100644
> new mode 100755
> diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> old mode 100644
> new mode 100755
> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> old mode 100644
> new mode 100755
> diff --git a/tools/testing/selftests/mm/test_hmm.sh b/tools/testing/selftests/mm/test_hmm.sh
> old mode 100644
> new mode 100755
> diff --git a/tools/testing/selftests/mm/test_vmalloc.sh b/tools/testing/selftests/mm/test_vmalloc.sh
> old mode 100644
> new mode 100755
> diff --git a/tools/testing/selftests/mm/va_high_addr_switch.sh b/tools/testing/selftests/mm/va_high_addr_switch.sh
> old mode 100644
> new mode 100755
> diff --git a/tools/testing/selftests/mm/write_hugetlb_memory.sh b/tools/testing/selftests/mm/write_hugetlb_memory.sh
> old mode 100644
> new mode 100755

Sounds reasonable to me.

Probably due to:

commit baa489fabd01596d5426d6e112b34ba5fb59ab82
Author: SeongJae Park <sj@kernel.org>
Date:   Tue Jan 3 18:07:53 2023 +0000

     selftests/vm: rename selftests/vm to selftests/mm
     
     Rename selftets/vm to selftests/mm for being more consistent with the
     code, documentation, and tools directories, and won't be confused with
     virtual machines.


and indeed, it contains

diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
old mode 100755
new mode 100644
similarity index 100%
rename from tools/testing/selftests/vm/run_vmtests.sh
rename to tools/testing/selftests/mm/run_vmtests.sh


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

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 6/9] selftests/mm: va_high_addr_switch should skip unsupported arm64 configs

[David Hildenbrand]

On 13.07.23 15:54, Ryan Roberts wrote:
> 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>
> ---
>   tools/testing/selftests/mm/va_high_addr_switch.c | 3 ++-
>   1 file changed, 2 insertions(+), 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..4b6f62c69a9d 100644
> --- a/tools/testing/selftests/mm/va_high_addr_switch.c
> +++ b/tools/testing/selftests/mm/va_high_addr_switch.c
> @@ -292,7 +292,8 @@ static int supported_arch(void)
>   #elif defined(__x86_64__)
>   	return 1;
>   #elif defined(__aarch64__)
> -	return 1;
> +	size_t page_size = getpagesize();
> +	return page_size == PAGE_SIZE;


return getpagesize() == PAGE_SIZE;

?


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

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 1/9] selftests: Line buffer test program's stdout

[Mark Brown]

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

On Thu, Jul 13, 2023 at 03:32:19PM +0100, Ryan Roberts wrote:
> On 13/07/2023 15:16, Mark Brown wrote:

> > so that if setbuf
> > isn't installed on the target system or the tests are run standalone we
> > don't run into issues there.  Even if the test isn't corrupting data
> > having things unbuffered is going to be good for making sure we don't
> > drop any output if the test dies.

> Note that currently I've set stdbuf to encourage line buffering rather than no
> buffering. Are you saying no buffering is preferred? I took the view that line
> buffering is a good middle ground, and and aligns with what people see when
> developing and running the program manually in the terminal.

TBH with the way KTAP is specified line buffered and unbuffered are
probably equivalent, I was just defaulting to unbuffered since it's the
more conservative (if less performant for lots of I/O) option.

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

Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh

[David Hildenbrand]

On 13.07.23 15:54, 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
> 

Which run_kselftest.sh are you referring to, the one in the parent directory?

How to invoke it to run these mm tests?

(I never dared invoking something different than
run_vmtests.sh ;) )

[...]

> 
> 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 66d7c07dc177..881ed96d96fd 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -35,39 +35,39 @@ 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 += 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
> +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 += soft-dirty
> +TEST_GEN_FILES += split_huge_page_test
> +TEST_GEN_FILES += ksm_tests
> +TEST_GEN_FILES += ksm_functional_tests
> +TEST_GEN_FILES += mdwe_test

IIRC, we recently converted all to TEST_GEN_PROGS. See

commit aef6fde75d8c6c1cad4a0e017a8d4cbee2143723
Author: Peter Xu <peterx@redhat.com>
Date:   Wed Apr 12 12:42:18 2023 -0400

     selftests/mm: use TEST_GEN_PROGS where proper
     
     TEST_GEN_PROGS and TEST_GEN_FILES are used randomly in the mm/Makefile to
     specify programs that need to build.  Logically all these binaries should
     all fall into TEST_GEN_PROGS.
     
     Replace those TEST_GEN_FILES with TEST_GEN_PROGS, so that we can reference
     all the tests easily later.


Why is that change required, and how does it interact with
run_kselftest.sh? (Not clear from you patch description.)

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh

[Ryan Roberts]

On 13/07/2023 15:50, David Hildenbrand wrote:
> On 13.07.23 15:54, 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
>>
> 
> Which run_kselftest.sh are you referring to, the one in the parent directory?

run_kselftest.sh is the uniform way of executing all the kselftests. mm seems to
be trying to be special as far as I can see. Certainly if you run the `install`
make target, kselftests will create a list of all the tests (including non-mm
tests if you have included them in the TARGETS variable) and copy that test list
and run_kselftest.sh to the install path along with all the test binaries. Then
the user can invoke any of the collections or specific tests in the collections
using that tool. It also wraps everything with tap output, runs tests with a
timeout, etc.

See Documentation/dev-tools/kselftest.rst

> 
> How to invoke it to run these mm tests?
> 
> (I never dared invoking something different than
> run_vmtests.sh ;) )

# single test:
$ sudo ./run_kselftest.sh -t mm:<test_name>

or

# all tests in collection:
$ sudo ./run_kselftest.sh -c mm

> 
> [...]
> 
>>
>> 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 66d7c07dc177..881ed96d96fd 100644
>> --- a/tools/testing/selftests/mm/Makefile
>> +++ b/tools/testing/selftests/mm/Makefile
>> @@ -35,39 +35,39 @@ 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 += 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
>> +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 += soft-dirty
>> +TEST_GEN_FILES += split_huge_page_test
>> +TEST_GEN_FILES += ksm_tests
>> +TEST_GEN_FILES += ksm_functional_tests
>> +TEST_GEN_FILES += mdwe_test
> 
> IIRC, we recently converted all to TEST_GEN_PROGS. See
> 
> commit aef6fde75d8c6c1cad4a0e017a8d4cbee2143723
> Author: Peter Xu <peterx@redhat.com>
> Date:   Wed Apr 12 12:42:18 2023 -0400
> 
>     selftests/mm: use TEST_GEN_PROGS where proper
>         TEST_GEN_PROGS and TEST_GEN_FILES are used randomly in the mm/Makefile to
>     specify programs that need to build.  Logically all these binaries should
>     all fall into TEST_GEN_PROGS.
>         Replace those TEST_GEN_FILES with TEST_GEN_PROGS, so that we can reference
>     all the tests easily later.
> 
> 
> Why is that change required, and how does it interact with
> run_kselftest.sh? (Not clear from you patch description.)

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.

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).

So with this change at the kselftest level, there is a single test in its list;
run_vmtests.sh. And all the other tests that were previously in that list are
moved into run_vmtests.sh (if they weren't there already).

I've only learnt all this today; All based on info in kselftest.rst.

> 

Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh

[David Hildenbrand]

>>
>> Which run_kselftest.sh are you referring to, the one in the parent directory?
> 
> run_kselftest.sh is the uniform way of executing all the kselftests. mm seems to
> be trying to be special as far as I can see. Certainly if you run the `install`
> make target, kselftests will create a list of all the tests (including non-mm
> tests if you have included them in the TARGETS variable) and copy that test list
> and run_kselftest.sh to the install path along with all the test binaries. Then
> the user can invoke any of the collections or specific tests in the collections
> using that tool. It also wraps everything with tap output, runs tests with a
> timeout, etc.
> 
> See Documentation/dev-tools/kselftest.rst
> 

Got it, thanks!

>>
>> How to invoke it to run these mm tests?
>>
>> (I never dared invoking something different than
>> run_vmtests.sh ;) )
> 
> # single test:
> $ sudo ./run_kselftest.sh -t mm:<test_name>
> 
> or
> 
> # all tests in collection:
> $ sudo ./run_kselftest.sh -c mm
> 

Ah, that makes sense. So I guess mm is then one "collection".

>>
>> [...]
>>
>>>
>>> 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 66d7c07dc177..881ed96d96fd 100644
>>> --- a/tools/testing/selftests/mm/Makefile
>>> +++ b/tools/testing/selftests/mm/Makefile
>>> @@ -35,39 +35,39 @@ 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 += 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
>>> +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 += soft-dirty
>>> +TEST_GEN_FILES += split_huge_page_test
>>> +TEST_GEN_FILES += ksm_tests
>>> +TEST_GEN_FILES += ksm_functional_tests
>>> +TEST_GEN_FILES += mdwe_test
>>
>> IIRC, we recently converted all to TEST_GEN_PROGS. See
>>
>> commit aef6fde75d8c6c1cad4a0e017a8d4cbee2143723
>> Author: Peter Xu <peterx@redhat.com>
>> Date:   Wed Apr 12 12:42:18 2023 -0400
>>
>>      selftests/mm: use TEST_GEN_PROGS where proper
>>          TEST_GEN_PROGS and TEST_GEN_FILES are used randomly in the mm/Makefile to
>>      specify programs that need to build.  Logically all these binaries should
>>      all fall into TEST_GEN_PROGS.
>>          Replace those TEST_GEN_FILES with TEST_GEN_PROGS, so that we can reference
>>      all the tests easily later.
>>
>>
>> Why is that change required, and how does it interact with
>> run_kselftest.sh? (Not clear from you patch description.)
> 
> 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.
> 
> 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).
> 
> So with this change at the kselftest level, there is a single test in its list;
> run_vmtests.sh. And all the other tests that were previously in that list are
> moved into run_vmtests.sh (if they weren't there already).

That sound good to me. (worth adding to the patch description)

Let me CC Peter, so he's aware.

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh

[Ryan Roberts]

On 13/07/2023 16:25, David Hildenbrand wrote:
>>>
>>> Which run_kselftest.sh are you referring to, the one in the parent directory?
>>
>> run_kselftest.sh is the uniform way of executing all the kselftests. mm seems to
>> be trying to be special as far as I can see. Certainly if you run the `install`
>> make target, kselftests will create a list of all the tests (including non-mm
>> tests if you have included them in the TARGETS variable) and copy that test list
>> and run_kselftest.sh to the install path along with all the test binaries. Then
>> the user can invoke any of the collections or specific tests in the collections
>> using that tool. It also wraps everything with tap output, runs tests with a
>> timeout, etc.
>>
>> See Documentation/dev-tools/kselftest.rst
>>
> 
> Got it, thanks!
> 
>>>
>>> How to invoke it to run these mm tests?
>>>
>>> (I never dared invoking something different than
>>> run_vmtests.sh ;) )
>>
>> # single test:
>> $ sudo ./run_kselftest.sh -t mm:<test_name>
>>
>> or
>>
>> # all tests in collection:
>> $ sudo ./run_kselftest.sh -c mm
>>
> 
> Ah, that makes sense. So I guess mm is then one "collection".

yep, the collections are the directories under tools/testing/selftests with a
few special exceptions.

> 
>>>
>>> [...]
>>>
>>>>
>>>> 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 66d7c07dc177..881ed96d96fd 100644
>>>> --- a/tools/testing/selftests/mm/Makefile
>>>> +++ b/tools/testing/selftests/mm/Makefile
>>>> @@ -35,39 +35,39 @@ 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 += 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
>>>> +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 += soft-dirty
>>>> +TEST_GEN_FILES += split_huge_page_test
>>>> +TEST_GEN_FILES += ksm_tests
>>>> +TEST_GEN_FILES += ksm_functional_tests
>>>> +TEST_GEN_FILES += mdwe_test
>>>
>>> IIRC, we recently converted all to TEST_GEN_PROGS. See
>>>
>>> commit aef6fde75d8c6c1cad4a0e017a8d4cbee2143723
>>> Author: Peter Xu <peterx@redhat.com>
>>> Date:   Wed Apr 12 12:42:18 2023 -0400
>>>
>>>      selftests/mm: use TEST_GEN_PROGS where proper
>>>          TEST_GEN_PROGS and TEST_GEN_FILES are used randomly in the
>>> mm/Makefile to
>>>      specify programs that need to build.  Logically all these binaries should
>>>      all fall into TEST_GEN_PROGS.
>>>          Replace those TEST_GEN_FILES with TEST_GEN_PROGS, so that we can
>>> reference
>>>      all the tests easily later.
>>>
>>>
>>> Why is that change required, and how does it interact with
>>> run_kselftest.sh? (Not clear from you patch description.)
>>
>> 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.
>>
>> 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).
>>
>> So with this change at the kselftest level, there is a single test in its list;
>> run_vmtests.sh. And all the other tests that were previously in that list are
>> moved into run_vmtests.sh (if they weren't there already).
> 
> That sound good to me. (worth adding to the patch description)
> 
> Let me CC Peter, so he's aware.

Thanks - would be good to hear his opinion!

Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh

[Mark Brown]

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

On Thu, Jul 13, 2023 at 04:04:44PM +0100, Ryan Roberts wrote:

> So with this change at the kselftest level, there is a single test in its list;
> run_vmtests.sh. And all the other tests that were previously in that list are
> moved into run_vmtests.sh (if they weren't there already).

The results parsers I'm aware of like the LAVA one will DTRT with nested
kselftests since that's required to pull see individual test cases run
by a single binary so it's the common case to see at least one level of
nesting.

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

Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh

[Ryan Roberts]

On 13/07/2023 16:30, Mark Brown wrote:
> On Thu, Jul 13, 2023 at 04:04:44PM +0100, Ryan Roberts wrote:
> 
>> So with this change at the kselftest level, there is a single test in its list;
>> run_vmtests.sh. And all the other tests that were previously in that list are
>> moved into run_vmtests.sh (if they weren't there already).
> 
> The results parsers I'm aware of like the LAVA one will DTRT with nested
> kselftests since that's required to pull see individual test cases run
> by a single binary so it's the common case to see at least one level of
> nesting.

That's good to hear. But bear in mind that run_vmtests.sh does not use TAP. So
you end up with a single top-level test who's result is reported with
run_kselftest.sh's TAP output. Then you have a second level (run_vmtests.sh)
using custom reporting, then _some_ of the tests invoked use TAP so you
sometimes have TAP at level 3. But those tests at level 2 that don't do their
own TAP output probably won't be parsed by LAVA?

Since you agreed to put this into the CI, I was going to call this part "your
problem" ;-)

Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh

[Mark Brown]

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

On Thu, Jul 13, 2023 at 04:36:18PM +0100, Ryan Roberts wrote:
> On 13/07/2023 16:30, Mark Brown wrote:

> > The results parsers I'm aware of like the LAVA one will DTRT with nested
> > kselftests since that's required to pull see individual test cases run
> > by a single binary so it's the common case to see at least one level of
> > nesting.

> That's good to hear. But bear in mind that run_vmtests.sh does not use TAP. So
> you end up with a single top-level test who's result is reported with
> run_kselftest.sh's TAP output. Then you have a second level (run_vmtests.sh)
> using custom reporting, then _some_ of the tests invoked use TAP so you
> sometimes have TAP at level 3. But those tests at level 2 that don't do their
> own TAP output probably won't be parsed by LAVA?

I think that should mostly mean that all the tests that don't
individually produce KTAP output get ignored by parsers and those which
do produce KTAP output will be seen as nesting one level up from where
they are (ie, the individual cases will run directly from vmtest),
though there's likely to be confusion about expected run numbers for
things that actually pay attention to that.

> Since you agreed to put this into the CI, I was going to call this part "your
> problem" ;-)

It'll run, the results are a different story. :P

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

Re: [PATCH v1 9/9] selftests/mm: Run all tests from run_vmtests.sh

[Ryan Roberts]

On 13/07/2023 16:43, Mark Brown wrote:
> On Thu, Jul 13, 2023 at 04:36:18PM +0100, Ryan Roberts wrote:
>> On 13/07/2023 16:30, Mark Brown wrote:
> 
>>> The results parsers I'm aware of like the LAVA one will DTRT with nested
>>> kselftests since that's required to pull see individual test cases run
>>> by a single binary so it's the common case to see at least one level of
>>> nesting.
> 
>> That's good to hear. But bear in mind that run_vmtests.sh does not use TAP. So
>> you end up with a single top-level test who's result is reported with
>> run_kselftest.sh's TAP output. Then you have a second level (run_vmtests.sh)
>> using custom reporting, then _some_ of the tests invoked use TAP so you
>> sometimes have TAP at level 3. But those tests at level 2 that don't do their
>> own TAP output probably won't be parsed by LAVA?
> 
> I think that should mostly mean that all the tests that don't
> individually produce KTAP output get ignored by parsers and those which
> do produce KTAP output will be seen as nesting one level up from where
> they are (ie, the individual cases will run directly from vmtest),
> though there's likely to be confusion about expected run numbers for
> things that actually pay attention to that.

I suspect it wouldn't be technically dififcult to add a --tap option to
run_vmtests.sh, which would switch the output format to TAP. If people are amenable.

> 
>> Since you agreed to put this into the CI, I was going to call this part "your
>> problem" ;-)
> 
> It'll run, the results are a different story. :P

Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission

[SeongJae Park]

On Thu, 13 Jul 2023 16:39:33 +0200 David Hildenbrand <david@redhat.com> wrote:

> On 13.07.23 15:54, Ryan Roberts wrote:
> > When run under run_vmtests.sh, test scripts were failing to run with
> > "permission denied" due to the scripts not being executable.
> > 
> > It is also annoying not to be able to directly invoke run_vmtests.sh,
> > which is solved by giving also it the execute permission.
> > 
> > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> > ---
> >   tools/testing/selftests/mm/charge_reserved_hugetlb.sh  | 0
> >   tools/testing/selftests/mm/check_config.sh             | 0
> >   tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 0
> >   tools/testing/selftests/mm/run_vmtests.sh              | 0
> >   tools/testing/selftests/mm/test_hmm.sh                 | 0
> >   tools/testing/selftests/mm/test_vmalloc.sh             | 0
> >   tools/testing/selftests/mm/va_high_addr_switch.sh      | 0
> >   tools/testing/selftests/mm/write_hugetlb_memory.sh     | 0
> >   8 files changed, 0 insertions(+), 0 deletions(-)
> >   mode change 100644 => 100755 tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/check_config.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/test_hmm.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/test_vmalloc.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/va_high_addr_switch.sh
> >   mode change 100644 => 100755 tools/testing/selftests/mm/write_hugetlb_memory.sh
> > 
> > diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/check_config.sh b/tools/testing/selftests/mm/check_config.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/test_hmm.sh b/tools/testing/selftests/mm/test_hmm.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/test_vmalloc.sh b/tools/testing/selftests/mm/test_vmalloc.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/va_high_addr_switch.sh b/tools/testing/selftests/mm/va_high_addr_switch.sh
> > old mode 100644
> > new mode 100755
> > diff --git a/tools/testing/selftests/mm/write_hugetlb_memory.sh b/tools/testing/selftests/mm/write_hugetlb_memory.sh
> > old mode 100644
> > new mode 100755
> 
> Sounds reasonable to me.
> 
> Probably due to:
> 
> commit baa489fabd01596d5426d6e112b34ba5fb59ab82
> Author: SeongJae Park <sj@kernel.org>
> Date:   Tue Jan 3 18:07:53 2023 +0000
> 
>      selftests/vm: rename selftests/vm to selftests/mm
>      
>      Rename selftets/vm to selftests/mm for being more consistent with the
>      code, documentation, and tools directories, and won't be confused with
>      virtual machines.
> 
> 
> and indeed, it contains
> 
> diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> old mode 100755
> new mode 100644
> similarity index 100%
> rename from tools/testing/selftests/vm/run_vmtests.sh
> rename to tools/testing/selftests/mm/run_vmtests.sh

Thank you for tracking this and kindly Cc-ing me!  I'd like to clarify a little
bit more, though.  The permission change has made by the commit as you found.
Nevertheless, the submitted version[1] of the patch didn't change the
permission.  I guess the change was made while managing it via some file
permission unsupported patches management tool.

I had a similar issue with DAMON selftest and sent a patch restoring the
permission.  Greg suggested me to update the framework instead, to support such
management tool[2], so I made it[3].  It recently also merged into 5.15.y for
DAMON selftests[4].

I have no strong opinion about whether we need to keep the permission or it's
good to have no execute permission since kselftest framework supports it.  I
just wanted to clarify the events I've shown.  Please correct me if I missed or
wrong something.  Cc-ing Greg, since he might have an opinion.

[1] https://lore.kernel.org/all/20230103180754.129637-5-sj@kernel.org/
[2] https://lore.kernel.org/mm-commits/YRJisBs9AunccCD4@kroah.com/
[3] https://lore.kernel.org/all/20210810164534.25902-1-sj38.park@gmail.com/
[4] https://lore.kernel.org/stable/2023042743-cheesy-parasitic-206d@gregkh/


Thanks,
SJ

> 
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 
> 
> 

Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission

[Ryan Roberts]

On 13/07/2023 18:32, SeongJae Park wrote:
> On Thu, 13 Jul 2023 16:39:33 +0200 David Hildenbrand <david@redhat.com> wrote:
> 
>> On 13.07.23 15:54, Ryan Roberts wrote:
>>> When run under run_vmtests.sh, test scripts were failing to run with
>>> "permission denied" due to the scripts not being executable.
>>>
>>> It is also annoying not to be able to directly invoke run_vmtests.sh,
>>> which is solved by giving also it the execute permission.
>>>
>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>> ---
>>>   tools/testing/selftests/mm/charge_reserved_hugetlb.sh  | 0
>>>   tools/testing/selftests/mm/check_config.sh             | 0
>>>   tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 0
>>>   tools/testing/selftests/mm/run_vmtests.sh              | 0
>>>   tools/testing/selftests/mm/test_hmm.sh                 | 0
>>>   tools/testing/selftests/mm/test_vmalloc.sh             | 0
>>>   tools/testing/selftests/mm/va_high_addr_switch.sh      | 0
>>>   tools/testing/selftests/mm/write_hugetlb_memory.sh     | 0
>>>   8 files changed, 0 insertions(+), 0 deletions(-)
>>>   mode change 100644 => 100755 tools/testing/selftests/mm/charge_reserved_hugetlb.sh
>>>   mode change 100644 => 100755 tools/testing/selftests/mm/check_config.sh
>>>   mode change 100644 => 100755 tools/testing/selftests/mm/hugetlb_reparenting_test.sh
>>>   mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
>>>   mode change 100644 => 100755 tools/testing/selftests/mm/test_hmm.sh
>>>   mode change 100644 => 100755 tools/testing/selftests/mm/test_vmalloc.sh
>>>   mode change 100644 => 100755 tools/testing/selftests/mm/va_high_addr_switch.sh
>>>   mode change 100644 => 100755 tools/testing/selftests/mm/write_hugetlb_memory.sh
>>>
>>> diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/tools/testing/selftests/mm/check_config.sh b/tools/testing/selftests/mm/check_config.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/tools/testing/selftests/mm/test_hmm.sh b/tools/testing/selftests/mm/test_hmm.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/tools/testing/selftests/mm/test_vmalloc.sh b/tools/testing/selftests/mm/test_vmalloc.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/tools/testing/selftests/mm/va_high_addr_switch.sh b/tools/testing/selftests/mm/va_high_addr_switch.sh
>>> old mode 100644
>>> new mode 100755
>>> diff --git a/tools/testing/selftests/mm/write_hugetlb_memory.sh b/tools/testing/selftests/mm/write_hugetlb_memory.sh
>>> old mode 100644
>>> new mode 100755
>>
>> Sounds reasonable to me.
>>
>> Probably due to:
>>
>> commit baa489fabd01596d5426d6e112b34ba5fb59ab82
>> Author: SeongJae Park <sj@kernel.org>
>> Date:   Tue Jan 3 18:07:53 2023 +0000
>>
>>      selftests/vm: rename selftests/vm to selftests/mm
>>      
>>      Rename selftets/vm to selftests/mm for being more consistent with the
>>      code, documentation, and tools directories, and won't be confused with
>>      virtual machines.
>>
>>
>> and indeed, it contains
>>
>> diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
>> old mode 100755
>> new mode 100644
>> similarity index 100%
>> rename from tools/testing/selftests/vm/run_vmtests.sh
>> rename to tools/testing/selftests/mm/run_vmtests.sh
> 
> Thank you for tracking this and kindly Cc-ing me!  I'd like to clarify a little
> bit more, though.  The permission change has made by the commit as you found.
> Nevertheless, the submitted version[1] of the patch didn't change the
> permission.  I guess the change was made while managing it via some file
> permission unsupported patches management tool.
> 
> I had a similar issue with DAMON selftest and sent a patch restoring the
> permission.  Greg suggested me to update the framework instead, to support such
> management tool[2], so I made it[3].  It recently also merged into 5.15.y for
> DAMON selftests[4].
> 
> I have no strong opinion about whether we need to keep the permission or it's
> good to have no execute permission since kselftest framework supports it.  I
> just wanted to clarify the events I've shown.  Please correct me if I missed or
> wrong something.  Cc-ing Greg, since he might have an opinion.

Thanks for the detailed explanation. Are you effectively saying this patch will
turn into a no-op once its been munged through the various patch management
tools? That's disappointing because it's a pain to have to invoke everything
though bash explicitly. Many other scripts manage to have the correct execute
permission set (see everything in ./scripts for example).

Personally I'd rather keep this patch and try rather than proactively do a work
around.


> 
> [1] https://lore.kernel.org/all/20230103180754.129637-5-sj@kernel.org/
> [2] https://lore.kernel.org/mm-commits/YRJisBs9AunccCD4@kroah.com/
> [3] https://lore.kernel.org/all/20210810164534.25902-1-sj38.park@gmail.com/
> [4] https://lore.kernel.org/stable/2023042743-cheesy-parasitic-206d@gregkh/
> 
> 
> Thanks,
> SJ
> 
>>
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>
>> -- 
>> Cheers,
>>
>> David / dhildenb
>>
>>
>>

Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission

[SeongJae Park]

On Fri, 14 Jul 2023 10:44:14 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote:

> On 13/07/2023 18:32, SeongJae Park wrote:
> > On Thu, 13 Jul 2023 16:39:33 +0200 David Hildenbrand <david@redhat.com> wrote:
> > 
> >> On 13.07.23 15:54, Ryan Roberts wrote:
> >>> When run under run_vmtests.sh, test scripts were failing to run with
> >>> "permission denied" due to the scripts not being executable.
> >>>
> >>> It is also annoying not to be able to directly invoke run_vmtests.sh,
> >>> which is solved by giving also it the execute permission.
> >>>
> >>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> >>> ---
> >>>   tools/testing/selftests/mm/charge_reserved_hugetlb.sh  | 0
> >>>   tools/testing/selftests/mm/check_config.sh             | 0
> >>>   tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 0
> >>>   tools/testing/selftests/mm/run_vmtests.sh              | 0
> >>>   tools/testing/selftests/mm/test_hmm.sh                 | 0
> >>>   tools/testing/selftests/mm/test_vmalloc.sh             | 0
> >>>   tools/testing/selftests/mm/va_high_addr_switch.sh      | 0
> >>>   tools/testing/selftests/mm/write_hugetlb_memory.sh     | 0
> >>>   8 files changed, 0 insertions(+), 0 deletions(-)
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/check_config.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/test_hmm.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/test_vmalloc.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/va_high_addr_switch.sh
> >>>   mode change 100644 => 100755 tools/testing/selftests/mm/write_hugetlb_memory.sh
> >>>
> >>> diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/check_config.sh b/tools/testing/selftests/mm/check_config.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/test_hmm.sh b/tools/testing/selftests/mm/test_hmm.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/test_vmalloc.sh b/tools/testing/selftests/mm/test_vmalloc.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/va_high_addr_switch.sh b/tools/testing/selftests/mm/va_high_addr_switch.sh
> >>> old mode 100644
> >>> new mode 100755
> >>> diff --git a/tools/testing/selftests/mm/write_hugetlb_memory.sh b/tools/testing/selftests/mm/write_hugetlb_memory.sh
> >>> old mode 100644
> >>> new mode 100755
> >>
> >> Sounds reasonable to me.
> >>
> >> Probably due to:
> >>
> >> commit baa489fabd01596d5426d6e112b34ba5fb59ab82
> >> Author: SeongJae Park <sj@kernel.org>
> >> Date:   Tue Jan 3 18:07:53 2023 +0000
> >>
> >>      selftests/vm: rename selftests/vm to selftests/mm
> >>      
> >>      Rename selftets/vm to selftests/mm for being more consistent with the
> >>      code, documentation, and tools directories, and won't be confused with
> >>      virtual machines.
> >>
> >>
> >> and indeed, it contains
> >>
> >> diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> >> old mode 100755
> >> new mode 100644
> >> similarity index 100%
> >> rename from tools/testing/selftests/vm/run_vmtests.sh
> >> rename to tools/testing/selftests/mm/run_vmtests.sh
> > 
> > Thank you for tracking this and kindly Cc-ing me!  I'd like to clarify a little
> > bit more, though.  The permission change has made by the commit as you found.
> > Nevertheless, the submitted version[1] of the patch didn't change the
> > permission.  I guess the change was made while managing it via some file
> > permission unsupported patches management tool.
> > 
> > I had a similar issue with DAMON selftest and sent a patch restoring the
> > permission.  Greg suggested me to update the framework instead, to support such
> > management tool[2], so I made it[3].  It recently also merged into 5.15.y for
> > DAMON selftests[4].
> > 
> > I have no strong opinion about whether we need to keep the permission or it's
> > good to have no execute permission since kselftest framework supports it.  I
> > just wanted to clarify the events I've shown.  Please correct me if I missed or
> > wrong something.  Cc-ing Greg, since he might have an opinion.
> 
> Thanks for the detailed explanation. Are you effectively saying this patch will
> turn into a no-op once its been munged through the various patch management
> tools?

Depending on what tool maintainers that will pick this patch is using in what
way, I guess.

> That's disappointing because it's a pain to have to invoke everything
> though bash explicitly. Many other scripts manage to have the correct execute
> permission set (see everything in ./scripts for example).
> 
> Personally I'd rather keep this patch and try rather than proactively do a work
> around.

I don't have a strong opinion here, as mentioned before.  That said, I feel it
would be good to have a clear agreement or explanation about that, since I got
similar situation before[1].

[1] https://lore.kernel.org/damon/20230221175612.131555-1-sj@kernel.org/


Thanks,
SJ

> 
> 
> > 
> > [1] https://lore.kernel.org/all/20230103180754.129637-5-sj@kernel.org/
> > [2] https://lore.kernel.org/mm-commits/YRJisBs9AunccCD4@kroah.com/
> > [3] https://lore.kernel.org/all/20210810164534.25902-1-sj38.park@gmail.com/
> > [4] https://lore.kernel.org/stable/2023042743-cheesy-parasitic-206d@gregkh/
> > 
> > 
> > Thanks,
> > SJ
> > 
> >>
> >>
> >> Reviewed-by: David Hildenbrand <david@redhat.com>
> >>
> >> -- 
> >> Cheers,
> >>
> >> David / dhildenb
> >>
> >>
> >>
> 
> 

Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission

[Mark Brown]

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

On Fri, Jul 14, 2023 at 04:00:58PM +0000, SeongJae Park wrote:
> On Fri, 14 Jul 2023 10:44:14 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote:

> > Personally I'd rather keep this patch and try rather than proactively do a work
> > around.

> I don't have a strong opinion here, as mentioned before.  That said, I feel it
> would be good to have a clear agreement or explanation about that, since I got
> similar situation before[1].

I think just from a usability point of view we want to end up with
things people are expected to execute actually executable.

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

Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission

[Andrew Morton]

On Fri, 14 Jul 2023 10:44:14 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote:

> Thanks for the detailed explanation. Are you effectively saying this patch will
> turn into a no-op once its been munged through the various patch management
> tools? That's disappointing because it's a pain to have to invoke everything
> though bash explicitly. Many other scripts manage to have the correct execute
> permission set (see everything in ./scripts for example).

Such patches need delicate handling :(

I queued this as a standalone thing, for 6.5-rcx.

Re: [PATCH v1 2/9] selftests/mm: Give scripts execute permission

[Ryan Roberts]

On 14/07/2023 17:26, Andrew Morton wrote:
> On Fri, 14 Jul 2023 10:44:14 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote:
> 
>> Thanks for the detailed explanation. Are you effectively saying this patch will
>> turn into a no-op once its been munged through the various patch management
>> tools? That's disappointing because it's a pain to have to invoke everything
>> though bash explicitly. Many other scripts manage to have the correct execute
>> permission set (see everything in ./scripts for example).
> 
> Such patches need delicate handling :(
> 
> I queued this as a standalone thing, for 6.5-rcx.

That's great - thanks Andrew! Do I'll drop this patch for my v2 of the series
(hopefully Monday).

Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[John Hubbard]

On 7/13/23 06:54, Ryan Roberts wrote:
> arm64 does not support the soft-dirty PTE bit. However there are tests
> in `madv_populate` and `soft-dirty` which assume it is supported and
> cause spurious failures to be reported when preferred behaviour would be
> to mark the tests as skipped.
> 
> 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.

...

> diff --git a/tools/testing/selftests/mm/soft-dirty.c b/tools/testing/selftests/mm/soft-dirty.c
> index cc5f144430d4..8a2cd161ec4d 100644
> --- a/tools/testing/selftests/mm/soft-dirty.c
> +++ b/tools/testing/selftests/mm/soft-dirty.c

Hi Ryan,

Probably very similar to what David is requesting: given that arm64
definitively does not support soft dirty, I'd suggest that we not even
*build* the soft dirty tests on arm64!

There is no need to worry about counting, skipping or waiving such
tests, either. Because it's just a non-issue: one does not care about
test status for something that is documented as "this feature is simply
unavailable here".


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v1 3/9] selftests/mm: Skip soft-dirty tests on arm64

[Ryan Roberts]

On 15/07/2023 01:04, John Hubbard wrote:
> On 7/13/23 06:54, Ryan Roberts wrote:
>> arm64 does not support the soft-dirty PTE bit. However there are tests
>> in `madv_populate` and `soft-dirty` which assume it is supported and
>> cause spurious failures to be reported when preferred behaviour would be
>> to mark the tests as skipped.
>>
>> 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.
> 
> ...
> 
>> diff --git a/tools/testing/selftests/mm/soft-dirty.c
>> b/tools/testing/selftests/mm/soft-dirty.c
>> index cc5f144430d4..8a2cd161ec4d 100644
>> --- a/tools/testing/selftests/mm/soft-dirty.c
>> +++ b/tools/testing/selftests/mm/soft-dirty.c
> 
> Hi Ryan,
> 
> Probably very similar to what David is requesting: given that arm64
> definitively does not support soft dirty, I'd suggest that we not even
> *build* the soft dirty tests on arm64!
> 
> There is no need to worry about counting, skipping or waiving such
> tests, either. Because it's just a non-issue: one does not care about
> test status for something that is documented as "this feature is simply
> unavailable here".

OK fair enough. I'll follow this approach for v2.

Thanks for the review!

> 
> 
> thanks,

Re: [PATCH v1 1/9] selftests: Line buffer test program's stdout

[Ryan Roberts]

On 13/07/2023 15:16, Mark Brown wrote:
> On Thu, Jul 13, 2023 at 02:54:32PM +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
>> 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().
> 
> This is going to be useful in general since not all selftests use the
> kselftest helpers but it'd probably also be good to make
> ksft_print_header() also make the output unbuffered so that if setbuf
> isn't installed on the target system or the tests are run standalone we
> don't run into issues there.  Even if the test isn't corrupting data
> having things unbuffered is going to be good for making sure we don't
> drop any output if the test dies.
> 
>> +		if [ -x /usr/bin/stdbuf ]; then
>> +			stdbuf="/usr/bin/stdbuf --output=L "
>> +		fi
> 
> Might be more robust to use type -p to find stdbuf in case it's in /bin
> or something?

Just looking at making this change; run_selftest.sh's shebang is for sh, and
sh's type doesn't support the -p option. So I'm inclined to leave it as is.
There are multiple other places in the script where /usr/bin is hardcoded when
looking for programs too. Shout if you violently disagree.