Received-SPF: pass (google.com: domain of linux-kernel+bounces-35468-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <4bf9bf87-0622-4824-9026-d7ab5839f433@collabora.com>
Date: Tue, 23 Jan 2024 19:45:08 +0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>, kernel@collabora.com,
 linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
 linux-kernel@vger.kernel.org, Andrew Morton <akpm@linux-foundation.org>,
 Shuah Khan <shuah@kernel.org>
Subject: Re: [PATCH v2 1/2] selftests/mm: run_vmtests.sh: add missing tests
Content-Language: en-US
To: Ryan Roberts <ryan.roberts@arm.com>
References: <20240123073615.920324-1-usama.anjum@collabora.com>
 <e92f7c49-5268-421e-a017-af268c845b1b@arm.com>
From: Muhammad Usama Anjum <usama.anjum@collabora.com>
In-Reply-To: <e92f7c49-5268-421e-a017-af268c845b1b@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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
> 
> 

-- 
BR,
Muhammad Usama Anjum