Re: [PATCH v3 5/5] selftests/mm: run_vmtests.sh: add missing tests

[Muhammad Usama Anjum]

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@xxxxxxx>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@xxxxxxxxxxxxx>
>> ---
>> 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
>>  
> 
> 

-- 
BR,
Muhammad Usama Anjum