Re: [PATCH v3 27/54] selftests/mm: hugepage_settings: add APIs for HugeTLB setup and teardown

[Luiz Capitulino <luizcap@redhat.com>]

On 2026-05-02 00:36, Mike Rapoport wrote:
> On Fri, May 01, 2026 at 03:30:18PM -0400, Luiz Capitulino wrote:
>> On 2026-05-01 11:25, Mike Rapoport wrote:
>>>>>     	return mode == 1 || mode == 3;
>>>>>     }
>>>>> +#define HUGETLB_MAX_NR_PAGESIZES 10
>>>>> +struct hugetlb_settings {
>>>>> +	unsigned long free_hugepages[HUGETLB_MAX_NR_PAGESIZES];
>>>>
>>>> It looks like we don't actually use this in the API or maybe I'm missing
>>>> something?
>>>
>>> Yes, it's used only in later patches.
>>
>> I don't see it, the patch below builds fine (again, sorry in advance if
>> I'm missing something obvious):
> 
> You are right, I misunderstood you comment and I thought you referred to
> entire API being unused in this patch.
> 
> free_hugepages is not used and it should not be a part of the saved state.

Thanks, I'll try to be more specific next time.

>   
>> diff --git a/tools/testing/selftests/mm/hugepage_settings.c b/tools/testing/selftests/mm/hugepage_settings.c
>> index 097ba085f423..a023cfeb69c5 100644
>> --- a/tools/testing/selftests/mm/hugepage_settings.c
>> +++ b/tools/testing/selftests/mm/hugepage_settings.c
>> @@ -341,7 +341,6 @@ bool thp_is_enabled(void)
>>   #define HUGETLB_MAX_NR_PAGESIZES 10
>>   struct hugetlb_settings {
>> -	unsigned long free_hugepages[HUGETLB_MAX_NR_PAGESIZES];
>>   	unsigned long nr_hugepages[HUGETLB_MAX_NR_PAGESIZES];
>>   	unsigned long sizes[HUGETLB_MAX_NR_PAGESIZES];
>>   	unsigned long default_size;
>> @@ -509,7 +508,6 @@ static void __hugetlb_save_settings(void)
>>   		if (!sz)
>>   			continue;
>> -		settings->free_hugepages[i] = hugetlb_free_pages(sz);
>>   		settings->nr_hugepages[i] = hugetlb_nr_pages(sz);
>>   	}
>>
>>> I felt it would be too churny to add users in this patch.
>>>> Otherwise, the API looks great to me. But if you allow me to bikeshed :)
>>>>
>>>> I think I'd do:
>>>>
>>>> struct hugetlb_state {
>>>> 	unsigned long nr_hugepages;
>>>> 	unsigned long size;
>>>> };
>>>>
>>>> struct hugetlb_settings {
>>>> 	struct hugetlb_state hstate[HUGETLB_MAX_PAGESIZES];
>>>> 	unsigned long default_size;
>>>> 	int nr_sizes;
>>>> };
>>>
>>> It might be slightly nicer abstraction but ...
>>>>> +static void __hugetlb_save_settings(void)
>>>>> +{
>>>>> +	struct hugetlb_settings *settings = &hugetlb_saved_settings;
>>>>> +	int nr_sizes;
>>>>> +
>>>>> +	settings->default_size = default_huge_page_size();
>>>>> +	if (!settings->default_size)
>>>>> +		return;
>>>>> +
>>>>> +	nr_sizes = detect_hugetlb_page_sizes(settings->sizes,
>>>>> +					     HUGETLB_MAX_NR_PAGESIZES);
>>>
>>> ... this immediately becomes more complex.
>>>
>>>>> +	if (!nr_sizes) {
>>>>> +		settings->default_size = 0;
>>>>> +		return;
>>>>> +	}
>>>>> +
>>>>> +	for (int i = 0; i < nr_sizes; i++) {
>>>>> +		unsigned long sz = settings->sizes[i];
>>>>> +
>>>>> +		if (!sz)
>>>>> +			continue;
>>>>> +
>>>>> +		settings->free_hugepages[i] = hugetlb_free_pages(sz);
>>>>> +		settings->nr_hugepages[i] = hugetlb_nr_pages(sz);
>>>
>>> And this as well.
>>
>> We'd need a little more code to handle those cases, may or may not be
>> worth it. But enough bikesheding from my side :) the series is great as
>> is.
>>
>>
>