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@oracl...
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
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"
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 ]; thenstdbuf="/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?
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 ]; thenstdbuf="/usr/bin/stdbuf --output=L "fiMight be more robust to use type -p to find stdbuf in case it's in /bin or something?
Yep good idea.
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.
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 ]; thenstdbuf="/usr/bin/stdbuf --output=L "fiMight 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.
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
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
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
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
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
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.
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.
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).
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
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.
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. ;-)
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)
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;if (system_has_softdirty())nr_tests += 5;ksft_print_header();
ksft_set_plan(21);
ksft_set_plan(nr_tests);@@ -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();
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 ;)
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. ;-)
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)
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".
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,
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,
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>
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
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);
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
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
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
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; } }
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
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.)
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.
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.
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!
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.
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" ;-)
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
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
linux-kselftest-mirror@lists.linaro.org
-
Andrew Morton -
David Hildenbrand -
John Hubbard -
Mark Brown -
Ryan Roberts -
SeongJae Park