Skip to contentSkip to conversation

kselftest: mm: fix some failure of split_huge_page_test

PATCH Superseded Yeoreum Yun 2 participants 30 messages Sep 7 – Sep 10

Subsystems (4)

v5 of 9 Superseded Latest is v9 →

lore
Share & tools
lore
#0 lore raw

v5 cover kselftest: mm: fix some failure of split_huge_page_test

split_huge_page_test can fail for the following reasons:

1. During the test, khugepaged may collapse previously split pages again, causing intermittent failures.

2. Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign(). The underlying VMA may start at a different address from the aligned address returned by memalign(). Moreover, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because it already has the same advice.

This causes the test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Address these issues by applying MADV_NOHUGEPAGE after faulting in the huge page, preventing khugepaged from collapsing it again, and instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

Also, current usage of memalign() would result memory area may unexpectedly merge with an adjacent VMA, causing tests that inspect it through /proc/self/smaps to fail.

Eliminate this potential source of test flakiness by introducing alloc_isolated_mem(), which places guard regions before and after the allocated area to prevent unexpected VMA merging.

This patch based on mm-unstable

---
Changes in v5:
- rename madv_nohuge() to disable_khugepaged().
- Link to v4: https://lore.kernel.org/r/[email protected]

Changes in v4:
- reparse commit message.
- introduce alloc_isolated_mem().
- make madv_nohuge() helper according to suggestion.
- Link to v3: https://lore.kernel.org/r/[email protected]

Changes in v3:
- change message in case of failure of madvise() with MADV_NOHUGEPAGE.
- fix collapse_single_mthp (mthp_khugepaged:anon) test case failure.
- Link to v2: https://lore.kernel.org/all/[email protected]/

Changes in v2:
- rebase to mm-unstable.
- add message in case of failure of madvise() with MADV_NOHUGEPAGE.
- fix wrong setup expected_huge in check_huge_shmem().
- Link to v1: https://lore.kernel.org/r/[email protected]

---
Yeoreum Yun (3):
kselftest: mm: prevent random failure of huge page split for khugepaged
kselftest: mm: replace usage of /proc/self/smaps for check_huge_xxx() helper
kselftest: mm: introduce alloc_isolated_mem()

 tools/testing/selftests/mm/pagemap_ioctl.c        |  18 +-
 tools/testing/selftests/mm/soft-dirty.c           |   6 +-
 tools/testing/selftests/mm/split_huge_page_test.c |  27 ++-
 tools/testing/selftests/mm/vm_util.c              | 212 ++++++++++++++++------
 tools/testing/selftests/mm/vm_util.h              |   3 +
 5 files changed, 192 insertions(+), 74 deletions(-)

---
base-commit: d118502628f8b673be9023db8bdf878f64a7ed45
change-id: 20260820-fix_split-f44939ec44b8

Best regards,

Signature · 2 lines
Sincerely,
Yeoreum Yun
#1 lore raw

PATCH v5 1/3 kselftest: mm: prevent random failure of huge page split for khugepaged

There're some random failure for split_huge_page_test when khugepaged collapses pages into pmd again which had split by the test.

Prevent the khugepaged's collapses for split page by setting the mapped pmd-huge-page with MADV_NOHUGEPAGE before split.

---
 tools/testing/selftests/mm/split_huge_page_test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
tools/testing/selftests/mm/split_huge_page_test.c +19 -0
index c01d227d7fd6..cba327f77873 100644
@@ -167,6 +167,18 @@ static char *allocate_zero_filled_hugepage(size_t len)
 	return result;
 }
 
+static void disable_khugepaged(void *addr, size_t len)
+{
+	/*
+	 * Disables khugepaged from collapsing THPs in range, existing THP
+	 * pages remain.
+	 */
+	if (!madvise(addr, len, MADV_NOHUGEPAGE))
+		return;
+
+	ksft_exit_fail_msg("MADV_NOHUGEPAGE failed, err=%d\n", errno);
+}
+
 static void verify_rss_anon_split_huge_page_all_zeroes(char *one_page, int nr_hpages, size_t len)
 {
 	unsigned long rss_anon_before, rss_anon_after;
@@ -179,6 +191,8 @@ static void verify_rss_anon_split_huge_page_all_zeroes(char *one_page, int nr_hp
 	if (!rss_anon_before)
 		ksft_exit_fail_msg("No RssAnon is allocated before split\n");
 
+	disable_khugepaged(one_page, len);
+
 	/* split all THPs */
 	write_debugfs(PID_FMT, getpid(), (uint64_t)one_page,
 		      (uint64_t)one_page + len, 0);
@@ -226,6 +240,8 @@ static void split_pmd_thp_to_order(int order)
 	if (!check_huge_anon(one_page, 4 * pmd_pagesize, 4, pmd_pagesize))
 		ksft_exit_fail_msg("No THP is allocated\n");
 
+	disable_khugepaged(one_page, len);
+
 	/* split all THPs */
 	write_debugfs(PID_FMT, getpid(), (uint64_t)one_page,
 		(uint64_t)one_page + len, order);
@@ -274,6 +290,8 @@ static void split_pte_mapped_thp(void)
 		goto out;
 	}
 
+	disable_khugepaged(thp_area, thp_area_size);
+
 	/*
 	 * To challenge spitting code, we will mremap a single page of each
 	 * THP (page[i] of thp[i]) in the thp_area into page_area. This will
@@ -541,6 +559,7 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
 		ksft_test_result_skip("Pagecache folio split skipped\n");
 		return -2;
 	}
+	disable_khugepaged(*addr, fd_size);
 	return 0;
 err_out_close:
 	close(*fd);
Signature · 1 line
2.43.0
maintainer Acked-by on v5 1/3
#2 lore raw

Re: [PATCH v5 1/3] kselftest: mm: prevent random failure of huge page split for khugepaged

Quoted 6 lines · Yeoreum Yun

There're some random failure for split_huge_page_test when khugepaged collapses pages into pmd again which had split by the test.

Prevent the khugepaged's collapses for split page by setting the mapped pmd-huge-page with MADV_NOHUGEPAGE before split.

Quoted 6 lines

---

Signature · 3 lines
Cheers,

David
#3 lore raw

PATCH v5 2/3 kselftest: mm: replace usage of /proc/self/smaps for check_huge_xxx() helper

In reply to Yeoreum Yun

Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign().

The underlying VMA may start at a different address from the aligned address returned by memalign(). Furthermore, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because the flag is already set.

This causes split_huge_page_test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

---
 tools/testing/selftests/mm/vm_util.c | 145 ++++++++++++++++++++++-------------
 tools/testing/selftests/mm/vm_util.h |   1 +
 2 files changed, 92 insertions(+), 54 deletions(-)
tools/testing/selftests/mm/vm_util.c +91 -54
index 4821a3563036..dc62ce84e143 100644
@@ -351,31 +351,13 @@ char *__get_smap_entry(void *addr, const char *pattern, char *buf, size_t len)
 	return entry;
 }
 
-static bool __check_pmd_huge(void *addr, char *pattern, int nr_hpages,
-		  uint64_t hpage_size)
-{
-	char buffer[MAX_LINE_LENGTH];
-	uint64_t thp = -1;
-	char *entry;
-
-	entry = __get_smap_entry(addr, pattern, buffer, sizeof(buffer));
-	if (!entry)
-		goto err_out;
-
-	if (sscanf(entry, "%9" SCNu64 " kB", &thp) != 1)
-		ksft_exit_fail_msg("Reading smap error\n");
-
-err_out:
-	return thp == (nr_hpages * (hpage_size >> 10));
-}
-
-static bool check_large_folios(void *addr, size_t len, int nr_hpages,
-		uint64_t hpage_size)
+static bool check_large_folios(int pagemap_fd, int kpageflags_fd,
+			       void *addr, size_t len, int nr_hpages,
+			       uint64_t hpage_size)
 {
 	int order = 0, pagesize = getpagesize();
 	unsigned int nr_pages = hpage_size / pagesize;
 	int orders[MAX_NR_ORDERS], status;
-	int pagemap_fd, kpageflags_fd;
 	bool ret = false;
 
 	if (!nr_pages)
@@ -386,15 +368,6 @@ static bool check_large_folios(void *addr, size_t len, int nr_hpages,
 		ksft_exit_fail_msg("invalid order\n");
 
 	memset(orders, 0, sizeof(int) * MAX_NR_ORDERS);
-	pagemap_fd = open(PAGEMAP_PATH, O_RDONLY);
-	if (pagemap_fd == -1)
-		ksft_exit_fail_msg("read pagemap fail\n");
-
-	kpageflags_fd = open(KPAGEFLAGS_PATH, O_RDONLY);
-	if (kpageflags_fd == -1) {
-		close(pagemap_fd);
-		ksft_exit_fail_msg("read kpageflags fail\n");
-	}
 
 	status = gather_folio_orders(addr, len, pagemap_fd,
 			kpageflags_fd, orders, MAX_NR_ORDERS);
@@ -405,48 +378,112 @@ static bool check_large_folios(void *addr, size_t len, int nr_hpages,
 		ret = true;
 
 out:
-	close(pagemap_fd);
-	close(kpageflags_fd);
 	return ret;
 }
 
-bool check_huge_anon(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
-{
-	uint64_t pmd_pagesize = read_pmd_pagesize();
-
-	if (!pmd_pagesize)
-		ksft_exit_fail_msg("reading PMD pagesize failed\n");
+enum check_huge_type {
+	CHECK_HUGE_ANON,
+	CHECK_HUGE_FILE,
+	CHECK_HUGE_SHMEM,
+};
 
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "AnonHugePages: ", nr_hpages, hpage_size);
+static bool check_huge_type(uint64_t categories, uint64_t kpageflags,
+			    enum check_huge_type type)
+{
+	const bool file = categories & PAGE_IS_FILE;
+	const bool swapbacked = kpageflags & KPF_SWAPBACKED;
+
+	switch (type) {
+	case CHECK_HUGE_ANON:
+		return !file;
+	case CHECK_HUGE_FILE:
+		return file && !swapbacked;
+	case CHECK_HUGE_SHMEM:
+		return file & swapbacked;
+	}
 
-	return check_large_folios(addr, len, nr_hpages, hpage_size);
+	return false;
 }
 
-bool check_huge_file(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
+static bool __check_huge(void *addr, size_t len, int nr_hpages,
+			 uint64_t hpage_size, enum check_huge_type type)
 {
-	uint64_t pmd_pagesize = read_pmd_pagesize();
+	bool ret = false;
+	int pagemap_fd, kpageflags_fd;
+	int nr_pmd_mappings = 0;
+	uint64_t pmd_pagesize, scan_mapping_size;
+	uint64_t categories, kpf;
+	unsigned long pfn;
+	bool check_pmd_mapping, allow_nonpresent;
+	char *start = addr;
+	char *end = start + len;
 
+	pmd_pagesize = read_pmd_pagesize();
 	if (!pmd_pagesize)
 		ksft_exit_fail_msg("reading PMD pagesize failed\n");
 
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "FilePmdMapped:", nr_hpages, hpage_size);
+	check_pmd_mapping = (hpage_size == pmd_pagesize);
+	scan_mapping_size = (nr_hpages > 0) ? hpage_size : psize();
+	/* Some mTHP tests check a partially populated PMD-sized range. */
+	allow_nonpresent = (uint64_t)nr_hpages * hpage_size < len;
 
-	return check_large_folios(addr, len, nr_hpages, hpage_size);
+	pagemap_fd = open(PAGEMAP_PATH, O_RDONLY);
+	if (pagemap_fd < 0)
+		ksft_exit_fail_msg("open pagemap fail\n");
+
+	kpageflags_fd = open(KPAGEFLAGS_PATH, O_RDONLY);
+	if (kpageflags_fd < 0) {
+		close(pagemap_fd);
+		ksft_exit_fail_msg("open kpageflags fail\n");
+	}
+
+	if (!check_pmd_mapping &&
+	    !check_large_folios(pagemap_fd, kpageflags_fd,
+				addr, len, nr_hpages, hpage_size))
+		goto out;
+
+	for (; start < end; start += scan_mapping_size) {
+		categories = pagemap_scan_get_categories(pagemap_fd, start);
+		pfn = pagemap_get_pfn(pagemap_fd, start);
+		if (pfn == -1UL) {
+			if (!allow_nonpresent)
+				goto out;
+			else
+				continue;
+		}
+		if (pageflags_get(pfn, kpageflags_fd, &kpf))
+			ksft_exit_fail_msg("read kpageflags: %s\n", strerror(errno));
+		if (check_pmd_mapping && (categories & PAGE_IS_HUGE))
+			nr_pmd_mappings++;
+		if (kpf & KPF_COMPOUND_TAIL)
+			continue;
+		if (!check_huge_type(categories, kpf, type))
+			goto out;
+	}
+
+	if (check_pmd_mapping && (nr_pmd_mappings != nr_hpages))
+		goto out;
+	ret = true;
+
+out:
+	close(pagemap_fd);
+	close(kpageflags_fd);
+	return ret;
 }
 
-bool check_huge_shmem(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
+bool check_huge_anon(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
 {
-	uint64_t pmd_pagesize = read_pmd_pagesize();
-
-	if (!pmd_pagesize)
-		ksft_exit_fail_msg("reading PMD pagesize failed\n");
+	return __check_huge(addr, len, nr_hpages, hpage_size, CHECK_HUGE_ANON);
+}
 
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "ShmemPmdMapped:", nr_hpages, hpage_size);
+bool check_huge_file(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
+{
+	return __check_huge(addr, len, nr_hpages, hpage_size, CHECK_HUGE_FILE);
+}
 
-	return check_large_folios(addr, len, nr_hpages, hpage_size);
+bool check_huge_shmem(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
+{
+	return __check_huge(addr, len, nr_hpages, hpage_size, CHECK_HUGE_SHMEM);
 }
 
 int64_t allocate_transhuge(void *ptr, int pagemap_fd)
tools/testing/selftests/mm/vm_util.h +1 -0
index 9a49af88702e..f19bd17817e5 100644
@@ -18,6 +18,7 @@
 #define PM_SWAP                       BIT_ULL(62)
 #define PM_PRESENT                    BIT_ULL(63)
 
+#define KPF_SWAPBACKED                BIT_ULL(14)
 #define KPF_COMPOUND_HEAD             BIT_ULL(15)
 #define KPF_COMPOUND_TAIL             BIT_ULL(16)
 #define KPF_HWPOISON                  BIT_ULL(19)
Signature · 1 line
2.43.0
#4 lore raw

Re: [PATCH v5 2/3] kselftest: mm: replace usage of /proc/self/smaps for check_huge_xxx() helper

Quoted 37 lines · Yeoreum Yun

Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign().

The underlying VMA may start at a different address from the aligned address returned by memalign(). Furthermore, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because the flag is already set.

This causes split_huge_page_test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

---
 tools/testing/selftests/mm/vm_util.c | 145 ++++++++++++++++++++++-------------
 tools/testing/selftests/mm/vm_util.h |   1 +
 2 files changed, 92 insertions(+), 54 deletions(-)
tools/testing/selftests/mm/vm_util.c +0 -0
index 4821a3563036..dc62ce84e143 100644

[...]

Quoted 7 lines
tools/testing/selftests/mm/vm_util.c +4 -2
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "AnonHugePages: ", nr_hpages, hpage_size);
+static bool check_huge_type(uint64_t categories, uint64_t kpageflags,
+			    enum check_huge_type type)
+{
+	const bool file = categories & PAGE_IS_FILE;

tools/testing/selftests/mm/vm_util.c static bool check_large_folios(void *addr, size_t len, int nr_hpages, · line 403

Quoted 10 lines
tools/testing/selftests/mm/vm_util.c +10 -0
+	const bool swapbacked = kpageflags & KPF_SWAPBACKED;
+
+	switch (type) {
+	case CHECK_HUGE_ANON:
+		return !file;
+	case CHECK_HUGE_FILE:
+		return file && !swapbacked;
+	case CHECK_HUGE_SHMEM:
+		return file & swapbacked;
+	}

Why do we even need CHECK_HUGE_SHMEM?

That's really just a legacy thing for using smaps to identify huge pages.

It's sufficient to identify CHECK_HUGE_FILE if you know that you have shmem mapping.

You will not arbitrarily have non-shmem folios in a shmem mapping :)

Quoted 29 lines
tools/testing/selftests/mm/vm_util.c +16 -5
-	return check_large_folios(addr, len, nr_hpages, hpage_size);
+	return false;
 }
 
-bool check_huge_file(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
+static bool __check_huge(void *addr, size_t len, int nr_hpages,
+			 uint64_t hpage_size, enum check_huge_type type)
 {
-	uint64_t pmd_pagesize = read_pmd_pagesize();
+	bool ret = false;
+	int pagemap_fd, kpageflags_fd;
+	int nr_pmd_mappings = 0;
+	uint64_t pmd_pagesize, scan_mapping_size;
+	uint64_t categories, kpf;
+	unsigned long pfn;
+	bool check_pmd_mapping, allow_nonpresent;
+	char *start = addr;
+	char *end = start + len;
 
+	pmd_pagesize = read_pmd_pagesize();
 	if (!pmd_pagesize)
 		ksft_exit_fail_msg("reading PMD pagesize failed\n");
 
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "FilePmdMapped:", nr_hpages, hpage_size);
+	check_pmd_mapping = (hpage_size == pmd_pagesize);
+	scan_mapping_size = (nr_hpages > 0) ? hpage_size : psize();
+	/* Some mTHP tests check a partially populated PMD-sized range. */

tools/testing/selftests/mm/vm_util.c static bool check_large_folios(void *addr, size_t len, int nr_hpages, · line 436

Quoted 10 lines
tools/testing/selftests/mm/vm_util.c +8 -1
+	allow_nonpresent = (uint64_t)nr_hpages * hpage_size < len;
 
-	return check_large_folios(addr, len, nr_hpages, hpage_size);
+	pagemap_fd = open(PAGEMAP_PATH, O_RDONLY);
+	if (pagemap_fd < 0)
+		ksft_exit_fail_msg("open pagemap fail\n");
+
+	kpageflags_fd = open(KPAGEFLAGS_PATH, O_RDONLY);
+	if (kpageflags_fd < 0) {
+		close(pagemap_fd);

No need to cleanup when exiting.

Quoted 36 lines
tools/testing/selftests/mm/vm_util.c +35 -0
+		ksft_exit_fail_msg("open kpageflags fail\n");
+	}
+
+	if (!check_pmd_mapping &&
+	    !check_large_folios(pagemap_fd, kpageflags_fd,
+				addr, len, nr_hpages, hpage_size))
+		goto out;
+
+	for (; start < end; start += scan_mapping_size) {
+		categories = pagemap_scan_get_categories(pagemap_fd, start);
+		pfn = pagemap_get_pfn(pagemap_fd, start);
+		if (pfn == -1UL) {
+			if (!allow_nonpresent)
+				goto out;
+			else
+				continue;
+		}
+		if (pageflags_get(pfn, kpageflags_fd, &kpf))
+			ksft_exit_fail_msg("read kpageflags: %s\n", strerror(errno));
+		if (check_pmd_mapping && (categories & PAGE_IS_HUGE))
+			nr_pmd_mappings++;
+		if (kpf & KPF_COMPOUND_TAIL)
+			continue;
+		if (!check_huge_type(categories, kpf, type))
+			goto out;
+	}
+
+	if (check_pmd_mapping && (nr_pmd_mappings != nr_hpages))
+		goto out;
+	ret = true;
+
+out:
+	close(pagemap_fd);
+	close(kpageflags_fd);
+	return ret;
 }
Signature · 3 lines
Cheers,

David
#5 lore raw

Re: [PATCH v5 2/3] kselftest: mm: replace usage of /proc/self/smaps for check_huge_xxx() helper

Quoted 61 lines · Yeoreum Yun
Quoted 37 lines

Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign().

The underlying VMA may start at a different address from the aligned address returned by memalign(). Furthermore, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because the flag is already set.

This causes split_huge_page_test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

---
 tools/testing/selftests/mm/vm_util.c | 145 ++++++++++++++++++++++-------------
 tools/testing/selftests/mm/vm_util.h |   1 +
 2 files changed, 92 insertions(+), 54 deletions(-)
tools/testing/selftests/mm/vm_util.c +0 -0
index 4821a3563036..dc62ce84e143 100644

[...]

Quoted 7 lines
tools/testing/selftests/mm/vm_util.c +4 -2
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "AnonHugePages: ", nr_hpages, hpage_size);
+static bool check_huge_type(uint64_t categories, uint64_t kpageflags,
+			    enum check_huge_type type)
+{
+	const bool file = categories & PAGE_IS_FILE;

tools/testing/selftests/mm/vm_util.c static bool check_large_folios(void *addr, size_t len, int nr_hpages, · line 403

Quoted 10 lines
tools/testing/selftests/mm/vm_util.c +10 -0
+	const bool swapbacked = kpageflags & KPF_SWAPBACKED;
+
+	switch (type) {
+	case CHECK_HUGE_ANON:
+		return !file;
+	case CHECK_HUGE_FILE:
+		return file && !swapbacked;
+	case CHECK_HUGE_SHMEM:
+		return file & swapbacked;
+	}

Why do we even need CHECK_HUGE_SHMEM?

Quoted 5 lines

That's really just a legacy thing for using smaps to identify huge pages.

It's sufficient to identify CHECK_HUGE_FILE if you know that you have shmem mapping.

You will not arbitrarily have non-shmem folios in a shmem mapping :)

Agree. but there seems the case where discern whethr the mapping is with regular file or shmem like tmpfs -- khugepaged test where using __shmem_ops. So I think it would be better to keep this as-is.

Quoted 32 lines
Quoted 32 lines
tools/testing/selftests/mm/vm_util.c +17 -5
-	return check_large_folios(addr, len, nr_hpages, hpage_size);
+	return false;
 }
 
-bool check_huge_file(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
+static bool __check_huge(void *addr, size_t len, int nr_hpages,
+			 uint64_t hpage_size, enum check_huge_type type)
 {
-	uint64_t pmd_pagesize = read_pmd_pagesize();
+	bool ret = false;
+	int pagemap_fd, kpageflags_fd;
+	int nr_pmd_mappings = 0;
+	uint64_t pmd_pagesize, scan_mapping_size;
+	uint64_t categories, kpf;
+	unsigned long pfn;
+	bool check_pmd_mapping, allow_nonpresent;
+	char *start = addr;
+	char *end = start + len;
 
+	pmd_pagesize = read_pmd_pagesize();
 	if (!pmd_pagesize)
 		ksft_exit_fail_msg("reading PMD pagesize failed\n");
 
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "FilePmdMapped:", nr_hpages, hpage_size);
+	check_pmd_mapping = (hpage_size == pmd_pagesize);
+	scan_mapping_size = (nr_hpages > 0) ? hpage_size : psize();
+	/* Some mTHP tests check a partially populated PMD-sized range. */
+	allow_nonpresent = (uint64_t)nr_hpages * hpage_size < len;
Quoted 10 lines

tools/testing/selftests/mm/vm_util.c static bool check_large_folios(void *addr, size_t len, int nr_hpages, · line 436

Quoted 8 lines
tools/testing/selftests/mm/vm_util.c +7 -1
-	return check_large_folios(addr, len, nr_hpages, hpage_size);
+	pagemap_fd = open(PAGEMAP_PATH, O_RDONLY);
+	if (pagemap_fd < 0)
+		ksft_exit_fail_msg("open pagemap fail\n");
+
+	kpageflags_fd = open(KPAGEFLAGS_PATH, O_RDONLY);
+	if (kpageflags_fd < 0) {
+		close(pagemap_fd);

No need to cleanup when exiting.

Yes, but some test code does clean it up, so I was just confused about the convention :\

Quoted 45 lines
Quoted 37 lines
tools/testing/selftests/mm/vm_util.c +35 -0
+		ksft_exit_fail_msg("open kpageflags fail\n");
+	}
+
+	if (!check_pmd_mapping &&
+	    !check_large_folios(pagemap_fd, kpageflags_fd,
+				addr, len, nr_hpages, hpage_size))
+		goto out;
+
+	for (; start < end; start += scan_mapping_size) {
+		categories = pagemap_scan_get_categories(pagemap_fd, start);
+		pfn = pagemap_get_pfn(pagemap_fd, start);
+		if (pfn == -1UL) {
+			if (!allow_nonpresent)
+				goto out;
+			else
+				continue;
+		}
+		if (pageflags_get(pfn, kpageflags_fd, &kpf))
+			ksft_exit_fail_msg("read kpageflags: %s\n", strerror(errno));
+		if (check_pmd_mapping && (categories & PAGE_IS_HUGE))
+			nr_pmd_mappings++;
+		if (kpf & KPF_COMPOUND_TAIL)
+			continue;
+		if (!check_huge_type(categories, kpf, type))
+			goto out;
+	}
+
+	if (check_pmd_mapping && (nr_pmd_mappings != nr_hpages))
+		goto out;
+	ret = true;
+
+out:
+	close(pagemap_fd);
+	close(kpageflags_fd);
+	return ret;
 }
Signature · 3 lines
Cheers,

David
Signature · 2 lines
Sincerely,
Yeoreum Yun
#6 lore raw

Re: [PATCH v5 2/3] kselftest: mm: replace usage of /proc/self/smaps for check_huge_xxx() helper

Quoted 63 lines · Yeoreum Yun
Quoted 63 lines · Yeoreum Yun
Quoted 37 lines

Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign().

The underlying VMA may start at a different address from the aligned address returned by memalign(). Furthermore, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because the flag is already set.

This causes split_huge_page_test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

---
 tools/testing/selftests/mm/vm_util.c | 145 ++++++++++++++++++++++-------------
 tools/testing/selftests/mm/vm_util.h |   1 +
 2 files changed, 92 insertions(+), 54 deletions(-)
tools/testing/selftests/mm/vm_util.c +0 -0
index 4821a3563036..dc62ce84e143 100644

[...]

Quoted 7 lines
tools/testing/selftests/mm/vm_util.c +4 -2
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "AnonHugePages: ", nr_hpages, hpage_size);
+static bool check_huge_type(uint64_t categories, uint64_t kpageflags,
+			    enum check_huge_type type)
+{
+	const bool file = categories & PAGE_IS_FILE;

tools/testing/selftests/mm/vm_util.c static bool check_large_folios(void *addr, size_t len, int nr_hpages, · line 403

Quoted 10 lines
tools/testing/selftests/mm/vm_util.c +10 -0
+	const bool swapbacked = kpageflags & KPF_SWAPBACKED;
+
+	switch (type) {
+	case CHECK_HUGE_ANON:
+		return !file;
+	case CHECK_HUGE_FILE:
+		return file && !swapbacked;
+	case CHECK_HUGE_SHMEM:
+		return file & swapbacked;
+	}

Why do we even need CHECK_HUGE_SHMEM?

That's really just a legacy thing for using smaps to identify huge pages.

Quoted 7 lines
Quoted 3 lines

It's sufficient to identify CHECK_HUGE_FILE if you know that you have shmem mapping.

You will not arbitrarily have non-shmem folios in a shmem mapping :)

Agree. but there seems the case where discern whethr the mapping is with regular file or shmem like tmpfs -- khugepaged test where using __shmem_ops. So I think it would be better to keep this as-is.

Let's not add or maintain unnecessary complexity.

hmem_check_huge can really just become check_huge_file.

Even file_check_huge can just be simplified.

There must be a pretty good reason why we'd want to keep this.

Signature · 3 lines
Cheers,

David
#7 lore raw

Re: [PATCH v5 2/3] kselftest: mm: replace usage of /proc/self/smaps for check_huge_xxx() helper

Quoted 72 lines · Yeoreum Yun
Quoted 63 lines
Quoted 63 lines · Yeoreum Yun
Quoted 37 lines

Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign().

The underlying VMA may start at a different address from the aligned address returned by memalign(). Furthermore, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because the flag is already set.

This causes split_huge_page_test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

---
 tools/testing/selftests/mm/vm_util.c | 145 ++++++++++++++++++++++-------------
 tools/testing/selftests/mm/vm_util.h |   1 +
 2 files changed, 92 insertions(+), 54 deletions(-)
tools/testing/selftests/mm/vm_util.c +0 -0
index 4821a3563036..dc62ce84e143 100644

[...]

Quoted 7 lines
tools/testing/selftests/mm/vm_util.c +4 -2
-	if (hpage_size == pmd_pagesize)
-		return __check_pmd_huge(addr, "AnonHugePages: ", nr_hpages, hpage_size);
+static bool check_huge_type(uint64_t categories, uint64_t kpageflags,
+			    enum check_huge_type type)
+{
+	const bool file = categories & PAGE_IS_FILE;

tools/testing/selftests/mm/vm_util.c static bool check_large_folios(void *addr, size_t len, int nr_hpages, · line 403

Quoted 10 lines
tools/testing/selftests/mm/vm_util.c +10 -0
+	const bool swapbacked = kpageflags & KPF_SWAPBACKED;
+
+	switch (type) {
+	case CHECK_HUGE_ANON:
+		return !file;
+	case CHECK_HUGE_FILE:
+		return file && !swapbacked;
+	case CHECK_HUGE_SHMEM:
+		return file & swapbacked;
+	}

Why do we even need CHECK_HUGE_SHMEM?

That's really just a legacy thing for using smaps to identify huge pages.

Quoted 7 lines
Quoted 3 lines

It's sufficient to identify CHECK_HUGE_FILE if you know that you have shmem mapping.

You will not arbitrarily have non-shmem folios in a shmem mapping :)

Agree. but there seems the case where discern whethr the mapping is with regular file or shmem like tmpfs -- khugepaged test where using __shmem_ops. So I think it would be better to keep this as-is.

Quoted 7 lines

Let's not add or maintain unnecessary complexity.

hmem_check_huge can really just become check_huge_file.

Even file_check_huge can just be simplified.

There must be a pretty good reason why we'd want to keep this.

Okay. then I'll change the check_huge_shmem() to check_huge_file() with the separate patch.

Signature · 2 lines
Sincerely,
Yeoreum Yun
#8 lore raw

Re: [PATCH v5 2/3] kselftest: mm: replace usage of /proc/self/smaps for check_huge_xxx() helper

Quoted 8 lines · Yeoreum Yun
Quoted 8 lines · Yeoreum Yun
Quoted 4 lines

Agree. but there seems the case where discern whethr the mapping is with regular file or shmem like tmpfs -- khugepaged test where using __shmem_ops. So I think it would be better to keep this as-is.

Let's not add or maintain unnecessary complexity.

Quoted 8 lines
Quoted 5 lines

hmem_check_huge can really just become check_huge_file.

Even file_check_huge can just be simplified.

There must be a pretty good reason why we'd want to keep this.

Okay. then I'll change the check_huge_shmem() to check_huge_file() with the separate patch.

You'll likely have to make both behave the same under the hood first to then remove check_huge_shmem().

Signature · 3 lines
Cheers,

David
#9 lore raw

Re: [PATCH v5 2/3] kselftest: mm: replace usage of /proc/self/smaps for check_huge_xxx() helper

Quoted 13 lines · David Hildenbrand (Arm)
Quoted 12 lines · Yeoreum Yun
Quoted 12 lines · Yeoreum Yun
Quoted 4 lines

Agree. but there seems the case where discern whethr the mapping is with regular file or shmem like tmpfs -- khugepaged test where using __shmem_ops. So I think it would be better to keep this as-is.

Let's not add or maintain unnecessary complexity.

hmem_check_huge can really just become check_huge_file.

Even file_check_huge can just be simplified.

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

There must be a pretty good reason why we'd want to keep this.

Okay. then I'll change the check_huge_shmem() to check_huge_file() with the separate patch.

You'll likely have to make both behave the same under the hood first to then remove check_huge_shmem().

Acked.

Signature · 2 lines
Sincerely,
Yeoreum Yun
#10 lore raw

PATCH v5 3/3 kselftest: mm: introduce alloc_isolated_mem()

In reply to Yeoreum Yun

Some MM tests use memalign() to allocate memory. However, the resulting memory area may unexpectedly merge with an adjacent VMA, causing tests that inspect it through /proc/self/smaps to fail.

Eliminate this potential source of test flakiness by introducing alloc_isolated_mem(), which places guard regions before and after the allocated area to prevent unexpected VMA merging.

Replace memalign() with this helper and use the helper in softdirty_supported() instead of mmap().

---
 tools/testing/selftests/mm/pagemap_ioctl.c        | 18 +++---
 tools/testing/selftests/mm/soft-dirty.c           |  6 +-
 tools/testing/selftests/mm/split_huge_page_test.c |  8 +--
 tools/testing/selftests/mm/vm_util.c              | 67 +++++++++++++++++++++--
 tools/testing/selftests/mm/vm_util.h              |  2 +
 5 files changed, 81 insertions(+), 20 deletions(-)
tools/testing/selftests/mm/pagemap_ioctl.c +9 -9
index eadc7159ca5b..7ed5b8fb3b25 100644
@@ -792,9 +792,9 @@ void *gethugepage(int map_size)
 	int ret;
 	char *map;
 
-	map = memalign(hpage_size, map_size);
+	map = alloc_isolated_mem(hpage_size, map_size);
 	if (!map)
-		ksft_exit_fail_msg("memalign failed %d %s\n", errno, strerror(errno));
+		ksft_exit_fail_msg("alloc_isolated_mem failed %d %s\n", errno, strerror(errno));
 
 	ret = madvise(map, map_size, MADV_HUGEPAGE);
 	if (ret)
@@ -856,7 +856,7 @@ int hpage_unit_tests(void)
 
 		/* 4. only middle page written */
 		wp_free(map, map_size);
-		free(map);
+		free_isolated_mem(map, map_size);
 		map = gethugepage(map_size);
 		wp_init(map, map_size);
 		wp_addr_range(map, map_size);
@@ -871,7 +871,7 @@ int hpage_unit_tests(void)
 				 "%s only middle page written\n", __func__);
 
 		wp_free(map, map_size);
-		free(map);
+		free_isolated_mem(map, map_size);
 	} else {
 		ksft_test_result_skip("%s all new huge page must be written\n", __func__);
 		ksft_test_result_skip("%s all the huge page must not be written\n", __func__);
@@ -898,7 +898,7 @@ int hpage_unit_tests(void)
 				 vec[0].start == (uintptr_t)(map + map_size/2),
 				 "%s clear first half of huge page\n", __func__);
 		wp_free(map, map_size);
-		free(map);
+		free_isolated_mem(map, map_size);
 	} else {
 		ksft_test_result_skip("%s clear first half of huge page\n", __func__);
 	}
@@ -927,7 +927,7 @@ int hpage_unit_tests(void)
 				 "%s clear first half of huge page with limited buffer\n",
 				 __func__);
 		wp_free(map, map_size);
-		free(map);
+		free_isolated_mem(map, map_size);
 	} else {
 		ksft_test_result_skip("%s clear first half of huge page with limited buffer\n",
 				      __func__);
@@ -955,7 +955,7 @@ int hpage_unit_tests(void)
 		ksft_test_result(ret == 1 && LEN(vec[0]) == vec_size/2,
 				 "%s clear second half huge page\n", __func__);
 		wp_free(map, map_size);
-		free(map);
+		free_isolated_mem(map, map_size);
 	} else {
 		ksft_test_result_skip("%s clear second half huge page\n", __func__);
 	}
@@ -988,7 +988,7 @@ int hpage_unit_tests(void)
 				 "%s get half huge page\n", __func__);
 
 		wp_free(map, map_size);
-		free(map);
+		free_isolated_mem(map, map_size);
 	} else {
 		ksft_test_result_skip("%s get half huge page\n", __func__);
 		ksft_test_result_skip("%s get half huge page\n", __func__);
@@ -1702,7 +1702,7 @@ int main(int __attribute__((unused)) argc, char *argv[])
 		wp_addr_range(map, hpage_size);
 		base_tests("Huge page testing:", map, hpage_size, 0);
 		wp_free(map, hpage_size);
-		free(map);
+		free_isolated_mem(map, hpage_size);
 	} else {
 		base_tests("Huge page testing:", NULL, 0, 1);
 	}
tools/testing/selftests/mm/soft-dirty.c +3 -3
index 5f278913c4d7..b6fad38c8bee 100644
@@ -92,9 +92,9 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 	if (!hpage_len)
 		ksft_exit_fail_msg("Reading PMD pagesize failed");
 
-	map = memalign(hpage_len, hpage_len);
+	map = alloc_isolated_mem(hpage_len, hpage_len);
 	if (!map)
-		ksft_exit_fail_msg("memalign failed\n");
+		ksft_exit_fail_msg("alloc_isolated_mem failed\n");
 
 	ret = madvise(map, hpage_len, MADV_HUGEPAGE);
 	if (ret)
@@ -130,7 +130,7 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 		ksft_test_result_skip("Test %s huge page allocation\n", __func__);
 		ksft_test_result_skip("Test %s huge page dirty bit\n", __func__);
 	}
-	free(map);
+	free_isolated_mem(map, hpage_len);
 }
 
 static void test_mprotect(int pagemap_fd, int pagesize, bool anon)
tools/testing/selftests/mm/split_huge_page_test.c +4 -4
index cba327f77873..7865b0bab011 100644
@@ -153,7 +153,7 @@ static char *allocate_zero_filled_hugepage(size_t len)
 	char *result;
 	size_t i;
 
-	result = memalign(pmd_pagesize, len);
+	result = alloc_isolated_mem(pmd_pagesize, len);
 	if (!result) {
 		printf("Fail to allocate memory\n");
 		exit(EXIT_FAILURE);
@@ -219,7 +219,7 @@ static void split_pmd_zero_pages(void)
 	one_page = allocate_zero_filled_hugepage(len);
 	verify_rss_anon_split_huge_page_all_zeroes(one_page, nr_hpages, len);
 	ksft_test_result_pass("Split zero filled huge pages successful\n");
-	free(one_page);
+	free_isolated_mem(one_page, len);
 }
 
 static void split_pmd_thp_to_order(int order)
@@ -228,7 +228,7 @@ static void split_pmd_thp_to_order(int order)
 	size_t len = 4 * pmd_pagesize;
 	size_t i;
 
-	one_page = memalign(pmd_pagesize, len);
+	one_page = alloc_isolated_mem(pmd_pagesize, len);
 	if (!one_page)
 		ksft_exit_fail_msg("Fail to allocate memory: %s\n", strerror(errno));
 
@@ -262,7 +262,7 @@ static void split_pmd_thp_to_order(int order)
 		ksft_exit_fail_msg("Still AnonHugePages not split\n");
 
 	ksft_test_result_pass("Split huge pages to order %d successful\n", order);
-	free(one_page);
+	free_isolated_mem(one_page, len);
 }
 
 static void split_pte_mapped_thp(void)
tools/testing/selftests/mm/vm_util.c +63 -4

88 lines not on this page · raw message

tools/testing/selftests/mm/vm_util.h +2 -0
index f19bd17817e5..ce1da9a55310 100644
@@ -95,6 +95,8 @@ bool check_huge_anon(void *addr, size_t len, int nr_hpages, uint64_t hpage_size)
 bool check_huge_file(void *addr, size_t len, int nr_hpages, uint64_t hpage_size);
 bool check_huge_shmem(void *addr, size_t len, int nr_hpages, uint64_t hpage_size);
 int64_t allocate_transhuge(void *ptr, int pagemap_fd);
+void *alloc_isolated_mem(size_t align, size_t size);
+void free_isolated_mem(void *addr, size_t size);
 int pageflags_get(unsigned long pfn, int kpageflags_fd, uint64_t *flags);
 int gather_folio_orders(char *vaddr_start, size_t len,
 		int pagemap_fd, int kpageflags_fd, int orders[], int nr_orders);
Signature · 1 line
2.43.0
#11 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 17 lines
tools/testing/selftests/mm/soft-dirty.c +2 -2
index 5f278913c4d7..b6fad38c8bee 100644
@@ -92,9 +92,9 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 	if (!hpage_len)
 		ksft_exit_fail_msg("Reading PMD pagesize failed");
 
-	map = memalign(hpage_len, hpage_len);
+	map = alloc_isolated_mem(hpage_len, hpage_len);
 	if (!map)
-		ksft_exit_fail_msg("memalign failed\n");
+		ksft_exit_fail_msg("alloc_isolated_mem failed\n");
 
 	ret = madvise(map, hpage_len, MADV_HUGEPAGE);
 	if (ret)

tools/testing/selftests/mm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 134

Quoted 6 lines
tools/testing/selftests/mm/soft-dirty.c +1 -1
@@ -130,7 +130,7 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 		ksft_test_result_skip("Test %s huge page allocation\n", __func__);
 		ksft_test_result_skip("Test %s huge page dirty bit\n", __func__);
 	}
-	free(map);
+	free_isolated_mem(map, hpage_len);
 }

smaps is really only problematic with merged VMAs when relying on exact page statistics. For other properties (vm flags, MMUPageSize) it's not a problem as long as our smap search just finds the VMA that covers an address.

Assuming we fixed check_huge_anon() to not use smaps, why is this change here required?

Signature · 3 lines
Cheers,

David
#12 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 28 lines
Quoted 17 lines
tools/testing/selftests/mm/soft-dirty.c +2 -2
index 5f278913c4d7..b6fad38c8bee 100644
@@ -92,9 +92,9 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 	if (!hpage_len)
 		ksft_exit_fail_msg("Reading PMD pagesize failed");
 
-	map = memalign(hpage_len, hpage_len);
+	map = alloc_isolated_mem(hpage_len, hpage_len);
 	if (!map)
-		ksft_exit_fail_msg("memalign failed\n");
+		ksft_exit_fail_msg("alloc_isolated_mem failed\n");
 
 	ret = madvise(map, hpage_len, MADV_HUGEPAGE);
 	if (ret)

tools/testing/selftests/mm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 134

Quoted 6 lines
tools/testing/selftests/mm/soft-dirty.c +1 -1
@@ -130,7 +130,7 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 		ksft_test_result_skip("Test %s huge page allocation\n", __func__);
 		ksft_test_result_skip("Test %s huge page dirty bit\n", __func__);
 	}
-	free(map);
+	free_isolated_mem(map, hpage_len);
 }

smaps is really only problematic with merged VMAs when relying on exact page statistics. For other properties (vm flags, MMUPageSize) it's not a problem as long as our smap search just finds the VMA that covers an address.

Quoted 2 lines

Assuming we fixed check_huge_anon() to not use smaps, why is this change here required?

Since there is no interface to get vm_flags not via /proc/self/smaps, It might be good to have it for preventing unwanted VMA merge.

And might be useful for future to prevent unwated VMA merge.

Signature · 2 lines
Sincerely,
Yeoreum Yun
#13 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 28 lines · Yeoreum Yun
Quoted 28 lines
Quoted 17 lines
tools/testing/selftests/mm/soft-dirty.c +2 -2
index 5f278913c4d7..b6fad38c8bee 100644
@@ -92,9 +92,9 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 	if (!hpage_len)
 		ksft_exit_fail_msg("Reading PMD pagesize failed");
 
-	map = memalign(hpage_len, hpage_len);
+	map = alloc_isolated_mem(hpage_len, hpage_len);
 	if (!map)
-		ksft_exit_fail_msg("memalign failed\n");
+		ksft_exit_fail_msg("alloc_isolated_mem failed\n");
 
 	ret = madvise(map, hpage_len, MADV_HUGEPAGE);
 	if (ret)

tools/testing/selftests/mm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 134

Quoted 6 lines
tools/testing/selftests/mm/soft-dirty.c +1 -1
@@ -130,7 +130,7 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 		ksft_test_result_skip("Test %s huge page allocation\n", __func__);
 		ksft_test_result_skip("Test %s huge page dirty bit\n", __func__);
 	}
-	free(map);
+	free_isolated_mem(map, hpage_len);
 }

smaps is really only problematic with merged VMAs when relying on exact page statistics. For other properties (vm flags, MMUPageSize) it's not a problem as long as our smap search just finds the VMA that covers an address.

Quoted 7 lines
Quoted 2 lines

Assuming we fixed check_huge_anon() to not use smaps, why is this change here required?

Since there is no interface to get vm_flags not via /proc/self/smaps, It might be good to have it for preventing unwanted VMA merge.

And might be useful for future to prevent unwated VMA merge.

See my reply on why vm flags are generally not a problem. Just like other properties that are not changed during VMA merging.

Let's not perform random code changes without a clear picture.

And just to emphasize again: VMA merging could already be a problem before memalign() internal changes.

(also observe here how we do a MADV_HUGEPAGE, so this is all rather arbitrary, which is not good)

Signature · 3 lines
Cheers,

David
#14 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 40 lines · David Hildenbrand (Arm)
Quoted 28 lines · Yeoreum Yun
Quoted 28 lines
Quoted 17 lines
tools/testing/selftests/mm/soft-dirty.c +2 -2
index 5f278913c4d7..b6fad38c8bee 100644
@@ -92,9 +92,9 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 	if (!hpage_len)
 		ksft_exit_fail_msg("Reading PMD pagesize failed");
 
-	map = memalign(hpage_len, hpage_len);
+	map = alloc_isolated_mem(hpage_len, hpage_len);
 	if (!map)
-		ksft_exit_fail_msg("memalign failed\n");
+		ksft_exit_fail_msg("alloc_isolated_mem failed\n");
 
 	ret = madvise(map, hpage_len, MADV_HUGEPAGE);
 	if (ret)

tools/testing/selftests/mm/soft-dirty.c static void test_hugepage(int pagemap_fd, int pagesize) · line 134

Quoted 6 lines
tools/testing/selftests/mm/soft-dirty.c +1 -1
@@ -130,7 +130,7 @@ static void test_hugepage(int pagemap_fd, int pagesize)
 		ksft_test_result_skip("Test %s huge page allocation\n", __func__);
 		ksft_test_result_skip("Test %s huge page dirty bit\n", __func__);
 	}
-	free(map);
+	free_isolated_mem(map, hpage_len);
 }

smaps is really only problematic with merged VMAs when relying on exact page statistics. For other properties (vm flags, MMUPageSize) it's not a problem as long as our smap search just finds the VMA that covers an address.

Quoted 7 lines
Quoted 2 lines

Assuming we fixed check_huge_anon() to not use smaps, why is this change here required?

Since there is no interface to get vm_flags not via /proc/self/smaps, It might be good to have it for preventing unwanted VMA merge.

And might be useful for future to prevent unwated VMA merge.

See my reply on why vm flags are generally not a problem. Just like other properties that are not changed during VMA merging.

Quoted 7 lines

Let's not perform random code changes without a clear picture.

And just to emphasize again: VMA merging could already be a problem before memalign() internal changes.

(also observe here how we do a MADV_HUGEPAGE, so this is all rather arbitrary, which is not good)

As I mentioned in my previous reply, what I’m trying to prevent here is a failure when checking, immediately after memory allocation, that a specific vm_flag is not set.

Yes, I agree that this could have been a problem even before the internal changes to memalign(). An unwanted VMA merge could already occur at the time of memory allocation.

So what I’m trying to avoid is a test failure where, due to such an unexpected VMA merge during allocation, the subsequent check that a specific vm_flag is not present fails.

I probably didn’t explain this clearly enough and ended up causing more confusion. Sorry about that.

Signature · 2 lines
Sincerely,
Yeoreum Yun
#15 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 23 lines · Yeoreum Yun
Quoted 10 lines · David Hildenbrand (Arm)
Quoted 5 lines · Yeoreum Yun

Since there is no interface to get vm_flags not via /proc/self/smaps, It might be good to have it for preventing unwanted VMA merge.

And might be useful for future to prevent unwated VMA merge.

See my reply on why vm flags are generally not a problem. Just like other properties that are not changed during VMA merging.

Quoted 7 lines

Let's not perform random code changes without a clear picture.

And just to emphasize again: VMA merging could already be a problem before memalign() internal changes.

(also observe here how we do a MADV_HUGEPAGE, so this is all rather arbitrary, which is not good)

As I mentioned in my previous reply, what I’m trying to prevent here is a failure when checking, immediately after memory allocation, that a specific vm_flag is not set.

Quoted 7 lines

Yes, I agree that this could have been a problem even before the internal changes to memalign(). An unwanted VMA merge could already occur at the time of memory allocation.

So what I’m trying to avoid is a test failure where, due to such an unexpected VMA merge during allocation, the subsequent check that a specific vm_flag is not present fails.

Which is only a guard-region marker problem?

Signature · 3 lines
Cheers,

David
#16 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 24 lines · Yeoreum Yun
Quoted 23 lines · David Hildenbrand (Arm)
Quoted 10 lines · Yeoreum Yun
Quoted 5 lines

Since there is no interface to get vm_flags not via /proc/self/smaps, It might be good to have it for preventing unwanted VMA merge.

And might be useful for future to prevent unwated VMA merge.

See my reply on why vm flags are generally not a problem. Just like other properties that are not changed during VMA merging.

Quoted 7 lines

Let's not perform random code changes without a clear picture.

And just to emphasize again: VMA merging could already be a problem before memalign() internal changes.

(also observe here how we do a MADV_HUGEPAGE, so this is all rather arbitrary, which is not good)

As I mentioned in my previous reply, what I’m trying to prevent here is a failure when checking, immediately after memory allocation, that a specific vm_flag is not set.

Quoted 8 lines
Quoted 7 lines

Yes, I agree that this could have been a problem even before the internal changes to memalign(). An unwanted VMA merge could already occur at the time of memory allocation.

So what I’m trying to avoid is a test failure where, due to such an unexpected VMA merge during allocation, the subsequent check that a specific vm_flag is not present fails.

Which is only a guard-region marker problem?

Yes. so if we remove ASSERT_FALSE(check_vmflag_guard(ptr)), TBH we don't need this patch unless other usage comes up to prevent unwanted VMA merge.

Would it be better to drop ASSERT_FALSE(check_vmflag_guard(ptr)) in guard test?

Signature · 2 lines
Sincerely,
Yeoreum Yun
#17 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 13 lines · Yeoreum Yun
Quoted 13 lines · Yeoreum Yun
Quoted 12 lines

As I mentioned in my previous reply, what I’m trying to prevent here is a failure when checking, immediately after memory allocation, that a specific vm_flag is not set.

Yes, I agree that this could have been a problem even before the internal changes to memalign(). An unwanted VMA merge could already occur at the time of memory allocation.

So what I’m trying to avoid is a test failure where, due to such an unexpected VMA merge during allocation, the subsequent check that a specific vm_flag is not present fails.

Quoted 8 lines
Quoted 1 line

Which is only a guard-region marker problem?

Yes. so if we remove ASSERT_FALSE(check_vmflag_guard(ptr)), TBH we don't need this patch unless other usage comes up to prevent unwanted VMA merge.

Would it be better to drop ASSERT_FALSE(check_vmflag_guard(ptr)) in guard test?

I guess there is value in asserting that not all VMAs by accident start with an over-indication of maybe having guard pages, which is why Lorenzo added that check :)

But I think even alloc_isolated_mem() is wrong in that regard: if the original VMA gets merged, we could inherit the guard-marker, no?

Maybe the following would be good enough?

tools/testing/selftests/mm/guard-regions.c +12 -2
index 5c8ec3ca75d7d..791bf6a68b9e6 100644
@@ -2257,10 +2257,20 @@ TEST_F(guard_regions, smaps)
        char *ptr, *ptr2;
        int i;
 
-       /* Map a region. */
-       ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0);
+       /* Reserve a 10 page region with 1 page space to both sides. */
+       ptr = mmap_(self, variant, NULL, 12 * page_size, PROT_NONE, 0, 0);
        ASSERT_NE(ptr, MAP_FAILED);
 
+       /* Map a new region that is guaranteed to not get merged in any way. */
+       ptr = mmap_(self, variant | MAP_FIXED, ptr + pagesize, 10 * page_size,
+                   PROT_READ | PROT_WRITE, 0, 0);
+       ASSERT_EQ(ptr, ptr + pagesize);
+
+       /* Clean up the excess pages left and right. */
+       munmap(ptr, pagesize);
+       munmap(ptr + 11, pagesize);
+       ptr + = pagesize;
+
        /* We shouldn't yet see a guard flag. */
        ASSERT_FALSE(check_vmflag_guard(ptr));
 
Signature · 3 lines
Cheers,

David
#18 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 23 lines · Yeoreum Yun
Quoted 13 lines
Quoted 13 lines · Yeoreum Yun
Quoted 12 lines

As I mentioned in my previous reply, what I’m trying to prevent here is a failure when checking, immediately after memory allocation, that a specific vm_flag is not set.

Yes, I agree that this could have been a problem even before the internal changes to memalign(). An unwanted VMA merge could already occur at the time of memory allocation.

So what I’m trying to avoid is a test failure where, due to such an unexpected VMA merge during allocation, the subsequent check that a specific vm_flag is not present fails.

Quoted 8 lines
Quoted 1 line

Which is only a guard-region marker problem?

Yes. so if we remove ASSERT_FALSE(check_vmflag_guard(ptr)), TBH we don't need this patch unless other usage comes up to prevent unwanted VMA merge.

Would it be better to drop ASSERT_FALSE(check_vmflag_guard(ptr)) in guard test?

Quoted 6 lines

I guess there is value in asserting that not all VMAs by accident start with an over-indication of maybe having guard pages, which is why Lorenzo added that check :)

But I think even alloc_isolated_mem() is wrong in that regard: if the original VMA gets merged, we could inherit the guard-marker, no?

True. I overlooked guard bit is sticky.

Quoted 32 lines

Maybe the following would be good enough?

tools/testing/selftests/mm/guard-regions.c +12 -2
index 5c8ec3ca75d7d..791bf6a68b9e6 100644
@@ -2257,10 +2257,20 @@ TEST_F(guard_regions, smaps)
        char *ptr, *ptr2;
        int i;
 
-       /* Map a region. */
-       ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0);
+       /* Reserve a 10 page region with 1 page space to both sides. */
+       ptr = mmap_(self, variant, NULL, 12 * page_size, PROT_NONE, 0, 0);
        ASSERT_NE(ptr, MAP_FAILED);
 
+       /* Map a new region that is guaranteed to not get merged in any way. */
+       ptr = mmap_(self, variant | MAP_FIXED, ptr + pagesize, 10 * page_size,
+                   PROT_READ | PROT_WRITE, 0, 0);
+       ASSERT_EQ(ptr, ptr + pagesize);
+
+       /* Clean up the excess pages left and right. */
+       munmap(ptr, pagesize);
+       munmap(ptr + 11, pagesize);
+       ptr + = pagesize;
+
        /* We shouldn't yet see a guard flag. */
        ASSERT_FALSE(check_vmflag_guard(ptr));
 
Signature · 1 line
Cheers,

It's enough but alloc_isolated_mem() could be modified to allocate with PROT_NONE first and then change the prot.

It seems whether make a helper or testcase should write the code properly in case of unwanted VMA merge.

IMHO, instead of testcase write them all, it would be better to use helper (but in guard-region.c, might need to wrapper function, munmap_() like mmap_()).

Signature · 2 lines
Sincerely,
Yeoreum Yun
#19 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 62 lines
Quoted 23 lines · Yeoreum Yun
Quoted 13 lines
Quoted 13 lines · Yeoreum Yun
Quoted 12 lines

As I mentioned in my previous reply, what I’m trying to prevent here is a failure when checking, immediately after memory allocation, that a specific vm_flag is not set.

Yes, I agree that this could have been a problem even before the internal changes to memalign(). An unwanted VMA merge could already occur at the time of memory allocation.

So what I’m trying to avoid is a test failure where, due to such an unexpected VMA merge during allocation, the subsequent check that a specific vm_flag is not present fails.

Quoted 8 lines
Quoted 1 line

Which is only a guard-region marker problem?

Yes. so if we remove ASSERT_FALSE(check_vmflag_guard(ptr)), TBH we don't need this patch unless other usage comes up to prevent unwanted VMA merge.

Would it be better to drop ASSERT_FALSE(check_vmflag_guard(ptr)) in guard test?

Quoted 6 lines

I guess there is value in asserting that not all VMAs by accident start with an over-indication of maybe having guard pages, which is why Lorenzo added that check :)

But I think even alloc_isolated_mem() is wrong in that regard: if the original VMA gets merged, we could inherit the guard-marker, no?

True. I overlooked guard bit is sticky.

Quoted 30 lines

Maybe the following would be good enough?

tools/testing/selftests/mm/guard-regions.c +12 -2
index 5c8ec3ca75d7d..791bf6a68b9e6 100644
@@ -2257,10 +2257,20 @@ TEST_F(guard_regions, smaps)
        char *ptr, *ptr2;
        int i;
 
-       /* Map a region. */
-       ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0);
+       /* Reserve a 10 page region with 1 page space to both sides. */
+       ptr = mmap_(self, variant, NULL, 12 * page_size, PROT_NONE, 0, 0);
        ASSERT_NE(ptr, MAP_FAILED);
 
+       /* Map a new region that is guaranteed to not get merged in any way. */
+       ptr = mmap_(self, variant | MAP_FIXED, ptr + pagesize, 10 * page_size,
+                   PROT_READ | PROT_WRITE, 0, 0);
+       ASSERT_EQ(ptr, ptr + pagesize);
+
+       /* Clean up the excess pages left and right. */
+       munmap(ptr, pagesize);
+       munmap(ptr + 11, pagesize);
+       ptr + = pagesize;
+
        /* We shouldn't yet see a guard flag. */
        ASSERT_FALSE(check_vmflag_guard(ptr));
 
Quoted 5 lines
Quoted 2 lines
Signature · 1 line
Cheers,

It's enough but alloc_isolated_mem() could be modified to allocate with PROT_NONE first and then change the prot.

Ah, but if merged with PROT_NONE | GURAD still problem... Hmm.. might above enough.

Signature · 2 lines
Sincerely,
Yeoreum Yun
#20 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

In reply to Yeoreum Yun
Quoted 54 lines · Yeoreum Yun
Quoted 9 lines · Yeoreum Yun
Quoted 7 lines

Yes. so if we remove ASSERT_FALSE(check_vmflag_guard(ptr)), TBH we don't need this patch unless other usage comes up to prevent unwanted VMA merge.

Would it be better to drop ASSERT_FALSE(check_vmflag_guard(ptr)) in guard test?

Quoted 6 lines

I guess there is value in asserting that not all VMAs by accident start with an over-indication of maybe having guard pages, which is why Lorenzo added that check :)

But I think even alloc_isolated_mem() is wrong in that regard: if the original VMA gets merged, we could inherit the guard-marker, no?

True. I overlooked guard bit is sticky.

Quoted 32 lines

Maybe the following would be good enough?

tools/testing/selftests/mm/guard-regions.c +12 -2
index 5c8ec3ca75d7d..791bf6a68b9e6 100644
@@ -2257,10 +2257,20 @@ TEST_F(guard_regions, smaps)
        char *ptr, *ptr2;
        int i;
 
-       /* Map a region. */
-       ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0);
+       /* Reserve a 10 page region with 1 page space to both sides. */
+       ptr = mmap_(self, variant, NULL, 12 * page_size, PROT_NONE, 0, 0);
        ASSERT_NE(ptr, MAP_FAILED);
 
+       /* Map a new region that is guaranteed to not get merged in any way. */
+       ptr = mmap_(self, variant | MAP_FIXED, ptr + pagesize, 10 * page_size,
+                   PROT_READ | PROT_WRITE, 0, 0);
+       ASSERT_EQ(ptr, ptr + pagesize);
+
+       /* Clean up the excess pages left and right. */
+       munmap(ptr, pagesize);
+       munmap(ptr + 11, pagesize);
+       ptr + = pagesize;
+
        /* We shouldn't yet see a guard flag. */
        ASSERT_FALSE(check_vmflag_guard(ptr));
 
Signature · 1 line
Cheers,

It's enough but alloc_isolated_mem() could be modified to allocate with PROT_NONE first and then change the prot.

Quoted 4 lines

It seems whether make a helper or testcase should write the code properly in case of unwanted VMA merge.

IMHO, instead of testcase write them all

So far I am aware of exactly one test case, which others do absolutely need this?

Signature · 3 lines
Cheers,

David
#21 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 55 lines · David Hildenbrand (Arm)
Quoted 54 lines · Yeoreum Yun
Quoted 9 lines · Yeoreum Yun
Quoted 7 lines

Yes. so if we remove ASSERT_FALSE(check_vmflag_guard(ptr)), TBH we don't need this patch unless other usage comes up to prevent unwanted VMA merge.

Would it be better to drop ASSERT_FALSE(check_vmflag_guard(ptr)) in guard test?

Quoted 6 lines

I guess there is value in asserting that not all VMAs by accident start with an over-indication of maybe having guard pages, which is why Lorenzo added that check :)

But I think even alloc_isolated_mem() is wrong in that regard: if the original VMA gets merged, we could inherit the guard-marker, no?

True. I overlooked guard bit is sticky.

Quoted 32 lines

Maybe the following would be good enough?

tools/testing/selftests/mm/guard-regions.c +12 -2
index 5c8ec3ca75d7d..791bf6a68b9e6 100644
@@ -2257,10 +2257,20 @@ TEST_F(guard_regions, smaps)
        char *ptr, *ptr2;
        int i;
 
-       /* Map a region. */
-       ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0);
+       /* Reserve a 10 page region with 1 page space to both sides. */
+       ptr = mmap_(self, variant, NULL, 12 * page_size, PROT_NONE, 0, 0);
        ASSERT_NE(ptr, MAP_FAILED);
 
+       /* Map a new region that is guaranteed to not get merged in any way. */
+       ptr = mmap_(self, variant | MAP_FIXED, ptr + pagesize, 10 * page_size,
+                   PROT_READ | PROT_WRITE, 0, 0);
+       ASSERT_EQ(ptr, ptr + pagesize);
+
+       /* Clean up the excess pages left and right. */
+       munmap(ptr, pagesize);
+       munmap(ptr + 11, pagesize);
+       ptr + = pagesize;
+
        /* We shouldn't yet see a guard flag. */
        ASSERT_FALSE(check_vmflag_guard(ptr));
 
Signature · 1 line
Cheers,

It's enough but alloc_isolated_mem() could be modified to allocate with PROT_NONE first and then change the prot.

Quoted 6 lines
Quoted 4 lines

It seems whether make a helper or testcase should write the code properly in case of unwanted VMA merge.

IMHO, instead of testcase write them all

So far I am aware of exactly one test case, which others do absolutely need this?

True. I'll respin with your suggestion.

Thanks!

Signature · 2 lines
Sincerely,
Yeoreum Yun
#22 lore raw

Re: [PATCH v5 3/3] kselftest: mm: introduce alloc_isolated_mem()

Quoted 9 lines · Yeoreum Yun
Quoted 8 lines · David Hildenbrand (Arm)
Quoted 7 lines · Yeoreum Yun

True. I overlooked guard bit is sticky.

It's enough but alloc_isolated_mem() could be modified to allocate with PROT_NONE first and then change the prot.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

It seems whether make a helper or testcase should write the code properly in case of unwanted VMA merge.

IMHO, instead of testcase write them all

So far I am aware of exactly one test case, which others do absolutely need this?

True. I'll respin with your suggestion.

Don't get me wrong, if we'll need this for other tests we can add helpers. But for a single user adding a helper is not really worth it.

Signature · 3 lines
Cheers,

David
#23 lore raw

;, Re: ;, [PATCH v5 3/3] kselftest: ;, mm: introducealloc_isolated_mem;

Quoted 13 lines · David Hildenbrand (Arm)
Quoted 12 lines · Yeoreum Yun
Quoted 11 lines · David Hildenbrand (Arm)
Quoted 10 lines · Yeoreum Yun

True. I overlooked guard bit is sticky.

It's enough but alloc_isolated_mem() could be modified to allocate with PROT_NONE first and then change the prot.

It seems whether make a helper or testcase should write the code properly in case of unwanted VMA merge.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

IMHO, instead of testcase write them all

So far I am aware of exactly one test case, which others do absolutely need this?

True. I'll respin with your suggestion.

Don't get me wrong, if we'll need this for other tests we can add helpers. But for a single user adding a helper is not really worth it.

No worries. Since I thought there is no use case right now except the test, I've agreed.

Thanks!

Signature · 2 lines
Sincerely,
Yeoreum Yun
#24 lore raw
In reply to Yeoreum Yun
Quoted 28 lines · Yeoreum Yun

split_huge_page_test can fail for the following reasons:

1. During the test, khugepaged may collapse previously split pages again, causing intermittent failures.

2. Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign(). The underlying VMA may start at a different address from the aligned address returned by memalign(). Moreover, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because it already has the same advice.

This causes the test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Address these issues by applying MADV_NOHUGEPAGE after faulting in the huge page, preventing khugepaged from collapsing it again, and instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

Quoted 3 lines

Also, current usage of memalign() would result memory area may unexpectedly merge with an adjacent VMA, causing tests that inspect it through /proc/self/smaps to fail.

I'm not particularly happy about this.

Relying on VMA merging details rather hints that we shouldn't be using smaps to query some stats/properties.

Which exact things are test querying through /proc/self/smaps? Could we convert the code to just query that stuff through different interfaces?

Signature · 3 lines
Cheers,

David
#25 lore raw
Quoted 32 lines · Yeoreum Yun
Quoted 28 lines

split_huge_page_test can fail for the following reasons:

1. During the test, khugepaged may collapse previously split pages again, causing intermittent failures.

2. Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign(). The underlying VMA may start at a different address from the aligned address returned by memalign(). Moreover, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because it already has the same advice.

This causes the test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Address these issues by applying MADV_NOHUGEPAGE after faulting in the huge page, preventing khugepaged from collapsing it again, and instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

Quoted 3 lines

Also, current usage of memalign() would result memory area may unexpectedly merge with an adjacent VMA, causing tests that inspect it through /proc/self/smaps to fail.

Quoted 7 lines

I'm not particularly happy about this.

Relying on VMA merging details rather hints that we shouldn't be using smaps to query some stats/properties.

Which exact things are test querying through /proc/self/smaps? Could we convert the code to just query that stuff through different interfaces?

Well, users currently for using /proc/self/smaps are for check vm-flags:
- guard-regions where using check_vmflags_guard()
- pfnmap test where uses check_vmflag_pfnmap()

AFAIK, there is no interface to get vm-flags except /proc/self/smaps, we should add new interface but I'm not sure whether it's useful except for test purpose.

Since for a testing, the most chance for VMA merge is when using anon private mapping and this would be enough with allocate_isolated_mem().

Signature · 2 lines
Sincerely,
Yeoreum Yun
#26 lore raw
Quoted 37 lines · Yeoreum Yun
Quoted 37 lines · Yeoreum Yun
Quoted 28 lines

split_huge_page_test can fail for the following reasons:

1. During the test, khugepaged may collapse previously split pages again, causing intermittent failures.

2. Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign(). The underlying VMA may start at a different address from the aligned address returned by memalign(). Moreover, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because it already has the same advice.

This causes the test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Address these issues by applying MADV_NOHUGEPAGE after faulting in the huge page, preventing khugepaged from collapsing it again, and instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

Quoted 3 lines

Also, current usage of memalign() would result memory area may unexpectedly merge with an adjacent VMA, causing tests that inspect it through /proc/self/smaps to fail.

I'm not particularly happy about this.

Relying on VMA merging details rather hints that we shouldn't be using smaps to query some stats/properties.

Quoted 6 lines
Quoted 2 lines

Which exact things are test querying through /proc/self/smaps? Could we convert the code to just query that stuff through different interfaces?

Well, users currently for using /proc/self/smaps are for check vm-flags:
- guard-regions where using check_vmflags_guard()
- pfnmap test where uses check_vmflag_pfnmap()

Most vm-flags should not be an issue when it comes to merging. The only exception are vmflags that do not prevent VMA merging.

So it's VM_SOFTDIRTY and VM_MAYBE_GUARD. And I agree that for guard-regions.c we likely have to care such that we don't merge by accident with other VMAs (guard regions).

But that's independent of memalign.

ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0); ASSERT_FALSE(check_vmflag_guard(ptr));

could be problematic on its own (unlikely but possible).

For other flags, you really only have to find the smaps area that covers the given address and look at the vm-flags.

Or am I missing something important?

(merging vnas with VM_PFNMAP is impossible right now IIRC)

Signature · 3 lines
Cheers,

David
#27 lore raw
Quoted 52 lines · David Hildenbrand (Arm)
Quoted 37 lines · Yeoreum Yun
Quoted 37 lines · Yeoreum Yun
Quoted 28 lines

split_huge_page_test can fail for the following reasons:

1. During the test, khugepaged may collapse previously split pages again, causing intermittent failures.

2. Since glibc commit 321e1fc73f (“malloc: Enable 2MB THP by default on AArch64”), glibc may call madvise(MADV_HUGEPAGE) for sufficiently large allocations made by memalign(). The underlying VMA may start at a different address from the aligned address returned by memalign(). Moreover, a subsequent madvise(MADV_HUGEPAGE) call does not split the VMA because it already has the same advice.

This causes the test to fail because the check_huge_xxx() helpers incorrectly require the address returned by memalign() to match the VMA start address reported in /proc/self/smaps.

Address these issues by applying MADV_NOHUGEPAGE after faulting in the huge page, preventing khugepaged from collapsing it again, and instead of relying on /proc/self/smaps, use /proc/self/pagemap and /proc/kpageflags to detect huge-page mappings and large folios:

1. If hpage_size == pmd_pagesize, check PAGE_IS_HUGE instead of using check_large_folios(), since only the mapping type matters. This identifies PMD-mapped huge pages.
2. Otherwise, use check_large_folios() to detect large folios. This covers mTHP cases.
3. Check the folio flags according to the type of huge page.

Quoted 3 lines

Also, current usage of memalign() would result memory area may unexpectedly merge with an adjacent VMA, causing tests that inspect it through /proc/self/smaps to fail.

I'm not particularly happy about this.

Relying on VMA merging details rather hints that we shouldn't be using smaps to query some stats/properties.

Quoted 6 lines
Quoted 2 lines

Which exact things are test querying through /proc/self/smaps? Could we convert the code to just query that stuff through different interfaces?

Well, users currently for using /proc/self/smaps are for check vm-flags:
- guard-regions where using check_vmflags_guard()
- pfnmap test where uses check_vmflag_pfnmap()

Most vm-flags should not be an issue when it comes to merging. The only exception are vmflags that do not prevent VMA merging.

So it's VM_SOFTDIRTY and VM_MAYBE_GUARD. And I agree that for guard-regions.c we likely have to care such that we don't merge by accident with other VMAs (guard regions).

Quoted 6 lines

But that's independent of memalign.

ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0); ASSERT_FALSE(check_vmflag_guard(ptr));

could be problematic on its own (unlikely but possible).

Yes. That's why I'm think it would be good to use alloc_isolated_mem() in case of ANON mapping for this case.

Quoted 7 lines

For other flags, you really only have to find the smaps area that covers the given address and look at the vm-flags.

Or am I missing something important?

(merging vnas with VM_PFNMAP is impossible right now IIRC)

No. what I want to say including the patch #3 is for the above case where you point out -- ASSERT_FALSE(check_vmflag_guard(ptr)).

Since we don't have any interface to check vm_flags execpt smap and for memory mmaped with anon would have a chance to merge, We need something to replace memalign() with preventing unexpected VMA merge. (But I forgot to change the those case in guard test case).

Signature · 2 lines
Sincerely,
Yeoreum Yun
#28 lore raw
Quoted 16 lines · Yeoreum Yun
Quoted 15 lines · David Hildenbrand (Arm)
Quoted 4 lines · Yeoreum Yun

Well, users currently for using /proc/self/smaps are for check vm-flags:
- guard-regions where using check_vmflags_guard()
- pfnmap test where uses check_vmflag_pfnmap()

Most vm-flags should not be an issue when it comes to merging. The only exception are vmflags that do not prevent VMA merging.

So it's VM_SOFTDIRTY and VM_MAYBE_GUARD. And I agree that for guard-regions.c we likely have to care such that we don't merge by accident with other VMAs (guard regions).

But that's independent of memalign.

Quoted 7 lines
Quoted 4 lines

ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0); ASSERT_FALSE(check_vmflag_guard(ptr));

could be problematic on its own (unlikely but possible).

Yes. That's why I'm think it would be good to use alloc_isolated_mem() in case of ANON mapping for this case.

It's really only guest-region code that needs this.

Quoted 9 lines
Quoted 8 lines

For other flags, you really only have to find the smaps area that covers the given address and look at the vm-flags.

Or am I missing something important?

(merging vnas with VM_PFNMAP is impossible right now IIRC)

Quoted 7 lines

No. what I want to say including the patch #3 is for the above case where you point out -- ASSERT_FALSE(check_vmflag_guard(ptr)).

Since we don't have any interface to check vm_flags execpt smap and for memory mmaped with anon would have a chance to merge, We need something to replace memalign() with preventing unexpected VMA merge.

Only for the cases that actually really needs this, which is in my understanding guard-regions.

And I repeat, this is not a memalign() problem.

Signature · 3 lines
Cheers,

David
#29 lore raw
Quoted 39 lines · David Hildenbrand (Arm)
Quoted 16 lines · Yeoreum Yun
Quoted 15 lines · David Hildenbrand (Arm)
Quoted 4 lines · Yeoreum Yun

Well, users currently for using /proc/self/smaps are for check vm-flags:
- guard-regions where using check_vmflags_guard()
- pfnmap test where uses check_vmflag_pfnmap()

Most vm-flags should not be an issue when it comes to merging. The only exception are vmflags that do not prevent VMA merging.

So it's VM_SOFTDIRTY and VM_MAYBE_GUARD. And I agree that for guard-regions.c we likely have to care such that we don't merge by accident with other VMAs (guard regions).

But that's independent of memalign.

Quoted 7 lines
Quoted 4 lines

ptr = mmap_(self, variant, NULL, 10 * page_size, PROT_READ | PROT_WRITE, 0, 0); ASSERT_FALSE(check_vmflag_guard(ptr));

could be problematic on its own (unlikely but possible).

Yes. That's why I'm think it would be good to use alloc_isolated_mem() in case of ANON mapping for this case.

It's really only guest-region code that needs this.

Quoted 12 lines
Quoted 8 lines

For other flags, you really only have to find the smaps area that covers the given address and look at the vm-flags.

Or am I missing something important?

(merging vnas with VM_PFNMAP is impossible right now IIRC)

No. what I want to say including the patch #3 is for the above case where you point out -- ASSERT_FALSE(check_vmflag_guard(ptr)).

Quoted 8 lines
Quoted 4 lines

Since we don't have any interface to check vm_flags execpt smap and for memory mmaped with anon would have a chance to merge, We need something to replace memalign() with preventing unexpected VMA merge.

Only for the cases that actually really needs this, which is in my understanding guard-regions.

And I repeat, this is not a memalign() problem.

Yes. I'm not claim memalign() is problem but want to prevent unwanted VMA merge. That's all.

And as I mentioned in another reply, if we get rid of ASSERT_FALSE(check_vmflag_guard(ptr)), this patch would be droppable.

Signature · 2 lines
Sincerely,
Yeoreum Yun