Re: [PATCH v3 01/13] selftests/mm: restore default nr_hugepages value during cleanup in charge_reserved_hugetlb.sh

[Sayali Patil <sayalip@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]


On 01/04/26 20:22, Sayali Patil wrote:
> On 27/03/26 12: 45, Sayali Patil wrote: During cleanup, the value of 
> /proc/sys/vm/nr_hugepages is currently being set to 0. At the end of 
> the test, if all tests pass, the original nr_hugepages value is 
> restored. However, if any test fails, it
> ZjQcmQR
> ZjQcmQRYFpfptBannerEnd
> On 27/03/26 12:45, Sayali Patil wrote:
>> During cleanup, the value of /proc/sys/vm/nr_hugepages is currently being
>> set to 0.  At the end of the test, if all tests pass, the original
>> nr_hugepages value is restored.  However, if any test fails, it remains
>> set to 0.
>> With this patch, we ensure that the original nr_hugepages value is
>> restored during cleanup, regardless of whether the test passes or fails.
>>
>> Fixes: 7d695b1c3695b ("selftests/mm: save and restore nr_hugepages value")
>> Reviewed-by: Zi Yan<ziy@nvidia.com>
>> Reviewed-by: David Hildenbrand (Arm)<david@kernel.org>
>> Tested-by: Venkat Rao Bagalkote<venkat88@linux.ibm.com>
>> Signed-off-by: Sayali Patil<sayalip@linux.ibm.com>
>> ---
>>   tools/testing/selftests/mm/charge_reserved_hugetlb.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
>> index 447769657634..c9fe68b6fcf9 100755
>> --- a/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
>> +++ b/tools/testing/selftests/mm/charge_reserved_hugetlb.sh
>> @@ -65,7 +65,7 @@ function cleanup() {
>>     if [[ -e $cgroup_path/hugetlb_cgroup_test2 ]]; then
>>       rmdir $cgroup_path/hugetlb_cgroup_test2
>>     fi
>> -  echo 0 >/proc/sys/vm/nr_hugepages
>> +  echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages
>>     echo CLEANUP DONE
>>   }
>>   
Previous reply was not formatted properly so sending it again.

AI review question
> Does this cause excessive memory allocation churn during test execution?
> Since cleanup() is invoked before and after every test iteration, restoring
> the original nr_hugepages here (which could be thousands of pages) and then
> immediately setting it back to 10 in the next test case forces the kernel
> to repeatedly allocate and free many hugepages for every single test case.
> Also, does this reliably restore nr_hugepages on test failures?
> The test script runs with "set -e" active. If a test fails while the
> background write_to_hugetlbfs process is still running, commands earlier
> in cleanup() like "rmdir /mnt/huge" can fail with EBUSY because the
> directory is still a mounted filesystem.
> Due to "set -e", this failure causes the script to immediately exit,
> completely bypassing this restore command at the end of cleanup().
> Would it be better to restore the original value once at the very end.
>   

Yes, it is better to use an EXIT trap here to avoid unnecessary allocation
churn and to ensure the original value is reliably restored on all exit
paths.

A similar change can also be applied to |hugetlb_reparenting_test.sh|. The
test modifies |nr_hugepages| during execution and restores it from
|cleanup()|, while also reconfiguring it again in |setup()|, which is
invoked multiple times across the test flow. This can lead to repeated
allocation and freeing of hugepages.

I will prepare a patch for both tests and include it in v4.

Thanks,
Sayali



[-- Attachment #2: Type: text/html, Size: 6719 bytes --]