diff mbox series

[v2,1/2] selftests/mm: run_vmtests.sh: add missing tests

Message ID 20240123073615.920324-1-usama.anjum@collabora.com
State New
Headers
Series [v2,1/2] selftests/mm: run_vmtests.sh: add missing tests |

Commit Message

Muhammad Usama Anjum Jan. 23, 2024, 7:36 a.m. UTC 
  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
---
 tools/testing/selftests/mm/Makefile       | 3 +++
 tools/testing/selftests/mm/run_vmtests.sh | 3 +++
 2 files changed, 6 insertions(+)

Comments

Ryan Roberts Jan. 23, 2024, 9:33 a.m. UTC | #1 
On 23/01/2024 07:36, 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
> ---
>  tools/testing/selftests/mm/Makefile       | 3 +++
>  tools/testing/selftests/mm/run_vmtests.sh | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index 2453add65d12f..c9c8112a7262e 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -114,6 +114,9 @@ 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 += write_hugetlb_memory.sh
> +TEST_FILES += hugetlb_reparenting_test.sh

I see you are exporting 3 scripts, but only invoking 2 of them from
run_vmtests.sh below. Is one a helper that gets called indirectly?

>  
>  include ../lib.mk
>  
> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> index 246d53a5d7f28..12754af00b39c 100755
> --- a/tools/testing/selftests/mm/run_vmtests.sh
> +++ b/tools/testing/selftests/mm/run_vmtests.sh
> @@ -248,6 +248,9 @@ CATEGORY="hugetlb" run_test ./map_hugetlb
>  CATEGORY="hugetlb" run_test ./hugepage-mremap
>  CATEGORY="hugetlb" run_test ./hugepage-vmemmap
>  CATEGORY="hugetlb" run_test ./hugetlb-madvise
> +CATEGORY="hugetlb" run_test ./charge_reserved_hugetlb.sh -cgroup-v2
> +CATEGORY="hugetlb" run_test ./hugetlb_reparenting_test.sh -cgroup-v2
> +CATEGORY="hugetlb" run_test ./hugetlb-read-hwpoison

I'm not really a fan of adding this last test here; its destructive because it
poisons 8 hugepages. So at a minimum, I think you need to modify the code in
run_vmtests.sh to ensure those extra pages are allocated (there is already a
section in the script that allocates hugepages).

However, given this test is destructive, I'd prefer that it wasn't run as part
of the main test set. Because the first time you run it, it will presumably
pass, but now some of the hugepages are poisoned so next time you run it, there
won't be enough unpoisoned hugepages and a test will fail. So you have very
confusing behaviour for a developer who might be running these tests multiple
times per boot (e.g. me).

Perhaps we can add a -d (destructive) option to the script, and this test will
only be run if that option is passed?

Thanks,
Ryan


>  
>  nr_hugepages_tmp=$(cat /proc/sys/vm/nr_hugepages)
>  # For this test, we need one and just one huge page
Muhammad Usama Anjum Jan. 23, 2024, 2:45 p.m. UTC | #2 
Hi Ryan,

Thank you so much for reviewing and getting involved.

On 1/23/24 2:33 PM, Ryan Roberts wrote:
> On 23/01/2024 07:36, 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
>> ---
>>  tools/testing/selftests/mm/Makefile       | 3 +++
>>  tools/testing/selftests/mm/run_vmtests.sh | 3 +++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
>> index 2453add65d12f..c9c8112a7262e 100644
>> --- a/tools/testing/selftests/mm/Makefile
>> +++ b/tools/testing/selftests/mm/Makefile
>> @@ -114,6 +114,9 @@ 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 += write_hugetlb_memory.sh
>> +TEST_FILES += hugetlb_reparenting_test.sh
> 
> I see you are exporting 3 scripts, but only invoking 2 of them from
> run_vmtests.sh below. Is one a helper that gets called indirectly?
Yeah, write_hugetlb_memory.sh is needed by charge_reserved_hugetlb.sh. I'll
put a comment there.

> 
>>  
>>  include ../lib.mk
>>  
>> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
>> index 246d53a5d7f28..12754af00b39c 100755
>> --- a/tools/testing/selftests/mm/run_vmtests.sh
>> +++ b/tools/testing/selftests/mm/run_vmtests.sh
>> @@ -248,6 +248,9 @@ CATEGORY="hugetlb" run_test ./map_hugetlb
>>  CATEGORY="hugetlb" run_test ./hugepage-mremap
>>  CATEGORY="hugetlb" run_test ./hugepage-vmemmap
>>  CATEGORY="hugetlb" run_test ./hugetlb-madvise
>> +CATEGORY="hugetlb" run_test ./charge_reserved_hugetlb.sh -cgroup-v2
>> +CATEGORY="hugetlb" run_test ./hugetlb_reparenting_test.sh -cgroup-v2
>> +CATEGORY="hugetlb" run_test ./hugetlb-read-hwpoison
> 
> I'm not really a fan of adding this last test here; its destructive because it
> poisons 8 hugepages. So at a minimum, I think you need to modify the code in
> run_vmtests.sh to ensure those extra pages are allocated (there is already a
> section in the script that allocates hugepages).
> 
> However, given this test is destructive, I'd prefer that it wasn't run as part
> of the main test set. Because the first time you run it, it will presumably
> pass, but now some of the hugepages are poisoned so next time you run it, there
> won't be enough unpoisoned hugepages and a test will fail. So you have very
> confusing behaviour for a developer who might be running these tests multiple
> times per boot (e.g. me).
> 
> Perhaps we can add a -d (destructive) option to the script, and this test will
> only be run if that option is passed?
Ideally we should be able to fix these tests before enabling them and there
shouldn't be any side-effect of these. I'm struggling with the
configurations where I'm getting consistent results. Studying and analyzing
how and how many hugetlbs are being allocated/deallocated isn't straight
forward enough in these.

I'll spend more time to either put it under some flag or modify the tests
to don't entangle with each other.

> 
> Thanks,
> Ryan
> 
> 
>>  
>>  nr_hugepages_tmp=$(cat /proc/sys/vm/nr_hugepages)
>>  # For this test, we need one and just one huge page
> 
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 2453add65d12f..c9c8112a7262e 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -114,6 +114,9 @@  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 += write_hugetlb_memory.sh
+TEST_FILES += hugetlb_reparenting_test.sh
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
index 246d53a5d7f28..12754af00b39c 100755
--- a/tools/testing/selftests/mm/run_vmtests.sh
+++ b/tools/testing/selftests/mm/run_vmtests.sh
@@ -248,6 +248,9 @@  CATEGORY="hugetlb" run_test ./map_hugetlb
 CATEGORY="hugetlb" run_test ./hugepage-mremap
 CATEGORY="hugetlb" run_test ./hugepage-vmemmap
 CATEGORY="hugetlb" run_test ./hugetlb-madvise
+CATEGORY="hugetlb" run_test ./charge_reserved_hugetlb.sh -cgroup-v2
+CATEGORY="hugetlb" run_test ./hugetlb_reparenting_test.sh -cgroup-v2
+CATEGORY="hugetlb" run_test ./hugetlb-read-hwpoison
 
 nr_hugepages_tmp=$(cat /proc/sys/vm/nr_hugepages)
 # For this test, we need one and just one huge page