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

[Mike Rapoport <rppt@kernel.org>]

On Thu, Apr 30, 2026 at 02:31:48PM -0400, Luiz Capitulino wrote:
> On 2026-04-28 16:42, Mike Rapoport wrote:
> > From: "Mike Rapoport (Microsoft)" <rppt@kernel.org>
> > 
> > A lot of tests require free HugeTLB pages. Some need just a few default
> > huge pages, some need a certain amount of memory available as HugeTLB, and
> > some just skip lots of tests if huge pages of all supported sizes are not
> > available.
> > 
> > This all resulted in a huge mess in run_vmtests.sh that sets up some huge
> > pages, adjusts them later and restores some of the settings if the stars
> > align.
> > 
> > Add APIs that allow saving the state of HugeTLB and setting up the desired
> > amount of HugeTLB pages.
> > 
> > Saving the state also registers atexit() callback and signal handler that
> > will ensure restoration of HugeTLB state.
> > 
> > Since many tests use both HugeTLB and THP, the atexit() callbacks and
> > signal handler are restoring both.
> > 
> > For kselftest_harness tests that run fixture setups and test in child
> > processes add a constructor that will save and restore settings in the main
> > process.
> > 
> > Signed-off-by: Mike Rapoport (Microsoft) <rppt@kernel.org>
> > ---
> >   .../testing/selftests/mm/hugepage_settings.c  | 178 +++++++++++++++---
> >   .../testing/selftests/mm/hugepage_settings.h  |  30 ++-
> >   2 files changed, 183 insertions(+), 25 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/mm/hugepage_settings.c b/tools/testing/selftests/mm/hugepage_settings.c
> > index 4ae7332b5e1b..9d31b53dbc67 100644
> > --- a/tools/testing/selftests/mm/hugepage_settings.c
> > +++ b/tools/testing/selftests/mm/hugepage_settings.c
> > @@ -306,31 +306,12 @@ void thp_restore_settings(void)
> >   		thp_write_settings(&saved_settings);
> >   }
> > -static void thp_restore_settings_atexit(void)
> > +static void __thp_save_settings(void)
> >   {
> > -	thp_restore_settings();
> > -}
> > -
> > -static void thp_restore_settings_sighandler(int sig)
> > -{
> > -	/* exit() will invoke the thp_restore_settings_atexit handler. */
> > -	exit(KSFT_FAIL);
> > -}
> > +	if (!thp_available())
> > +		return;
> > -void thp_save_settings(void)
> > -{
> >   	thp_read_settings(&saved_settings);
> > -
> > -	/*
> > -	 * setup exit hooks to make sure THP settings are restored on graceful
> > -	 * and error exits and signals
> > -	 */
> > -	atexit(thp_restore_settings_atexit);
> > -	signal(SIGTERM, thp_restore_settings_sighandler);
> > -	signal(SIGINT, thp_restore_settings_sighandler);
> > -	signal(SIGHUP, thp_restore_settings_sighandler);
> > -	signal(SIGQUIT, thp_restore_settings_sighandler);
> > -
> >   	thp_settings_saved = true;
> >   }
> > @@ -399,11 +380,31 @@ bool thp_is_enabled(void)
> >   	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 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.

-- 
Sincerely yours,
Mike.