[PATCH 1/2] selftests/mm: export get_free_hugepages()

[PATCH 1/2] selftests/mm: export get_free_hugepages()

[Breno Leitao]

get_free_hugepages() is helpful for other hugepage tests. Export it to
the common file (vm_util.c) to be reused.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/mm/hugetlb-madvise.c | 19 -------------------
 tools/testing/selftests/mm/vm_util.c         | 19 +++++++++++++++++++
 tools/testing/selftests/mm/vm_util.h         |  1 +
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/mm/hugetlb-madvise.c b/tools/testing/selftests/mm/hugetlb-madvise.c
index d55322df4b73..f32d99565c5e 100644
--- a/tools/testing/selftests/mm/hugetlb-madvise.c
+++ b/tools/testing/selftests/mm/hugetlb-madvise.c
@@ -36,25 +36,6 @@
 unsigned long huge_page_size;
 unsigned long base_page_size;
 
-unsigned long get_free_hugepages(void)
-{
-	unsigned long fhp = 0;
-	char *line = NULL;
-	size_t linelen = 0;
-	FILE *f = fopen("/proc/meminfo", "r");
-
-	if (!f)
-		return fhp;
-	while (getline(&line, &linelen, f) > 0) {
-		if (sscanf(line, "HugePages_Free:      %lu", &fhp) == 1)
-			break;
-	}
-
-	free(line);
-	fclose(f);
-	return fhp;
-}
-
 void write_fault_pages(void *addr, unsigned long nr_pages)
 {
 	unsigned long i;
diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index 558c9cd8901c..3082b40492dd 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -269,3 +269,22 @@ int uffd_unregister(int uffd, void *addr, uint64_t len)
 
 	return ret;
 }
+
+unsigned long get_free_hugepages(void)
+{
+	unsigned long fhp = 0;
+	char *line = NULL;
+	size_t linelen = 0;
+	FILE *f = fopen("/proc/meminfo", "r");
+
+	if (!f)
+		return fhp;
+	while (getline(&line, &linelen, f) > 0) {
+		if (sscanf(line, "HugePages_Free:      %lu", &fhp) == 1)
+			break;
+	}
+
+	free(line);
+	fclose(f);
+	return fhp;
+}
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index c7fa61f0dff8..c02990bbd56f 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -51,6 +51,7 @@ int uffd_register(int uffd, void *addr, uint64_t len,
 int uffd_unregister(int uffd, void *addr, uint64_t len);
 int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
 			      bool miss, bool wp, bool minor, uint64_t *ioctls);
+unsigned long get_free_hugepages(void);
 
 /*
  * On ppc64 this will only work with radix 2M hugepage size
-- 
2.34.1

[PATCH 2/2] selftests/mm: Add a new test for madv and hugetlb

[Breno Leitao]

Create a selftest that exercises the conflict between page faults and
madvise(MADV_DONTNEED) in the same huge page. Do it by running two
threads that touches the huge page and madvise(MADV_DONTNEED) at the same
time.

In case of a SIGBUS coming at pagefault, the test should fail, since we
hit the bug.

The test doesn't have a signal handler, and if it fails, it fails like
the following

  ----------------------------------
  running ./hugetlb_fault_after_madv
  ----------------------------------
  ./run_vmtests.sh: line 186: 595563 Bus error    (core dumped) "$@"
  [FAIL]

This selftest goes together with the fix of the bug[1] itself.

[1] https://lore.kernel.org/all/20231001005659.2185316-1-riel@surriel.com/#r

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 tools/testing/selftests/mm/Makefile           |  1 +
 .../selftests/mm/hugetlb_fault_after_madv.c   | 82 +++++++++++++++++++
 tools/testing/selftests/mm/run_vmtests.sh     |  4 +
 3 files changed, 87 insertions(+)
 create mode 100644 tools/testing/selftests/mm/hugetlb_fault_after_madv.c

diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 6a9fc5693145..e71ec9910c62 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -68,6 +68,7 @@ TEST_GEN_FILES += split_huge_page_test
 TEST_GEN_FILES += ksm_tests
 TEST_GEN_FILES += ksm_functional_tests
 TEST_GEN_FILES += mdwe_test
+TEST_GEN_FILES += hugetlb_fault_after_madv
 
 ifneq ($(ARCH),arm64)
 TEST_GEN_PROGS += soft-dirty
diff --git a/tools/testing/selftests/mm/hugetlb_fault_after_madv.c b/tools/testing/selftests/mm/hugetlb_fault_after_madv.c
new file mode 100644
index 000000000000..d6d38d443840
--- /dev/null
+++ b/tools/testing/selftests/mm/hugetlb_fault_after_madv.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include "vm_util.h"
+
+#define MMAP_SIZE (1 << 21)
+#define INLOOP_ITER 100
+
+char *huge_ptr;
+
+/* Touch the memory while it is being madvised() */
+void *touch(void *unused)
+{
+	char *ptr = (char *)huge_ptr;
+
+	if (!ptr) {
+		fprintf(stderr, "Failed to allocate memory\n");
+		perror("");
+	}
+
+	for (int i = 0; i < INLOOP_ITER; i++)
+		ptr[0] = '.';
+
+	return NULL;
+}
+
+void *madv(void *unused)
+{
+	usleep(rand() % 10);
+	if (!huge_ptr)
+		return NULL;
+
+	for (int i = 0; i < INLOOP_ITER; i++)
+		madvise(huge_ptr, MMAP_SIZE, MADV_DONTNEED);
+
+	return NULL;
+}
+
+int main(void)
+{
+	unsigned long free_hugepages;
+	pthread_t thread1, thread2;
+	/*
+	 * On kernel 6.4, we are able to reproduce the problem with ~1000
+	 * interactions
+	 */
+	int max = 10000;
+
+	srand(getpid());
+
+	free_hugepages = get_free_hugepages();
+	if (free_hugepages != 1) {
+		fprintf(stderr,
+			"This test needs one and only one page to execute. Got %lu\n",
+			free_hugepages);
+		exit(1);
+	}
+
+	while (max--) {
+		huge_ptr = mmap(NULL, MMAP_SIZE, PROT_READ | PROT_WRITE,
+				MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0);
+
+		if ((unsigned long)huge_ptr == -1) {
+			perror("Failed to allocate\n");
+			continue;
+		}
+
+		pthread_create(&thread1, NULL, madv, NULL);
+		pthread_create(&thread2, NULL, touch, NULL);
+
+		pthread_join(thread1, NULL);
+		pthread_join(thread2, NULL);
+		munmap(huge_ptr, MMAP_SIZE);
+	}
+
+	return 0;
+}
diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
index 3e2bc818d566..9f53f7318a38 100755
--- a/tools/testing/selftests/mm/run_vmtests.sh
+++ b/tools/testing/selftests/mm/run_vmtests.sh
@@ -221,6 +221,10 @@ CATEGORY="hugetlb" run_test ./hugepage-mremap
 CATEGORY="hugetlb" run_test ./hugepage-vmemmap
 CATEGORY="hugetlb" run_test ./hugetlb-madvise
 
+# For this test, we need one and just one huge page
+echo 1 > /proc/sys/vm/nr_hugepages
+CATEGORY="hugetlb" run_test ./hugetlb_fault_after_madv
+
 if test_selected "hugetlb"; then
 	echo "NOTE: These hugetlb tests provide minimal coverage.  Use"
 	echo "      https://github.com/libhugetlbfs/libhugetlbfs.git for"
-- 
2.34.1

Re: [PATCH 2/2] selftests/mm: Add a new test for madv and hugetlb

[Rik van Riel]

On Wed, 2023-10-04 at 10:11 -0700, Breno Leitao wrote:
> 
> +char *huge_ptr;
> +
> +/* Touch the memory while it is being madvised() */
> +void *touch(void *unused)
> +{
> +       char *ptr = (char *)huge_ptr;
> +
> +       if (!ptr) {
> +               fprintf(stderr, "Failed to allocate memory\n");
> +               perror("");
> +       }

I'm not sure this error message makes a lot of sense
away from where the huge page gets allocated.

> 
> +       while (max--) {
> +               huge_ptr = mmap(NULL, MMAP_SIZE, PROT_READ |
> PROT_WRITE,
> +                               MAP_PRIVATE | MAP_ANONYMOUS |
> MAP_HUGETLB, -1, 0);
> +
> +               if ((unsigned long)huge_ptr == -1) {
> +                       perror("Failed to allocate\n");
> +                       continue;
> +               }

Should the test case just exit with an error here, when
the allocation fails?

Looping around when it cannot get memory seems pointless,
but telling the user that the allocation fails, when it
should clearly have succeeded could be useful.

This test case certainly seems to do the trick in showing
whether the race between MADV_DONTNEED and page faults
exists in a particular kernel.


-- 
All Rights Reversed.

Re: [PATCH 2/2] selftests/mm: Add a new test for madv and hugetlb

[Breno Leitao]

On Wed, Oct 04, 2023 at 08:22:08PM -0400, Rik van Riel wrote:
> On Wed, 2023-10-04 at 10:11 -0700, Breno Leitao wrote:
> > 
> > +char *huge_ptr;
> > +
> > +/* Touch the memory while it is being madvised() */
> > +void *touch(void *unused)
> > +{
> > +       char *ptr = (char *)huge_ptr;
> > +
> > +       if (!ptr) {
> > +               fprintf(stderr, "Failed to allocate memory\n");
> > +               perror("");
> > +       }
> 
> I'm not sure this error message makes a lot of sense
> away from where the huge page gets allocated.

Right. I think I don't need this whole "if" clause at all. Let me remove
it.

> > 
> > +       while (max--) {
> > +               huge_ptr = mmap(NULL, MMAP_SIZE, PROT_READ |
> > PROT_WRITE,
> > +                               MAP_PRIVATE | MAP_ANONYMOUS |
> > MAP_HUGETLB, -1, 0);
> > +
> > +               if ((unsigned long)huge_ptr == -1) {
> > +                       perror("Failed to allocate\n");
> > +                       continue;
> > +               }
> 
> Should the test case just exit with an error here, when
> the allocation fails?

Yes, probably skip the test if we are not able to allocate the memory.
I just found I can use something as `ksft_exit_skip()` for this purpose.

Thanks for the great feedbacks!