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

[Mike Rapoport <rppt@kernel.org>]

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

-- 
Sincerely yours,
Mike.