Re: [PATCH v3] selftests/mm: hugetlb_madv_vs_map: add underflow test

[Mike Rapoport <rppt@kernel.org>]

On Wed, Sep 02, 2026 at 06:31:08PM +0200, Guillaume Morin wrote:
> Add a test that checks for underflows when a parent unmaps the page
> first. Also check that when the child exits the reserve count is
> correct.
> 
> Link: https://lore.kernel.org/all/alEJkwn5VlTTH_ZX@bender.morinfr.org/
> Signed-off-by: Guillaume Morin <guillaume@morinfr.org
> Cc: Breno Leitao <leitao@debian.org>
> Cc: Mike Rapoport <rppt@kernel.org>
> ---
> v3: address Mike's comments
> v2: refactor tests into 2 functions
> 
>  .../testing/selftests/mm/hugepage_settings.c  |   9 ++
>  .../testing/selftests/mm/hugepage_settings.h  |   1 +
>  .../selftests/mm/hugetlb_madv_vs_map.c        | 149 +++++++++++++++---
>  3 files changed, 138 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/hugepage_settings.c b/tools/testing/selftests/mm/hugepage_settings.c
> index d7917dce3aba..584054736ce9 100644
> --- a/tools/testing/selftests/mm/hugepage_settings.c
> +++ b/tools/testing/selftests/mm/hugepage_settings.c
> @@ -449,6 +449,15 @@ unsigned long hugetlb_free_pages(unsigned long size)
>  	return read_num(path);
>  }
>  
> +unsigned long hugetlb_nr_resv_pages(unsigned long size)
> +{
> +	char path[PATH_MAX];
> +
> +	hugetlb_sysfs_path(path, sizeof(path), size, "resv_hugepages");
> +
> +	return read_num(path);
> +}
> +
>  static bool __hugetlb_setup(unsigned long size, unsigned long nr)
>  {
>  	unsigned long free = hugetlb_free_pages(size);
> diff --git a/tools/testing/selftests/mm/hugepage_settings.h b/tools/testing/selftests/mm/hugepage_settings.h
> index 726c73c43c05..548e9d288d1d 100644
> --- a/tools/testing/selftests/mm/hugepage_settings.h
> +++ b/tools/testing/selftests/mm/hugepage_settings.h
> @@ -98,6 +98,7 @@ unsigned long default_huge_page_size(void);
>  unsigned long hugetlb_nr_pages(unsigned long size);
>  void hugetlb_set_nr_pages(unsigned long size, unsigned long nr);
>  unsigned long hugetlb_free_pages(unsigned long size);
> +unsigned long hugetlb_nr_resv_pages(unsigned long size);
>  
>  static inline void hugetlb_save_settings(void)
>  {
> diff --git a/tools/testing/selftests/mm/hugetlb_madv_vs_map.c b/tools/testing/selftests/mm/hugetlb_madv_vs_map.c
> index f94549efcc6f..c944d99d5523 100644
> --- a/tools/testing/selftests/mm/hugetlb_madv_vs_map.c
> +++ b/tools/testing/selftests/mm/hugetlb_madv_vs_map.c
> @@ -3,25 +3,16 @@
>   * A test case that must run on a system with one and only one huge page available.
>   *	# echo 1 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
>   *
> - * During setup, the test allocates the only available page, and starts three threads:
> - *  - thread1:
> - *	* madvise(MADV_DONTNEED) on the allocated huge page
> - *  - thread 2:
> - *	* Write to the allocated huge page
> - *  - thread 3:
> - *	* Try to allocated an extra huge page (which must not available)
> - *
> - *  The test fails if thread3 is able to allocate a page.
> - *
> - *  Touching the first page after thread3's allocation will raise a SIGBUS
> - *
>   *  Author: Breno Leitao <leitao@debian.org>
>   */
> +#include <limits.h>
>  #include <pthread.h>
> +#include <signal.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <sys/mman.h>
>  #include <sys/types.h>
> +#include <sys/wait.h>
>  #include <unistd.h>
>  
>  #include "vm_util.h"
> @@ -74,7 +65,22 @@ void *map_extra(void *unused)
>  	return NULL;
>  }
>  
> -int main(void)
> +/* During setup in main, the only available page was allocated. This test then
> + * starts three threads:
> + *
> + * - thread1:
> + *	* madvise(MADV_DONTNEED) on the allocated huge page
> + *  - thread 2:
> + *	* Write to the allocated huge page
> + *  - thread 3:
> + *	* Try to allocated an extra huge page (which must not available)
> + *
> + *  The test fails if thread3 is able to allocate a page.
> + *
> + *  Touching the first page after thread3's allocation will raise a SIGBUS
> + */
> +

Nit: no need for an empty line here

> +void test_madv_vs_map(void)
>  {
>  	pthread_t thread1, thread2, thread3;
>  	void *ret;
> @@ -85,13 +91,6 @@ int main(void)
>  	 */
>  	int max = 10;
>  
> -	ksft_print_header();
> -	ksft_set_plan(1);
> -
> -	if (!hugetlb_setup_default_exact(1))
> -		ksft_exit_skip("This test needs one and only one page to execute. Got %lu\n",
> -			       hugetlb_free_default_pages());
> -
>  	mmap_size = default_huge_page_size();
>  
>  	while (max--) {
> @@ -100,7 +99,7 @@ int main(void)
>  				-1, 0);
>  
>  		if ((unsigned long)huge_ptr == -1)
> -			ksft_exit_fail_msg("Failed to allocate huge page\n");
> +			ksft_exit_fail_perror("Failed to allocate huge page\n");
>  
>  		pthread_create(&thread1, NULL, madv, NULL);
>  		pthread_create(&thread2, NULL, touch, NULL);
> @@ -120,5 +119,113 @@ int main(void)
>  	}
>  
>  	ksft_test_result_pass("No unexpected huge page allocations\n");
> +}
> +
> +/*  We create a child process, then unmap the page in the parent while the child
> + *  waits and verify that there is no underflow of the reserved count.
> + */
> +

And here

Please add a sentence about the second test this function implements.

> +void test_underflow(void)
> +{
> +	pid_t pid;
> +	int pipe_fds[2];
> +	unsigned long nr_reserved = 0;
> +
> +	huge_ptr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0);
> +
> +	if ((unsigned long)huge_ptr == -1)
> +		ksft_exit_fail_perror("Failed to allocate huge page\n");
> +
> +	nr_reserved = hugetlb_nr_resv_pages(default_huge_page_size());
> +	if (nr_reserved != 1)
> +		ksft_exit_fail_msg("Unexpected number of reserved pages: %lu, expected 1\n",
> +				   nr_reserved);
> +
> +	/* Force the fault to ensure the reservation is consumed */
> +	*huge_ptr = 0;
> +	nr_reserved = hugetlb_nr_resv_pages(default_huge_page_size());
> +	if (nr_reserved != 0)
> +		ksft_exit_fail_msg("Unexpected number of reserved pages: %lu, expected 0\n",
> +				   nr_reserved);
> +
> +	if (pipe(pipe_fds) != 0)
> +		ksft_exit_fail_perror("pipe failed");
> +
> +	pid = fork();
> +	if (pid < 0)
> +		ksft_exit_fail_perror("fork failed");
> +
> +	if (pid == 0) {
> +		/* Child: Simply wait for the parent */
> +		char b;
> +
> +		close(pipe_fds[1]);
> +		if (read(pipe_fds[0], &b, 1) < 0)
> +			ksft_perror("child read failed");
> +		/* Let the parent do the cleanup */
> +		_exit(0);
> +	}
> +
> +	/* Parent */
> +	close(pipe_fds[0]);
> +
> +	/* First unmap, this will close the vma */
> +	if (munmap(huge_ptr, mmap_size) != 0) {
> +		ksft_perror("munmap failed");
> +		goto err_cleanup;

This will perror(), return and print that there's a mismatch between the
plan and the tests.
Maybe add a ksft_exit_fail() in the end of err_cleanup?

> +	}
> +
> +	nr_reserved = hugetlb_nr_resv_pages(default_huge_page_size());
> +	if (nr_reserved == ULONG_MAX) {
> +		ksft_test_result_fail("After the munmap, HugePages_Rsvd underflowed!\n");
> +		goto err_cleanup;
> +	}
> +	if (nr_reserved == 0) {
> +		ksft_test_result_pass("Underflow not present!\n");
> +	} else {
> +		ksft_test_result_fail("Unexpected HugePages_Rsvd=%ld after munmap, should be 0 or -1.  Repeat the test\n",
> +				      nr_reserved);
> +		goto err_cleanup;
> +	}
> +	/* Make the child exit, this should restore HugePages_Rsvd to 0 */
> +	if (write(pipe_fds[1], &nr_reserved, 1) < 0) {
> +		/* If write failed, the child is likely already gone */
> +		ksft_exit_fail_perror("write failed");
> +	}
> +	close(pipe_fds[1]);
> +	if (waitpid(pid, NULL, 0) <= 0) {
> +		ksft_exit_fail_msg("waitpid failed");

Unlike _perror(), ksft_exit_fail_msg does not add '\n'. The same for
occurrences below.

> +	}
> +
> +	nr_reserved = hugetlb_nr_resv_pages(default_huge_page_size());
> +	if (nr_reserved == 0) {
> +		ksft_test_result_pass("After the child dies, HugePages_Rsvd is properly set to 0\n");
> +	} else {
> +		ksft_exit_fail_msg("Unexpected HugePages_Rsvd=%ld after the child termination munmap, should be 0 or -1.  Repeat the test\n",

Here we expect 0 reserved pages, I think.

Not sure if "repeat the test" adds anything, I'd simply drop it.

> +				   nr_reserved);
> +	}
> +	return;

And here I'd put an empty line :)

> +err_cleanup:
> +	if (write(pipe_fds[1], &nr_reserved, 1) < 0) {
> +		ksft_exit_fail_perror("write failed");
> +	}
> +	if (waitpid(pid, NULL, 0) <= 0) {
> +		ksft_exit_fail_perror("waitpid failed");
> +	}
> +}
> +
> +int main(void)
> +{
> +	ksft_print_header();
> +	ksft_set_plan(3);
> +
> +	if (!hugetlb_setup_default_exact(1))
> +		ksft_exit_skip("This test needs one and only one page to execute. Got %lu\n",
> +			       hugetlb_free_default_pages());
> +
> +	test_madv_vs_map();
> +	test_underflow();
> +
>  	ksft_finished();
>  }
> -- 
> 2.39.1
> 
> -- 
> Guillaume Morin <guillaume@morinfr.org>
> 

-- 
Sincerely yours,
Mike.