In this series, I'm trying to add 3 missing tests to vm_runtests.sh which is used to run all the tests in mm suite. These tests weren't running by CIs. While enabling them and through review feedback, I've fixed some problems in tests as well. I've found more flakiness in more tests which I'll be fixing with future patches.
hugetlb-read-hwpoison test is being added where it can only run with newly added "-d" (destructive) flag only. Not sure why it is failing again. So once it become stable, we can think of moving it to default set of tests if it doesn't have any side-effect to them.
Cc: Ryan Roberts ryan.roberts@arm.com Cc: Andrew Morton akpm@linux-foundation.org --- Changes in v3: - Add cover letter - Fix flakiness in tests found during enablement - Move additional tests down in the file - Add "-d" option which poisons the pages and aren't being useable after the test
v2: https://lore.kernel.org/all/20240123073615.920324-1-usama.anjum@collabora.co...
Muhammad Usama Anjum (5): selftests/mm: hugetlb_reparenting_test: do not unmount selftests/mm: run_vmtests: remove sudo and conform to tap selftests/mm: save and restore nr_hugepages value selftests/mm: protection_keys: save/restore nr_hugepages settings selftests/mm: run_vmtests.sh: add missing tests
tools/testing/selftests/mm/Makefile | 5 +++ .../selftests/mm/charge_reserved_hugetlb.sh | 4 +++ .../selftests/mm/hugetlb_reparenting_test.sh | 9 +++-- tools/testing/selftests/mm/on-fault-limit.c | 36 +++++++++---------- tools/testing/selftests/mm/protection_keys.c | 34 ++++++++++++++++++ tools/testing/selftests/mm/run_vmtests.sh | 10 +++++- 6 files changed, 76 insertions(+), 22 deletions(-)
Do not unmount the cgroup if it wasn't mounted by the test. The earlier patch had fixed this for charge_reserved_hugetlb, but not for this test. I'm adding fixes tag to that earlier patch.
Fixes: 209376ed2a84 ("selftests/vm: make charge_reserved_hugetlb.sh work with existing cgroup setting") Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh index 14d26075c8635..615c4d766c909 100755 --- a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh +++ b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh @@ -248,5 +248,7 @@ cleanup
echo ALL PASS
-umount $CGROUP_ROOT -rm -rf $CGROUP_ROOT +if [[ $do_umount ]]; then + umount $CGROUP_ROOT + rm -rf $CGROUP_ROOT +fi
Remove sudo as some test running environments may not have sudo available. Instead skip the test if root privileges aren't available in the test.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- Changes since v1: - Added this patch in v2
We are allocating 2*RLIMIT_MEMLOCK.rlim_max memory and mmap() isn't failing. This seems like true bug in the kernel. Even the root user shouldn't be able to allocate more memory than allowed MEMLOCKed memory. Any ideas? --- tools/testing/selftests/mm/on-fault-limit.c | 36 ++++++++++----------- tools/testing/selftests/mm/run_vmtests.sh | 2 +- 2 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/mm/on-fault-limit.c b/tools/testing/selftests/mm/on-fault-limit.c index b5888d613f34e..0ea98ffab3589 100644 --- a/tools/testing/selftests/mm/on-fault-limit.c +++ b/tools/testing/selftests/mm/on-fault-limit.c @@ -5,40 +5,38 @@ #include <string.h> #include <sys/time.h> #include <sys/resource.h> +#include "../kselftest.h"
-static int test_limit(void) +static void test_limit(void) { - int ret = 1; struct rlimit lims; void *map;
- if (getrlimit(RLIMIT_MEMLOCK, &lims)) { - perror("getrlimit"); - return ret; - } + if (getrlimit(RLIMIT_MEMLOCK, &lims)) + ksft_exit_fail_msg("getrlimit: %s\n", strerror(errno));
- if (mlockall(MCL_ONFAULT | MCL_FUTURE)) { - perror("mlockall"); - return ret; - } + if (mlockall(MCL_ONFAULT | MCL_FUTURE)) + ksft_exit_fail_msg("mlockall: %s\n", strerror(errno));
map = mmap(NULL, 2 * lims.rlim_max, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_POPULATE, -1, 0); + + ksft_test_result(map == MAP_FAILED, "Failed mmap\n"); + if (map != MAP_FAILED) - printf("mmap should have failed, but didn't\n"); - else { - ret = 0; munmap(map, 2 * lims.rlim_max); - } - munlockall(); - return ret; }
int main(int argc, char **argv) { - int ret = 0; + ksft_print_header(); + ksft_set_plan(1); + + if (getuid()) + ksft_test_result_skip("Require root privileges to run\n"); + else + test_limit();
- ret += test_limit(); - return ret; + ksft_finished(); } diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index 246d53a5d7f28..e373d592dbf5c 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -291,7 +291,7 @@ echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages
CATEGORY="compaction" run_test ./compaction_test
-CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit +CATEGORY="mlock" run_test ./on-fault-limit
CATEGORY="mmap" run_test ./map_populate
Save and restore nr_hugepages before changing it during the test. A test should not change system wide settings.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- tools/testing/selftests/mm/charge_reserved_hugetlb.sh | 4 ++++ tools/testing/selftests/mm/hugetlb_reparenting_test.sh | 3 +++ 2 files changed, 7 insertions(+)
diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh index e14bdd4455f2d..d680c00d2853a 100755 --- a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh +++ b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh @@ -11,6 +11,8 @@ if [[ $(id -u) -ne 0 ]]; then exit $ksft_skip fi
+nr_hugepgs=$(cat /proc/sys/vm/nr_hugepages) + fault_limit_file=limit_in_bytes reservation_limit_file=rsvd.limit_in_bytes fault_usage_file=usage_in_bytes @@ -582,3 +584,5 @@ if [[ $do_umount ]]; then umount $cgroup_path rmdir $cgroup_path fi + +echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages diff --git a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh index 615c4d766c909..11f9bbe7dc222 100755 --- a/tools/testing/selftests/mm/hugetlb_reparenting_test.sh +++ b/tools/testing/selftests/mm/hugetlb_reparenting_test.sh @@ -11,6 +11,7 @@ if [[ $(id -u) -ne 0 ]]; then exit $ksft_skip fi
+nr_hugepgs=$(cat /proc/sys/vm/nr_hugepages) usage_file=usage_in_bytes
if [[ "$1" == "-cgroup-v2" ]]; then @@ -252,3 +253,5 @@ if [[ $do_umount ]]; then umount $CGROUP_ROOT rm -rf $CGROUP_ROOT fi + +echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages
Save and restore nr_hugepages before changing it during the test. A test should not change system wide settings.
Fixes: 5f23f6d082a9 ("x86/pkeys: Add self-tests") Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- tools/testing/selftests/mm/protection_keys.c | 34 ++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index 48dc151f8fca8..f822ae31af22e 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -54,6 +54,7 @@ int test_nr; u64 shadow_pkey_reg; int dprint_in_signal; char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE]; +char buf[256];
void cat_into_file(char *str, char *file) { @@ -1744,6 +1745,38 @@ void pkey_setup_shadow(void) shadow_pkey_reg = __read_pkey_reg(); }
+void restore_settings_atexit(void) +{ + cat_into_file(buf, "/proc/sys/vm/nr_hugepages"); +} + +void save_settings(void) +{ + int fd; + int err; + + if (geteuid()) + return; + + fd = open("/proc/sys/vm/nr_hugepages", O_RDONLY); + if (fd < 0) { + fprintf(stderr, "error opening\n"); + perror("error: "); + exit(__LINE__); + } + + /* -1 to guarantee leaving the trailing \0 */ + err = read(fd, buf, sizeof(buf)-1); + if (err < 0) { + fprintf(stderr, "error reading\n"); + perror("error: "); + exit(__LINE__); + } + + atexit(restore_settings_atexit); + close(fd); +} + int main(void) { int nr_iterations = 22; @@ -1751,6 +1784,7 @@ int main(void)
srand((unsigned int)time(NULL));
+ save_settings(); setup_handlers();
printf("has pkeys: %d\n", pkeys_supported);
Add missing tests to run_vmtests.sh. The mm kselftests are run through run_vmtests.sh. If a test isn't present in this script, it'll not run with run_tests or `make -C tools/testing/selftests/mm run_tests`.
Cc: Ryan Roberts ryan.roberts@arm.com Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- Changes since v1: - Copy the original scripts and their dependence script to install directory as well
Changes since v2: - Add a comment - Move tests down in the file - Add "-d" option which poisons the pages and aren't being useable after the test --- tools/testing/selftests/mm/Makefile | 5 +++++ tools/testing/selftests/mm/run_vmtests.sh | 8 ++++++++ 2 files changed, 13 insertions(+)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 2453add65d12f..f3aec7be80730 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -114,6 +114,11 @@ TEST_PROGS := run_vmtests.sh TEST_FILES := test_vmalloc.sh TEST_FILES += test_hmm.sh TEST_FILES += va_high_addr_switch.sh +TEST_FILES += charge_reserved_hugetlb.sh +TEST_FILES += hugetlb_reparenting_test.sh + +# required by charge_reserved_hugetlb.sh +TEST_FILES += write_hugetlb_memory.sh
include ../lib.mk
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index e373d592dbf5c..a0f37e4438937 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -19,6 +19,7 @@ usage: ${BASH_SOURCE[0]:-$0} [ options ] -t: specify specific categories to tests to run -h: display this message -n: disable TAP output + -d: run destructive tests
The default behavior is to run required tests only. If -a is specified, will run all tests. @@ -79,6 +80,7 @@ EOF }
RUN_ALL=false +RUN_DESTRUCTIVE_TEST=false TAP_PREFIX="# "
while getopts "aht:n" OPT; do @@ -87,6 +89,7 @@ while getopts "aht:n" OPT; do "h") usage ;; "t") VM_SELFTEST_ITEMS=${OPTARG} ;; "n") TAP_PREFIX= ;; + "a") RUN_DESTRUCTIVE_TEST=true ;; esac done shift $((OPTIND -1)) @@ -304,6 +307,11 @@ CATEGORY="process_mrelease" run_test ./mrelease_test CATEGORY="mremap" run_test ./mremap_test
CATEGORY="hugetlb" run_test ./thuge-gen +CATEGORY="hugetlb" run_test ./charge_reserved_hugetlb.sh -cgroup-v2 +CATEGORY="hugetlb" run_test ./hugetlb_reparenting_test.sh -cgroup-v2 +if $RUN_DESTRUCTIVE_TEST; then +CATEGORY="hugetlb" run_test ./hugetlb-read-hwpoison +fi
if [ $VADDR64 -ne 0 ]; then
On 25/01/2024 15:46, Muhammad Usama Anjum wrote:
Remove sudo as some test running environments may not have sudo available. Instead skip the test if root privileges aren't available in the test.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes since v1:
- Added this patch in v2
We are allocating 2*RLIMIT_MEMLOCK.rlim_max memory and mmap() isn't failing. This seems like true bug in the kernel. Even the root user shouldn't be able to allocate more memory than allowed MEMLOCKed memory. Any ideas?
tools/testing/selftests/mm/on-fault-limit.c | 36 ++++++++++----------- tools/testing/selftests/mm/run_vmtests.sh | 2 +- 2 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/mm/on-fault-limit.c b/tools/testing/selftests/mm/on-fault-limit.c index b5888d613f34e..0ea98ffab3589 100644 --- a/tools/testing/selftests/mm/on-fault-limit.c +++ b/tools/testing/selftests/mm/on-fault-limit.c @@ -5,40 +5,38 @@ #include <string.h> #include <sys/time.h> #include <sys/resource.h> +#include "../kselftest.h" -static int test_limit(void) +static void test_limit(void) {
- int ret = 1; struct rlimit lims; void *map;
- if (getrlimit(RLIMIT_MEMLOCK, &lims)) {
perror("getrlimit");return ret;- }
- if (getrlimit(RLIMIT_MEMLOCK, &lims))
ksft_exit_fail_msg("getrlimit: %s\n", strerror(errno));
- if (mlockall(MCL_ONFAULT | MCL_FUTURE)) {
perror("mlockall");return ret;- }
- if (mlockall(MCL_ONFAULT | MCL_FUTURE))
ksft_exit_fail_msg("mlockall: %s\n", strerror(errno));map = mmap(NULL, 2 * lims.rlim_max, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_POPULATE, -1, 0);
- ksft_test_result(map == MAP_FAILED, "Failed mmap\n");
- if (map != MAP_FAILED)
printf("mmap should have failed, but didn't\n");- else {
ret = 0;- }
- munlockall();
- return ret;
} int main(int argc, char **argv) {
- int ret = 0;
- ksft_print_header();
- ksft_set_plan(1);
- if (getuid())
ksft_test_result_skip("Require root privileges to run\n");- else
test_limit();
- ret += test_limit();
- return ret;
- ksft_finished();
} diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index 246d53a5d7f28..e373d592dbf5c 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -291,7 +291,7 @@ echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages CATEGORY="compaction" run_test ./compaction_test -CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit +CATEGORY="mlock" run_test ./on-fault-limit
I think changing this is going to give unintended results. run_vmtests.sh must already be running as root. "sudo -u nobody" is deprivieging the test to run as nobody. The rlimit is not enforced for root so this test must run unprivileged to work. See man page for getrlimit():
Since Linux 2.6.9, no limits are placed on the amount of memory that a privileged process may lock, and this limit instead governs the amount of memory that an unprivileged process may lock
So I think the correct fix is actually to install sudo on your CI.
CATEGORY="mmap" run_test ./map_populate
On 25/01/2024 15:46, Muhammad Usama Anjum wrote:
Add missing tests to run_vmtests.sh. The mm kselftests are run through run_vmtests.sh. If a test isn't present in this script, it'll not run with run_tests or `make -C tools/testing/selftests/mm run_tests`.
Cc: Ryan Roberts ryan.roberts@arm.com Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes since v1:
- Copy the original scripts and their dependence script to install directory as well
Changes since v2:
- Add a comment
- Move tests down in the file
- Add "-d" option which poisons the pages and aren't being useable after the test
tools/testing/selftests/mm/Makefile | 5 +++++ tools/testing/selftests/mm/run_vmtests.sh | 8 ++++++++ 2 files changed, 13 insertions(+)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 2453add65d12f..f3aec7be80730 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -114,6 +114,11 @@ TEST_PROGS := run_vmtests.sh TEST_FILES := test_vmalloc.sh TEST_FILES += test_hmm.sh TEST_FILES += va_high_addr_switch.sh +TEST_FILES += charge_reserved_hugetlb.sh +TEST_FILES += hugetlb_reparenting_test.sh
+# required by charge_reserved_hugetlb.sh +TEST_FILES += write_hugetlb_memory.sh include ../lib.mk diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index e373d592dbf5c..a0f37e4438937 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -19,6 +19,7 @@ usage: ${BASH_SOURCE[0]:-$0} [ options ] -t: specify specific categories to tests to run -h: display this message -n: disable TAP output
- -d: run destructive tests
You probably want to clarify the behaviour for -a (all). I guess providing -a should NOT run destructive tests unless -d is also explicitly provided.
The default behavior is to run required tests only. If -a is specified, will run all tests. @@ -79,6 +80,7 @@ EOF } RUN_ALL=false +RUN_DESTRUCTIVE_TEST=false
Either call this RUN_DESTRUCTIVE (my preference) or at least make it plural (RUN_DESTRUCTIVE_TESTS).
TAP_PREFIX="# " while getopts "aht:n" OPT; do @@ -87,6 +89,7 @@ while getopts "aht:n" OPT; do "h") usage ;; "t") VM_SELFTEST_ITEMS=${OPTARG} ;; "n") TAP_PREFIX= ;;
"a") RUN_DESTRUCTIVE_TEST=true ;;
The help you added says the option is -d, but this is looking for -a, and conflicting with the existing -a=all option.
esac done shift $((OPTIND -1)) @@ -304,6 +307,11 @@ CATEGORY="process_mrelease" run_test ./mrelease_test CATEGORY="mremap" run_test ./mremap_test CATEGORY="hugetlb" run_test ./thuge-gen +CATEGORY="hugetlb" run_test ./charge_reserved_hugetlb.sh -cgroup-v2 +CATEGORY="hugetlb" run_test ./hugetlb_reparenting_test.sh -cgroup-v2 +if $RUN_DESTRUCTIVE_TEST; then +CATEGORY="hugetlb" run_test ./hugetlb-read-hwpoison +fi if [ $VADDR64 -ne 0 ]; then
On 2/1/24 5:04 PM, Ryan Roberts wrote:
On 25/01/2024 15:46, Muhammad Usama Anjum wrote:
Remove sudo as some test running environments may not have sudo available. Instead skip the test if root privileges aren't available in the test.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes since v1:
- Added this patch in v2
We are allocating 2*RLIMIT_MEMLOCK.rlim_max memory and mmap() isn't failing. This seems like true bug in the kernel. Even the root user shouldn't be able to allocate more memory than allowed MEMLOCKed memory. Any ideas?
tools/testing/selftests/mm/on-fault-limit.c | 36 ++++++++++----------- tools/testing/selftests/mm/run_vmtests.sh | 2 +- 2 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/mm/on-fault-limit.c b/tools/testing/selftests/mm/on-fault-limit.c index b5888d613f34e..0ea98ffab3589 100644 --- a/tools/testing/selftests/mm/on-fault-limit.c +++ b/tools/testing/selftests/mm/on-fault-limit.c @@ -5,40 +5,38 @@ #include <string.h> #include <sys/time.h> #include <sys/resource.h> +#include "../kselftest.h" -static int test_limit(void) +static void test_limit(void) {
- int ret = 1; struct rlimit lims; void *map;
- if (getrlimit(RLIMIT_MEMLOCK, &lims)) {
perror("getrlimit");return ret;- }
- if (getrlimit(RLIMIT_MEMLOCK, &lims))
ksft_exit_fail_msg("getrlimit: %s\n", strerror(errno));
- if (mlockall(MCL_ONFAULT | MCL_FUTURE)) {
perror("mlockall");return ret;- }
- if (mlockall(MCL_ONFAULT | MCL_FUTURE))
ksft_exit_fail_msg("mlockall: %s\n", strerror(errno));map = mmap(NULL, 2 * lims.rlim_max, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_POPULATE, -1, 0);
- ksft_test_result(map == MAP_FAILED, "Failed mmap\n");
- if (map != MAP_FAILED)
printf("mmap should have failed, but didn't\n");- else {
ret = 0;- }
- munlockall();
- return ret;
} int main(int argc, char **argv) {
- int ret = 0;
- ksft_print_header();
- ksft_set_plan(1);
- if (getuid())
ksft_test_result_skip("Require root privileges to run\n");
I'd sent a patch to fix this behavior today. This test should run without root privileges. https://lore.kernel.org/all/20240201071307.592317-1-usama.anjum@collabora.co...
- else
test_limit();
- ret += test_limit();
- return ret;
- ksft_finished();
} diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index 246d53a5d7f28..e373d592dbf5c 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -291,7 +291,7 @@ echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages CATEGORY="compaction" run_test ./compaction_test -CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit +CATEGORY="mlock" run_test ./on-fault-limit
I think changing this is going to give unintended results. run_vmtests.sh must already be running as root. "sudo -u nobody" is deprivieging the test to run as nobody. The rlimit is not enforced for root so this test must run unprivileged to work. See man page for getrlimit():
Since Linux 2.6.9, no limits are placed on the amount of memory that a privileged process may lock, and this limit instead governs the amount of memory that an unprivileged process may lock
So I think the correct fix is actually to install sudo on your CI.
run_vmtests.sh is invoked without sudo with following: make -C tools/testing/selftests/mm run_tests
Installing sudo in rootfs wouldn't be trivial enough on the CI side. Alternatively, we can check if sudo is present before executing this test to avoid error that sudo isn't found.
CATEGORY="mmap" run_test ./map_populate
On 2/1/24 5:11 PM, Ryan Roberts wrote:
On 25/01/2024 15:46, Muhammad Usama Anjum wrote:
Add missing tests to run_vmtests.sh. The mm kselftests are run through run_vmtests.sh. If a test isn't present in this script, it'll not run with run_tests or `make -C tools/testing/selftests/mm run_tests`.
Cc: Ryan Roberts ryan.roberts@arm.com Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes since v1:
- Copy the original scripts and their dependence script to install directory as well
Changes since v2:
- Add a comment
- Move tests down in the file
- Add "-d" option which poisons the pages and aren't being useable after the test
tools/testing/selftests/mm/Makefile | 5 +++++ tools/testing/selftests/mm/run_vmtests.sh | 8 ++++++++ 2 files changed, 13 insertions(+)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 2453add65d12f..f3aec7be80730 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -114,6 +114,11 @@ TEST_PROGS := run_vmtests.sh TEST_FILES := test_vmalloc.sh TEST_FILES += test_hmm.sh TEST_FILES += va_high_addr_switch.sh +TEST_FILES += charge_reserved_hugetlb.sh +TEST_FILES += hugetlb_reparenting_test.sh
+# required by charge_reserved_hugetlb.sh +TEST_FILES += write_hugetlb_memory.sh include ../lib.mk diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index e373d592dbf5c..a0f37e4438937 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -19,6 +19,7 @@ usage: ${BASH_SOURCE[0]:-$0} [ options ] -t: specify specific categories to tests to run -h: display this message -n: disable TAP output
- -d: run destructive tests
You probably want to clarify the behaviour for -a (all). I guess providing -a should NOT run destructive tests unless -d is also explicitly provided.
The default behavior is to run required tests only. If -a is specified, will run all tests. @@ -79,6 +80,7 @@ EOF } RUN_ALL=false +RUN_DESTRUCTIVE_TEST=false
Either call this RUN_DESTRUCTIVE (my preference) or at least make it plural (RUN_DESTRUCTIVE_TESTS).
TAP_PREFIX="# " while getopts "aht:n" OPT; do @@ -87,6 +89,7 @@ while getopts "aht:n" OPT; do "h") usage ;; "t") VM_SELFTEST_ITEMS=${OPTARG} ;; "n") TAP_PREFIX= ;;
"a") RUN_DESTRUCTIVE_TEST=true ;;The help you added says the option is -d, but this is looking for -a, and conflicting with the existing -a=all option.
Sorry, that's a typo. I'll resolve your above comments with fix patch as well.
esac done shift $((OPTIND -1)) @@ -304,6 +307,11 @@ CATEGORY="process_mrelease" run_test ./mrelease_test CATEGORY="mremap" run_test ./mremap_test CATEGORY="hugetlb" run_test ./thuge-gen +CATEGORY="hugetlb" run_test ./charge_reserved_hugetlb.sh -cgroup-v2 +CATEGORY="hugetlb" run_test ./hugetlb_reparenting_test.sh -cgroup-v2 +if $RUN_DESTRUCTIVE_TEST; then +CATEGORY="hugetlb" run_test ./hugetlb-read-hwpoison +fi if [ $VADDR64 -ne 0 ]; then
On 01/02/2024 12:24, Muhammad Usama Anjum wrote:
On 2/1/24 5:04 PM, Ryan Roberts wrote:
On 25/01/2024 15:46, Muhammad Usama Anjum wrote:
Remove sudo as some test running environments may not have sudo available. Instead skip the test if root privileges aren't available in the test.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes since v1:
- Added this patch in v2
We are allocating 2*RLIMIT_MEMLOCK.rlim_max memory and mmap() isn't failing. This seems like true bug in the kernel. Even the root user shouldn't be able to allocate more memory than allowed MEMLOCKed memory. Any ideas?
tools/testing/selftests/mm/on-fault-limit.c | 36 ++++++++++----------- tools/testing/selftests/mm/run_vmtests.sh | 2 +- 2 files changed, 18 insertions(+), 20 deletions(-)
diff --git a/tools/testing/selftests/mm/on-fault-limit.c b/tools/testing/selftests/mm/on-fault-limit.c index b5888d613f34e..0ea98ffab3589 100644 --- a/tools/testing/selftests/mm/on-fault-limit.c +++ b/tools/testing/selftests/mm/on-fault-limit.c @@ -5,40 +5,38 @@ #include <string.h> #include <sys/time.h> #include <sys/resource.h> +#include "../kselftest.h" -static int test_limit(void) +static void test_limit(void) {
- int ret = 1; struct rlimit lims; void *map;
- if (getrlimit(RLIMIT_MEMLOCK, &lims)) {
perror("getrlimit");return ret;- }
- if (getrlimit(RLIMIT_MEMLOCK, &lims))
ksft_exit_fail_msg("getrlimit: %s\n", strerror(errno));
- if (mlockall(MCL_ONFAULT | MCL_FUTURE)) {
perror("mlockall");return ret;- }
- if (mlockall(MCL_ONFAULT | MCL_FUTURE))
ksft_exit_fail_msg("mlockall: %s\n", strerror(errno));map = mmap(NULL, 2 * lims.rlim_max, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_POPULATE, -1, 0);
- ksft_test_result(map == MAP_FAILED, "Failed mmap\n");
- if (map != MAP_FAILED)
printf("mmap should have failed, but didn't\n");- else {
ret = 0;- }
- munlockall();
- return ret;
} int main(int argc, char **argv) {
- int ret = 0;
- ksft_print_header();
- ksft_set_plan(1);
- if (getuid())
ksft_test_result_skip("Require root privileges to run\n");I'd sent a patch to fix this behavior today. This test should run without root privileges. https://lore.kernel.org/all/20240201071307.592317-1-usama.anjum@collabora.co...
- else
test_limit();
- ret += test_limit();
- return ret;
- ksft_finished();
} diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh index 246d53a5d7f28..e373d592dbf5c 100755 --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -291,7 +291,7 @@ echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages CATEGORY="compaction" run_test ./compaction_test -CATEGORY="mlock" run_test sudo -u nobody ./on-fault-limit +CATEGORY="mlock" run_test ./on-fault-limit
I think changing this is going to give unintended results. run_vmtests.sh must already be running as root. "sudo -u nobody" is deprivieging the test to run as nobody. The rlimit is not enforced for root so this test must run unprivileged to work. See man page for getrlimit():
Since Linux 2.6.9, no limits are placed on the amount of memory that a privileged process may lock, and this limit instead governs the amount of memory that an unprivileged process may lock
So I think the correct fix is actually to install sudo on your CI.
run_vmtests.sh is invoked without sudo with following: make -C tools/testing/selftests/mm run_tests
Unfortunately, I live in a world where my build machine isn't always the same as the target machine, so I'm not too familiar with this method of invocation.
Regardless, the vast majority of the tests in run_vmtests.sh (as well as the configuration code in the script itself) require root. So invoking run_vmtests.sh as anything other than root is a BadIdea (TM). And when run_vmtests.sh is running as root, then you need the "sudo -u nobody" to deprivilege this particular test.
Installing sudo in rootfs wouldn't be trivial enough on the CI side. Alternatively, we can check if sudo is present before executing this test to avoid error that sudo isn't found.
Yeah, that's probably the easiest solution; just skip it if the requirements are not met.
CATEGORY="mmap" run_test ./map_populate
Hi Muhammad,
On Thu, Jan 25, 2024 at 08:46:07PM +0500, Muhammad Usama Anjum wrote:
Save and restore nr_hugepages before changing it during the test. A test should not change system wide settings.
Fixes: 5f23f6d082a9 ("x86/pkeys: Add self-tests") Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
tools/testing/selftests/mm/protection_keys.c | 34 ++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index 48dc151f8fca8..f822ae31af22e 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -54,6 +54,7 @@ int test_nr; u64 shadow_pkey_reg; int dprint_in_signal; char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE]; +char buf[256]; void cat_into_file(char *str, char *file) { @@ -1744,6 +1745,38 @@ void pkey_setup_shadow(void) shadow_pkey_reg = __read_pkey_reg(); } +void restore_settings_atexit(void) +{
- cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
+}
+void save_settings(void) +{
- int fd;
- int err;
- if (geteuid())
return;- fd = open("/proc/sys/vm/nr_hugepages", O_RDONLY);
- if (fd < 0) {
fprintf(stderr, "error opening\n");perror("error: ");exit(__LINE__);- }
- /* -1 to guarantee leaving the trailing \0 */
- err = read(fd, buf, sizeof(buf)-1);
- if (err < 0) {
fprintf(stderr, "error reading\n");perror("error: ");exit(__LINE__);- }
- atexit(restore_settings_atexit);
- close(fd);
+}
int main(void) { int nr_iterations = 22; @@ -1751,6 +1784,7 @@ int main(void) srand((unsigned int)time(NULL));
- save_settings(); setup_handlers();
printf("has pkeys: %d\n", pkeys_supported); -- 2.42.0
This break the tests for me:
assert() at protection_keys.c::812 test_nr: 19 iteration: 1 running abort_hooks()...
This is because some of the tests fork, so on their atexit() they will set the nr_hugepages back to the previous setting. Specifically the test_pkey_alloc_exhaust() test.
Thanks, Joey
On 3/13/24 7:58 PM, Joey Gouly wrote:
Hi Muhammad,
On Thu, Jan 25, 2024 at 08:46:07PM +0500, Muhammad Usama Anjum wrote:
Save and restore nr_hugepages before changing it during the test. A test should not change system wide settings.
Fixes: 5f23f6d082a9 ("x86/pkeys: Add self-tests") Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
tools/testing/selftests/mm/protection_keys.c | 34 ++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index 48dc151f8fca8..f822ae31af22e 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -54,6 +54,7 @@ int test_nr; u64 shadow_pkey_reg; int dprint_in_signal; char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE]; +char buf[256]; void cat_into_file(char *str, char *file) { @@ -1744,6 +1745,38 @@ void pkey_setup_shadow(void) shadow_pkey_reg = __read_pkey_reg(); } +void restore_settings_atexit(void) +{
- cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
+}
+void save_settings(void) +{
- int fd;
- int err;
- if (geteuid())
return;- fd = open("/proc/sys/vm/nr_hugepages", O_RDONLY);
- if (fd < 0) {
fprintf(stderr, "error opening\n");perror("error: ");exit(__LINE__);- }
- /* -1 to guarantee leaving the trailing \0 */
- err = read(fd, buf, sizeof(buf)-1);
- if (err < 0) {
fprintf(stderr, "error reading\n");perror("error: ");exit(__LINE__);- }
- atexit(restore_settings_atexit);
- close(fd);
+}
int main(void) { int nr_iterations = 22; @@ -1751,6 +1784,7 @@ int main(void) srand((unsigned int)time(NULL));
- save_settings(); setup_handlers();
printf("has pkeys: %d\n", pkeys_supported); -- 2.42.0
This break the tests for me:
assert() at protection_keys.c::812 test_nr: 19 iteration: 1 running abort_hooks()...
This is because some of the tests fork, so on their atexit() they will set the nr_hugepages back to the previous setting. Specifically the test_pkey_alloc_exhaust() test.
Thank you for reporting. Please can you test the following patch:
--- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -1745,9 +1745,12 @@ void pkey_setup_shadow(void) shadow_pkey_reg = __read_pkey_reg(); }
+pid_t parent_pid; + void restore_settings_atexit(void) { - cat_into_file(buf, "/proc/sys/vm/nr_hugepages"); + if (parent_pid == getpid()) + cat_into_file(buf, "/proc/sys/vm/nr_hugepages"); }
void save_settings(void) @@ -1773,6 +1776,7 @@ void save_settings(void) exit(__LINE__); }
+ parent_pid = getpid(); atexit(restore_settings_atexit); close(fd); }
Thanks, Joey
On Wed, Mar 13, 2024 at 11:12:58PM +0500, Muhammad Usama Anjum wrote:
On 3/13/24 7:58 PM, Joey Gouly wrote:
Hi Muhammad,
On Thu, Jan 25, 2024 at 08:46:07PM +0500, Muhammad Usama Anjum wrote:
Save and restore nr_hugepages before changing it during the test. A test should not change system wide settings.
Fixes: 5f23f6d082a9 ("x86/pkeys: Add self-tests") Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
tools/testing/selftests/mm/protection_keys.c | 34 ++++++++++++++++++++ 1 file changed, 34 insertions(+)
diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index 48dc151f8fca8..f822ae31af22e 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -54,6 +54,7 @@ int test_nr; u64 shadow_pkey_reg; int dprint_in_signal; char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE]; +char buf[256]; void cat_into_file(char *str, char *file) { @@ -1744,6 +1745,38 @@ void pkey_setup_shadow(void) shadow_pkey_reg = __read_pkey_reg(); } +void restore_settings_atexit(void) +{
- cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
+}
+void save_settings(void) +{
- int fd;
- int err;
- if (geteuid())
return;- fd = open("/proc/sys/vm/nr_hugepages", O_RDONLY);
- if (fd < 0) {
fprintf(stderr, "error opening\n");perror("error: ");exit(__LINE__);- }
- /* -1 to guarantee leaving the trailing \0 */
- err = read(fd, buf, sizeof(buf)-1);
- if (err < 0) {
fprintf(stderr, "error reading\n");perror("error: ");exit(__LINE__);- }
- atexit(restore_settings_atexit);
- close(fd);
+}
int main(void) { int nr_iterations = 22; @@ -1751,6 +1784,7 @@ int main(void) srand((unsigned int)time(NULL));
- save_settings(); setup_handlers();
printf("has pkeys: %d\n", pkeys_supported); -- 2.42.0
This break the tests for me:
assert() at protection_keys.c::812 test_nr: 19 iteration: 1 running abort_hooks()...
This is because some of the tests fork, so on their atexit() they will set the nr_hugepages back to the previous setting. Specifically the test_pkey_alloc_exhaust() test.
Thank you for reporting. Please can you test the following patch:
--- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -1745,9 +1745,12 @@ void pkey_setup_shadow(void) shadow_pkey_reg = __read_pkey_reg(); }
+pid_t parent_pid;
void restore_settings_atexit(void) {
- cat_into_file(buf, "/proc/sys/vm/nr_hugepages");
- if (parent_pid == getpid())
cat_into_file(buf, "/proc/sys/vm/nr_hugepages");}
void save_settings(void) @@ -1773,6 +1776,7 @@ void save_settings(void) exit(__LINE__); }
- parent_pid = getpid(); atexit(restore_settings_atexit); close(fd);
}
Thanks, works for me:
Tested-by: Joey Gouly joey.gouly@arm.com
linux-kselftest-mirror@lists.linaro.org
-
Joey Gouly -
Muhammad Usama Anjum -
Ryan Roberts