Received-SPF: pass (google.com: domain of linux-kernel+bounces-48086-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <ab7523db-f032-4f99-8090-785f126a3ae2@arm.com>
Date: Thu, 1 Feb 2024 12:11:32 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 5/5] selftests/mm: run_vmtests.sh: add missing tests
Content-Language: en-GB
To: Muhammad Usama Anjum <usama.anjum@collabora.com>,
 Andrew Morton <akpm@linux-foundation.org>, Shuah Khan <shuah@kernel.org>
Cc: kernel@collabora.com, linux-mm@kvack.org,
 linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240125154608.720072-1-usama.anjum@collabora.com>
 <20240125154608.720072-6-usama.anjum@collabora.com>
From: Ryan Roberts <ryan.roberts@arm.com>
In-Reply-To: <20240125154608.720072-6-usama.anjum@collabora.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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
>