[PATCH v2 0/3] Add hugetlb MADV_DONTNEED support

[PATCH v2 0/3] Add hugetlb MADV_DONTNEED support

[Mike Kravetz]

Userfaultfd selftests for hugetlb does not perform UFFD_EVENT_REMAP
testing.  However, mremap support was recently added in commit
550a7d60bd5e ("mm, hugepages: add mremap() support for hugepage backed
vma").  While attempting to enable mremap support in the test, it was
discovered that the mremap test indirectly depends on MADV_DONTNEED.

madvise does not allow MADV_DONTNEED for hugetlb mappings.  However,
that is primarily due to the check in can_madv_lru_vma().  By simply
removing the check and adding huge page alignment, MADV_DONTNEED can
be made to work for hugetlb mappings.

Do note that there is no compelling use case for adding this support.
This was discussed in the RFC [1].  However, adding support makes sense
as it is fairly trivial and brings hugetlb functionality more in line
with 'normal' memory.

After enabling support, add selftest for MADV_DONTNEED as well as
MADV_REMOVE.  Then update userfaultfd selftest.

v1 -> v2
- Use is_vm_hugetlb_page() instead of open coding vma hugetlb check.
- Add new test to .gitignore and use meaningful symbolic names (#define)
  for constants used in test.  Shuah
- Updated help text in userfaultfd test and modified run_vmtests to not
  pass in a file for userfaultfd hugetlb test.  Axel
- Added Reviewed-by for selftest patches.

RFC -> v1
- Fixed alignment issues when calling zap_page_range.  Naoya
- Added checks for invalid arguments and misalignment to selftest.

[1] https://lore.kernel.org/linux-mm/20220113180308.15610-1-mike.kravetz@oracle.com/

Mike Kravetz (3):
  mm: enable MADV_DONTNEED for hugetlb mappings
  selftests/vm: add hugetlb madvise MADV_DONTNEED MADV_REMOVE test
  userfaultfd/selftests: enable huegtlb remap and remove event testing

 mm/madvise.c                                 |  24 +-
 tools/testing/selftests/vm/.gitignore        |   1 +
 tools/testing/selftests/vm/Makefile          |   1 +
 tools/testing/selftests/vm/hugetlb-madvise.c | 413 +++++++++++++++++++
 tools/testing/selftests/vm/run_vmtests.sh    |  15 +-
 tools/testing/selftests/vm/userfaultfd.c     |  69 ++--
 6 files changed, 485 insertions(+), 38 deletions(-)
 create mode 100644 tools/testing/selftests/vm/hugetlb-madvise.c

-- 
2.34.1

[PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Mike Kravetz]

MADV_DONTNEED is currently disabled for hugetlb mappings.  This
certainly makes sense in shared file mappings as the pagecache maintains
a reference to the page and it will never be freed.  However, it could
be useful to unmap and free pages in private mappings.

The only thing preventing MADV_DONTNEED from working on hugetlb mappings
is a check in can_madv_lru_vma().  To allow support for hugetlb mappings
create and use a new routine madvise_dontneed_free_valid_vma() that will
allow hugetlb mappings.  Also, before calling zap_page_range in the
DONTNEED case align start and size to huge page size for hugetlb vmas.
madvise only requires PAGE_SIZE alignment, but the hugetlb unmap routine
requires huge page size alignment.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/madvise.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 5604064df464..7ae891e030a4 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -796,10 +796,30 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
 static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
 					unsigned long start, unsigned long end)
 {
+	/*
+	 * start and size (end - start) must be huge page size aligned
+	 * for hugetlb vmas.
+	 */
+	if (is_vm_hugetlb_page(vma)) {
+		struct hstate *h = hstate_vma(vma);
+
+		start = ALIGN_DOWN(start, huge_page_size(h));
+		end = ALIGN(end, huge_page_size(h));
+	}
+
 	zap_page_range(vma, start, end - start);
 	return 0;
 }
 
+static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
+						int behavior)
+{
+	if (is_vm_hugetlb_page(vma))
+		return behavior == MADV_DONTNEED;
+	else
+		return can_madv_lru_vma(vma);
+}
+
 static long madvise_dontneed_free(struct vm_area_struct *vma,
 				  struct vm_area_struct **prev,
 				  unsigned long start, unsigned long end,
@@ -808,7 +828,7 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
 	struct mm_struct *mm = vma->vm_mm;
 
 	*prev = vma;
-	if (!can_madv_lru_vma(vma))
+	if (!madvise_dontneed_free_valid_vma(vma, behavior))
 		return -EINVAL;
 
 	if (!userfaultfd_remove(vma, start, end)) {
@@ -830,7 +850,7 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
 			 */
 			return -ENOMEM;
 		}
-		if (!can_madv_lru_vma(vma))
+		if (!madvise_dontneed_free_valid_vma(vma, behavior))
 			return -EINVAL;
 		if (end > vma->vm_end) {
 			/*
-- 
2.34.1

[PATCH v2 2/3] selftests/vm: add hugetlb madvise MADV_DONTNEED MADV_REMOVE test

[Mike Kravetz]

Now that MADV_DONTNEED support for hugetlb is enabled, add corresponding
tests.  MADV_REMOVE has been enabled for some time, but no tests exist
so add them as well.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
---
 tools/testing/selftests/vm/.gitignore        |   1 +
 tools/testing/selftests/vm/Makefile          |   1 +
 tools/testing/selftests/vm/hugetlb-madvise.c | 413 +++++++++++++++++++
 tools/testing/selftests/vm/run_vmtests.sh    |  12 +
 4 files changed, 427 insertions(+)
 create mode 100644 tools/testing/selftests/vm/hugetlb-madvise.c

diff --git a/tools/testing/selftests/vm/.gitignore b/tools/testing/selftests/vm/.gitignore
index 3b5faec3c04f..d7507f3c7c76 100644
--- a/tools/testing/selftests/vm/.gitignore
+++ b/tools/testing/selftests/vm/.gitignore
@@ -3,6 +3,7 @@ hugepage-mmap
 hugepage-mremap
 hugepage-shm
 hugepage-vmemmap
+hugetlb-madvise
 khugepaged
 map_hugetlb
 map_populate
diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
index 96714d2d49dc..5e43f072f5b7 100644
--- a/tools/testing/selftests/vm/Makefile
+++ b/tools/testing/selftests/vm/Makefile
@@ -28,6 +28,7 @@ LDLIBS = -lrt -lpthread
 TEST_GEN_FILES = compaction_test
 TEST_GEN_FILES += gup_test
 TEST_GEN_FILES += hmm-tests
+TEST_GEN_FILES += hugetlb-madvise
 TEST_GEN_FILES += hugepage-mmap
 TEST_GEN_FILES += hugepage-mremap
 TEST_GEN_FILES += hugepage-shm
diff --git a/tools/testing/selftests/vm/hugetlb-madvise.c b/tools/testing/selftests/vm/hugetlb-madvise.c
new file mode 100644
index 000000000000..77af0ffff01d
--- /dev/null
+++ b/tools/testing/selftests/vm/hugetlb-madvise.c
@@ -0,0 +1,413 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * hugepage-madvise:
+ *
+ * Basic functional testing of madvise MADV_DONTNEED and MADV_REMOVE
+ * on hugetlb mappings.
+ *
+ * Before running this test, make sure the administrator has pre-allocated
+ * at least MIN_FREE_PAGES hugetlb pages and they are free.  In addition,
+ * the test takes an argument that is the path to a file in a hugetlbfs
+ * filesystem.  Therefore, a hugetlbfs filesystem must be mounted on some
+ * directory.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/mman.h>
+#define __USE_GNU
+#include <fcntl.h>
+
+#define USAGE	"USAGE: %s <hugepagefile_name>\n"
+#define MIN_FREE_PAGES	20
+#define NR_HUGE_PAGES	10	/* common map/allocation value */
+
+#define validate_free_pages(exp_free)					\
+	do {								\
+		int fhp = get_free_hugepages();				\
+		if (fhp != (exp_free)) {				\
+			printf("Unexpected number of free huge "	\
+				"pages line %d\n", __LINE__);		\
+			exit(1);					\
+		}							\
+	} while (0)
+
+unsigned long huge_page_size;
+unsigned long base_page_size;
+
+/*
+ * default_huge_page_size copied from mlock2-tests.c
+ */
+unsigned long default_huge_page_size(void)
+{
+	unsigned long hps = 0;
+	char *line = NULL;
+	size_t linelen = 0;
+	FILE *f = fopen("/proc/meminfo", "r");
+
+	if (!f)
+		return 0;
+	while (getline(&line, &linelen, f) > 0) {
+		if (sscanf(line, "Hugepagesize:       %lu kB", &hps) == 1) {
+			hps <<= 10;
+			break;
+		}
+	}
+
+	free(line);
+	fclose(f);
+	return hps;
+}
+
+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;
+
+	for (i = 0; i < nr_pages; i++)
+		*((unsigned long *)(addr + (i * huge_page_size))) = i;
+}
+
+void read_fault_pages(void *addr, unsigned long nr_pages)
+{
+	unsigned long i, tmp;
+
+	for (i = 0; i < nr_pages; i++)
+		tmp += *((unsigned long *)(addr + (i * huge_page_size)));
+}
+
+int main(int argc, char **argv)
+{
+	unsigned long free_hugepages;
+	void *addr, *addr2;
+	int fd;
+	int ret;
+
+	if (argc != 2) {
+		printf(USAGE, argv[0]);
+		exit(1);
+	}
+
+	huge_page_size = default_huge_page_size();
+	if (!huge_page_size) {
+		printf("Unable to determine huge page size, exiting!\n");
+		exit(1);
+	}
+	base_page_size = sysconf(_SC_PAGE_SIZE);
+	if (!huge_page_size) {
+		printf("Unable to determine base page size, exiting!\n");
+		exit(1);
+	}
+
+	free_hugepages = get_free_hugepages();
+	if (free_hugepages < MIN_FREE_PAGES) {
+		printf("Not enough free huge pages to test, exiting!\n");
+		exit(1);
+	}
+
+	fd = open(argv[1], O_CREAT | O_RDWR, 0755);
+	if (fd < 0) {
+		perror("Open failed");
+		exit(1);
+	}
+
+	/*
+	 * Test validity of MADV_DONTNEED addr and length arguments.  mmap
+	 * size is NR_HUGE_PAGES + 2.  One page at the beginning and end of
+	 * the mapping will be unmapped so we KNOW there is nothing mapped
+	 * there.
+	 */
+	addr = mmap(NULL, (NR_HUGE_PAGES + 2) * huge_page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
+			-1, 0);
+	if (addr == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+	if (munmap(addr, huge_page_size) ||
+			munmap(addr + (NR_HUGE_PAGES + 1) * huge_page_size,
+				huge_page_size)) {
+		perror("munmap");
+		exit(1);
+	}
+	addr = addr + huge_page_size;
+
+	write_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* addr before mapping should fail */
+	ret = madvise(addr - base_page_size, NR_HUGE_PAGES * huge_page_size,
+		MADV_DONTNEED);
+	if (!ret) {
+		printf("Unexpected success of madvise call with invalid addr line %d\n",
+				__LINE__);
+			exit(1);
+	}
+
+	/* addr + length after mapping should fail */
+	ret = madvise(addr, (NR_HUGE_PAGES * huge_page_size) + base_page_size,
+		MADV_DONTNEED);
+	if (!ret) {
+		printf("Unexpected success of madvise call with invalid length line %d\n",
+				__LINE__);
+			exit(1);
+	}
+
+	(void)munmap(addr, NR_HUGE_PAGES * huge_page_size);
+
+	/*
+	 * Test alignment of MADV_DONTNEED addr and length arguments
+	 */
+	addr = mmap(NULL, NR_HUGE_PAGES * huge_page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
+			-1, 0);
+	if (addr == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+	write_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* addr should be aligned down to huge page size */
+	if (madvise(addr + base_page_size,
+			NR_HUGE_PAGES * huge_page_size - base_page_size,
+			MADV_DONTNEED)) {
+		perror("madvise");
+		exit(1);
+	}
+
+	/* should free all pages in mapping */
+	validate_free_pages(free_hugepages);
+
+	write_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* addr + length should be aligned up to huge page size */
+	if (madvise(addr, (NR_HUGE_PAGES * huge_page_size) - base_page_size,
+			MADV_DONTNEED)) {
+		perror("madvise");
+		exit(1);
+	}
+
+	/* should free all pages in mapping */
+	validate_free_pages(free_hugepages);
+
+	(void)munmap(addr, NR_HUGE_PAGES * huge_page_size);
+
+	/*
+	 * Test MADV_DONTNEED on anonymous private mapping
+	 */
+	addr = mmap(NULL, NR_HUGE_PAGES * huge_page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB,
+			-1, 0);
+	if (addr == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+	write_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	if (madvise(addr, NR_HUGE_PAGES * huge_page_size, MADV_DONTNEED)) {
+		perror("madvise");
+		exit(1);
+	}
+
+	/* should free all pages in mapping */
+	validate_free_pages(free_hugepages);
+
+	(void)munmap(addr, NR_HUGE_PAGES * huge_page_size);
+
+	/*
+	 * Test MADV_DONTNEED on private mapping of hugetlb file
+	 */
+	if (fallocate(fd, 0, 0, NR_HUGE_PAGES * huge_page_size)) {
+		perror("fallocate");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	addr = mmap(NULL, NR_HUGE_PAGES * huge_page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE, fd, 0);
+	if (addr == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+
+	/* read should not consume any pages */
+	read_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* madvise should not free any pages */
+	if (madvise(addr, NR_HUGE_PAGES * huge_page_size, MADV_DONTNEED)) {
+		perror("madvise");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* writes should allocate private pages */
+	write_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - (2 * NR_HUGE_PAGES));
+
+	/* madvise should free private pages */
+	if (madvise(addr, NR_HUGE_PAGES * huge_page_size, MADV_DONTNEED)) {
+		perror("madvise");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* writes should allocate private pages */
+	write_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - (2 * NR_HUGE_PAGES));
+
+	/*
+	 * The fallocate below certainly should free the pages associated
+	 * with the file.  However, pages in the private mapping are also
+	 * freed.  This is not the 'correct' behavior, but is expected
+	 * because this is how it has worked since the initial hugetlb
+	 * implementation.
+	 */
+	if (fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+					0, NR_HUGE_PAGES * huge_page_size)) {
+		perror("fallocate");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages);
+
+	(void)munmap(addr, NR_HUGE_PAGES * huge_page_size);
+
+	/*
+	 * Test MADV_DONTNEED on shared mapping of hugetlb file
+	 */
+	if (fallocate(fd, 0, 0, NR_HUGE_PAGES * huge_page_size)) {
+		perror("fallocate");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	addr = mmap(NULL, NR_HUGE_PAGES * huge_page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_SHARED, fd, 0);
+	if (addr == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+
+	/* write should not consume any pages */
+	write_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* madvise should not free any pages */
+	if (madvise(addr, NR_HUGE_PAGES * huge_page_size, MADV_DONTNEED)) {
+		perror("madvise");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/*
+	 * Test MADV_REMOVE on shared mapping of hugetlb file
+	 *
+	 * madvise is same as hole punch and should free all pages.
+	 */
+	if (madvise(addr, NR_HUGE_PAGES * huge_page_size, MADV_REMOVE)) {
+		perror("madvise");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages);
+	(void)munmap(addr, NR_HUGE_PAGES * huge_page_size);
+
+	/*
+	 * Test MADV_REMOVE on shared and private mapping of hugetlb file
+	 */
+	if (fallocate(fd, 0, 0, NR_HUGE_PAGES * huge_page_size)) {
+		perror("fallocate");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	addr = mmap(NULL, NR_HUGE_PAGES * huge_page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_SHARED, fd, 0);
+	if (addr == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+
+	/* shared write should not consume any additional pages */
+	write_fault_pages(addr, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	addr2 = mmap(NULL, NR_HUGE_PAGES * huge_page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE, fd, 0);
+	if (addr2 == MAP_FAILED) {
+		perror("mmap");
+		exit(1);
+	}
+
+	/* private read should not consume any pages */
+	read_fault_pages(addr2, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* private write should consume additional pages */
+	write_fault_pages(addr2, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - (2 * NR_HUGE_PAGES));
+
+	/* madvise of shared mapping should not free any pages */
+	if (madvise(addr, NR_HUGE_PAGES * huge_page_size, MADV_DONTNEED)) {
+		perror("madvise");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages - (2 * NR_HUGE_PAGES));
+
+	/* madvise of private mapping should free private pages */
+	if (madvise(addr2, NR_HUGE_PAGES * huge_page_size, MADV_DONTNEED)) {
+		perror("madvise");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages - NR_HUGE_PAGES);
+
+	/* private write should consume additional pages again */
+	write_fault_pages(addr2, NR_HUGE_PAGES);
+	validate_free_pages(free_hugepages - (2 * NR_HUGE_PAGES));
+
+	/*
+	 * madvise should free both file and private pages although this is
+	 * not correct.  private pages should not be freed, but this is
+	 * expected.  See comment associated with FALLOC_FL_PUNCH_HOLE call.
+	 */
+	if (madvise(addr, NR_HUGE_PAGES * huge_page_size, MADV_REMOVE)) {
+		perror("madvise");
+		exit(1);
+	}
+	validate_free_pages(free_hugepages);
+
+	(void)munmap(addr, NR_HUGE_PAGES * huge_page_size);
+	(void)munmap(addr2, NR_HUGE_PAGES * huge_page_size);
+
+	close(fd);
+	unlink(argv[1]);
+	return 0;
+}
diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh
index e09040a3dc08..ff901ee66838 100755
--- a/tools/testing/selftests/vm/run_vmtests.sh
+++ b/tools/testing/selftests/vm/run_vmtests.sh
@@ -130,6 +130,18 @@ else
 	echo "[PASS]"
 fi
 
+echo "-----------------------"
+echo "running hugetlb-madvise"
+echo "-----------------------"
+./hugetlb-madvise $mnt/madvise-test
+if [ $? -ne 0 ]; then
+	echo "[FAIL]"
+	exitcode=1
+else
+	echo "[PASS]"
+fi
+rm -f $mnt/madvise-test
+
 echo "NOTE: The above hugetlb tests provide minimal coverage.  Use"
 echo "      https://github.com/libhugetlbfs/libhugetlbfs.git for"
 echo "      hugetlb regression testing."
-- 
2.34.1

[PATCH v2 3/3] userfaultfd/selftests: enable huegtlb remap and remove event testing

[Mike Kravetz]

With MADV_DONTNEED support added to hugetlb mappings, mremap testing
can also be enabled for hugetlb.

Modify the tests to use madvise MADV_DONTNEED and MADV_REMOVE instead of
fallocate hole puch for releasing hugetlb pages.

Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
Reviewed-by: Axel Rasmussen <axelrasmussen@google.com>
---
 tools/testing/selftests/vm/run_vmtests.sh |  3 +-
 tools/testing/selftests/vm/userfaultfd.c  | 69 ++++++++++++-----------
 2 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh
index ff901ee66838..7178d6fa8c92 100755
--- a/tools/testing/selftests/vm/run_vmtests.sh
+++ b/tools/testing/selftests/vm/run_vmtests.sh
@@ -207,14 +207,13 @@ echo "running userfaultfd_hugetlb"
 echo "---------------------------"
 # Test requires source and destination huge pages.  Size of source
 # (half_ufd_size_MB) is passed as argument to test.
-./userfaultfd hugetlb $half_ufd_size_MB 32 $mnt/ufd_test_file
+./userfaultfd hugetlb $half_ufd_size_MB 32
 if [ $? -ne 0 ]; then
 	echo "[FAIL]"
 	exitcode=1
 else
 	echo "[PASS]"
 fi
-rm -f $mnt/ufd_test_file
 
 echo "-------------------------"
 echo "running userfaultfd_shmem"
diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
index d2480ab93037..c18beebc0dd6 100644
--- a/tools/testing/selftests/vm/userfaultfd.c
+++ b/tools/testing/selftests/vm/userfaultfd.c
@@ -88,7 +88,6 @@ static bool test_uffdio_minor = false;
 static bool map_shared;
 static int shm_fd;
 static int huge_fd;
-static char *huge_fd_off0;
 static unsigned long long *count_verify;
 static int uffd = -1;
 static int uffd_flags, finished, *pipefd;
@@ -127,9 +126,9 @@ const char *examples =
     "./userfaultfd anon 100 99999\n\n"
     "# Run share memory test on 1GiB region with 99 bounces:\n"
     "./userfaultfd shmem 1000 99\n\n"
-    "# Run hugetlb memory test on 256MiB region with 50 bounces (using /dev/hugepages/hugefile):\n"
-    "./userfaultfd hugetlb 256 50 /dev/hugepages/hugefile\n\n"
-    "# Run the same hugetlb test but using shmem:\n"
+    "# Run hugetlb memory test on 256MiB region with 50 bounces:\n"
+    "./userfaultfd hugetlb 256 50\n\n"
+    "# Run the same hugetlb test but using shared file:\n"
     "./userfaultfd hugetlb_shared 256 50 /dev/hugepages/hugefile\n\n"
     "# 10MiB-~6GiB 999 bounces anonymous test, "
     "continue forever unless an error triggers\n"
@@ -226,10 +225,13 @@ static void noop_alias_mapping(__u64 *start, size_t len, unsigned long offset)
 
 static void hugetlb_release_pages(char *rel_area)
 {
-	if (fallocate(huge_fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
-		      rel_area == huge_fd_off0 ? 0 : nr_pages * page_size,
-		      nr_pages * page_size))
-		err("fallocate() failed");
+	if (!map_shared) {
+		if (madvise(rel_area, nr_pages * page_size, MADV_DONTNEED))
+			err("madvise(MADV_DONTNEED) failed");
+	} else {
+		if (madvise(rel_area, nr_pages * page_size, MADV_REMOVE))
+			err("madvise(MADV_REMOVE) failed");
+	}
 }
 
 static void hugetlb_allocate_area(void **alloc_area)
@@ -237,26 +239,37 @@ static void hugetlb_allocate_area(void **alloc_area)
 	void *area_alias = NULL;
 	char **alloc_area_alias;
 
-	*alloc_area = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
-			   (map_shared ? MAP_SHARED : MAP_PRIVATE) |
-			   MAP_HUGETLB |
-			   (*alloc_area == area_src ? 0 : MAP_NORESERVE),
-			   huge_fd, *alloc_area == area_src ? 0 :
-			   nr_pages * page_size);
+	if (!map_shared)
+		*alloc_area = mmap(NULL,
+			nr_pages * page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB |
+				(*alloc_area == area_src ? 0 : MAP_NORESERVE),
+			-1,
+			0);
+	else
+		*alloc_area = mmap(NULL,
+			nr_pages * page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_SHARED |
+				(*alloc_area == area_src ? 0 : MAP_NORESERVE),
+			huge_fd,
+			*alloc_area == area_src ? 0 : nr_pages * page_size);
 	if (*alloc_area == MAP_FAILED)
 		err("mmap of hugetlbfs file failed");
 
 	if (map_shared) {
-		area_alias = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
-				  MAP_SHARED | MAP_HUGETLB,
-				  huge_fd, *alloc_area == area_src ? 0 :
-				  nr_pages * page_size);
+		area_alias = mmap(NULL,
+			nr_pages * page_size,
+			PROT_READ | PROT_WRITE,
+			MAP_SHARED,
+			huge_fd,
+			*alloc_area == area_src ? 0 : nr_pages * page_size);
 		if (area_alias == MAP_FAILED)
 			err("mmap of hugetlb file alias failed");
 	}
 
 	if (*alloc_area == area_src) {
-		huge_fd_off0 = *alloc_area;
 		alloc_area_alias = &area_src_alias;
 	} else {
 		alloc_area_alias = &area_dst_alias;
@@ -269,12 +282,7 @@ static void hugetlb_alias_mapping(__u64 *start, size_t len, unsigned long offset
 {
 	if (!map_shared)
 		return;
-	/*
-	 * We can't zap just the pagetable with hugetlbfs because
-	 * MADV_DONTEED won't work. So exercise -EEXIST on a alias
-	 * mapping where the pagetables are not established initially,
-	 * this way we'll exercise the -EEXEC at the fs level.
-	 */
+
 	*start = (unsigned long) area_dst_alias + offset;
 }
 
@@ -427,7 +435,6 @@ static void uffd_test_ctx_clear(void)
 		uffd = -1;
 	}
 
-	huge_fd_off0 = NULL;
 	munmap_area((void **)&area_src);
 	munmap_area((void **)&area_src_alias);
 	munmap_area((void **)&area_dst);
@@ -925,10 +932,7 @@ static int faulting_process(int signal_test)
 	struct sigaction act;
 	unsigned long signalled = 0;
 
-	if (test_type != TEST_HUGETLB)
-		split_nr_pages = (nr_pages + 1) / 2;
-	else
-		split_nr_pages = nr_pages;
+	split_nr_pages = (nr_pages + 1) / 2;
 
 	if (signal_test) {
 		sigbuf = &jbuf;
@@ -985,9 +989,6 @@ static int faulting_process(int signal_test)
 	if (signal_test)
 		return signalled != split_nr_pages;
 
-	if (test_type == TEST_HUGETLB)
-		return 0;
-
 	area_dst = mremap(area_dst, nr_pages * page_size,  nr_pages * page_size,
 			  MREMAP_MAYMOVE | MREMAP_FIXED, area_src);
 	if (area_dst == MAP_FAILED)
@@ -1670,7 +1671,7 @@ int main(int argc, char **argv)
 	}
 	nr_pages = nr_pages_per_cpu * nr_cpus;
 
-	if (test_type == TEST_HUGETLB) {
+	if (test_type == TEST_HUGETLB && map_shared) {
 		if (argc < 5)
 			usage();
 		huge_fd = open(argv[4], O_CREAT | O_RDWR, 0755);
-- 
2.34.1

Re: [PATCH v2 3/3] userfaultfd/selftests: enable huegtlb remap and remove event testing

[Mike Rapoport]

On Tue, Feb 01, 2022 at 05:40:34PM -0800, Mike Kravetz wrote:
> Subject: [PATCH v2 3/3] userfaultfd/selftests: enable huegtlb remap and remove event testing

Nit:                                                  ^ hugetlb

> With MADV_DONTNEED support added to hugetlb mappings, mremap testing
> can also be enabled for hugetlb.
> 
> Modify the tests to use madvise MADV_DONTNEED and MADV_REMOVE instead of
> fallocate hole puch for releasing hugetlb pages.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> Reviewed-by: Axel Rasmussen <axelrasmussen@google.com>
> ---
>  tools/testing/selftests/vm/run_vmtests.sh |  3 +-
>  tools/testing/selftests/vm/userfaultfd.c  | 69 ++++++++++++-----------
>  2 files changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/testing/selftests/vm/run_vmtests.sh b/tools/testing/selftests/vm/run_vmtests.sh
> index ff901ee66838..7178d6fa8c92 100755
> --- a/tools/testing/selftests/vm/run_vmtests.sh
> +++ b/tools/testing/selftests/vm/run_vmtests.sh
> @@ -207,14 +207,13 @@ echo "running userfaultfd_hugetlb"
>  echo "---------------------------"
>  # Test requires source and destination huge pages.  Size of source
>  # (half_ufd_size_MB) is passed as argument to test.
> -./userfaultfd hugetlb $half_ufd_size_MB 32 $mnt/ufd_test_file
> +./userfaultfd hugetlb $half_ufd_size_MB 32
>  if [ $? -ne 0 ]; then
>  	echo "[FAIL]"
>  	exitcode=1
>  else
>  	echo "[PASS]"
>  fi
> -rm -f $mnt/ufd_test_file
>  
>  echo "-------------------------"
>  echo "running userfaultfd_shmem"
> diff --git a/tools/testing/selftests/vm/userfaultfd.c b/tools/testing/selftests/vm/userfaultfd.c
> index d2480ab93037..c18beebc0dd6 100644
> --- a/tools/testing/selftests/vm/userfaultfd.c
> +++ b/tools/testing/selftests/vm/userfaultfd.c
> @@ -88,7 +88,6 @@ static bool test_uffdio_minor = false;
>  static bool map_shared;
>  static int shm_fd;
>  static int huge_fd;
> -static char *huge_fd_off0;
>  static unsigned long long *count_verify;
>  static int uffd = -1;
>  static int uffd_flags, finished, *pipefd;
> @@ -127,9 +126,9 @@ const char *examples =
>      "./userfaultfd anon 100 99999\n\n"
>      "# Run share memory test on 1GiB region with 99 bounces:\n"
>      "./userfaultfd shmem 1000 99\n\n"
> -    "# Run hugetlb memory test on 256MiB region with 50 bounces (using /dev/hugepages/hugefile):\n"
> -    "./userfaultfd hugetlb 256 50 /dev/hugepages/hugefile\n\n"
> -    "# Run the same hugetlb test but using shmem:\n"
> +    "# Run hugetlb memory test on 256MiB region with 50 bounces:\n"
> +    "./userfaultfd hugetlb 256 50\n\n"
> +    "# Run the same hugetlb test but using shared file:\n"
>      "./userfaultfd hugetlb_shared 256 50 /dev/hugepages/hugefile\n\n"
>      "# 10MiB-~6GiB 999 bounces anonymous test, "
>      "continue forever unless an error triggers\n"
> @@ -226,10 +225,13 @@ static void noop_alias_mapping(__u64 *start, size_t len, unsigned long offset)
>  
>  static void hugetlb_release_pages(char *rel_area)
>  {
> -	if (fallocate(huge_fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -		      rel_area == huge_fd_off0 ? 0 : nr_pages * page_size,
> -		      nr_pages * page_size))
> -		err("fallocate() failed");
> +	if (!map_shared) {
> +		if (madvise(rel_area, nr_pages * page_size, MADV_DONTNEED))
> +			err("madvise(MADV_DONTNEED) failed");
> +	} else {
> +		if (madvise(rel_area, nr_pages * page_size, MADV_REMOVE))
> +			err("madvise(MADV_REMOVE) failed");
> +	}
>  }
>  
>  static void hugetlb_allocate_area(void **alloc_area)
> @@ -237,26 +239,37 @@ static void hugetlb_allocate_area(void **alloc_area)
>  	void *area_alias = NULL;
>  	char **alloc_area_alias;
>  
> -	*alloc_area = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
> -			   (map_shared ? MAP_SHARED : MAP_PRIVATE) |
> -			   MAP_HUGETLB |
> -			   (*alloc_area == area_src ? 0 : MAP_NORESERVE),
> -			   huge_fd, *alloc_area == area_src ? 0 :
> -			   nr_pages * page_size);
> +	if (!map_shared)
> +		*alloc_area = mmap(NULL,
> +			nr_pages * page_size,
> +			PROT_READ | PROT_WRITE,
> +			MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB |
> +				(*alloc_area == area_src ? 0 : MAP_NORESERVE),
> +			-1,
> +			0);
> +	else
> +		*alloc_area = mmap(NULL,
> +			nr_pages * page_size,
> +			PROT_READ | PROT_WRITE,
> +			MAP_SHARED |
> +				(*alloc_area == area_src ? 0 : MAP_NORESERVE),
> +			huge_fd,
> +			*alloc_area == area_src ? 0 : nr_pages * page_size);
>  	if (*alloc_area == MAP_FAILED)
>  		err("mmap of hugetlbfs file failed");
>  
>  	if (map_shared) {
> -		area_alias = mmap(NULL, nr_pages * page_size, PROT_READ | PROT_WRITE,
> -				  MAP_SHARED | MAP_HUGETLB,
> -				  huge_fd, *alloc_area == area_src ? 0 :
> -				  nr_pages * page_size);
> +		area_alias = mmap(NULL,
> +			nr_pages * page_size,
> +			PROT_READ | PROT_WRITE,
> +			MAP_SHARED,
> +			huge_fd,
> +			*alloc_area == area_src ? 0 : nr_pages * page_size);
>  		if (area_alias == MAP_FAILED)
>  			err("mmap of hugetlb file alias failed");
>  	}
>  
>  	if (*alloc_area == area_src) {
> -		huge_fd_off0 = *alloc_area;
>  		alloc_area_alias = &area_src_alias;
>  	} else {
>  		alloc_area_alias = &area_dst_alias;
> @@ -269,12 +282,7 @@ static void hugetlb_alias_mapping(__u64 *start, size_t len, unsigned long offset
>  {
>  	if (!map_shared)
>  		return;
> -	/*
> -	 * We can't zap just the pagetable with hugetlbfs because
> -	 * MADV_DONTEED won't work. So exercise -EEXIST on a alias
> -	 * mapping where the pagetables are not established initially,
> -	 * this way we'll exercise the -EEXEC at the fs level.
> -	 */
> +
>  	*start = (unsigned long) area_dst_alias + offset;
>  }
>  
> @@ -427,7 +435,6 @@ static void uffd_test_ctx_clear(void)
>  		uffd = -1;
>  	}
>  
> -	huge_fd_off0 = NULL;
>  	munmap_area((void **)&area_src);
>  	munmap_area((void **)&area_src_alias);
>  	munmap_area((void **)&area_dst);
> @@ -925,10 +932,7 @@ static int faulting_process(int signal_test)
>  	struct sigaction act;
>  	unsigned long signalled = 0;
>  
> -	if (test_type != TEST_HUGETLB)
> -		split_nr_pages = (nr_pages + 1) / 2;
> -	else
> -		split_nr_pages = nr_pages;
> +	split_nr_pages = (nr_pages + 1) / 2;
>  
>  	if (signal_test) {
>  		sigbuf = &jbuf;
> @@ -985,9 +989,6 @@ static int faulting_process(int signal_test)
>  	if (signal_test)
>  		return signalled != split_nr_pages;
>  
> -	if (test_type == TEST_HUGETLB)
> -		return 0;
> -
>  	area_dst = mremap(area_dst, nr_pages * page_size,  nr_pages * page_size,
>  			  MREMAP_MAYMOVE | MREMAP_FIXED, area_src);
>  	if (area_dst == MAP_FAILED)
> @@ -1670,7 +1671,7 @@ int main(int argc, char **argv)
>  	}
>  	nr_pages = nr_pages_per_cpu * nr_cpus;
>  
> -	if (test_type == TEST_HUGETLB) {
> +	if (test_type == TEST_HUGETLB && map_shared) {
>  		if (argc < 5)
>  			usage();
>  		huge_fd = open(argv[4], O_CREAT | O_RDWR, 0755);
> -- 
> 2.34.1
> 
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[David Hildenbrand]

On 02.02.22 02:40, Mike Kravetz wrote:
> MADV_DONTNEED is currently disabled for hugetlb mappings.  This
> certainly makes sense in shared file mappings as the pagecache maintains
> a reference to the page and it will never be freed.  However, it could
> be useful to unmap and free pages in private mappings.
> 
> The only thing preventing MADV_DONTNEED from working on hugetlb mappings
> is a check in can_madv_lru_vma().  To allow support for hugetlb mappings
> create and use a new routine madvise_dontneed_free_valid_vma() that will
> allow hugetlb mappings.  Also, before calling zap_page_range in the
> DONTNEED case align start and size to huge page size for hugetlb vmas.
> madvise only requires PAGE_SIZE alignment, but the hugetlb unmap routine
> requires huge page size alignment.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  mm/madvise.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 5604064df464..7ae891e030a4 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -796,10 +796,30 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
>  static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
>  					unsigned long start, unsigned long end)
>  {
> +	/*
> +	 * start and size (end - start) must be huge page size aligned
> +	 * for hugetlb vmas.
> +	 */
> +	if (is_vm_hugetlb_page(vma)) {
> +		struct hstate *h = hstate_vma(vma);
> +
> +		start = ALIGN_DOWN(start, huge_page_size(h));
> +		end = ALIGN(end, huge_page_size(h));

So you effectively extend the range silently. IIUC, if someone would zap
a 4k range you would implicitly zap a whole 2M page and effectively zero
out more data than requested.


Looking at do_madvise(), we:
(1) reject start addresses that are not page-aligned
(2) shrink lengths that are not page-aligned and refuse if it turns 0

The man page documents (1) but doesn't really document (2).

Naturally I'd have assume that we apply the same logic to huge page
sizes and documenting it in the man page accordingly.


Why did you decide to extend the range? I'd assume MADV_REMOVE behaves
like FALLOC_FL_PUNCH_HOLE:
  "Within the specified range, partial filesystem blocks are zeroed, and
   whole filesystem blocks are removed from the file.  After a
   successful call, subsequent reads from  this  range will return
   zeros."
So we don't "discard more than requested".


I see the following possible alternatives:
(a) Fail if the range is not aligned
-> Clear semantics
(b) Fail if the start is not aligned, shrink the end if required
-> Same rules as for PAGE_SIZE
(c) Zero out the requested part
-> Same semantics as FALLOC_FL_PUNCH_HOLE.

My preference would be a), properly documenting it in the man page.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Mike Kravetz]

On 2/2/22 00:14, David Hildenbrand wrote:
> On 02.02.22 02:40, Mike Kravetz wrote:
>> MADV_DONTNEED is currently disabled for hugetlb mappings.  This
>> certainly makes sense in shared file mappings as the pagecache maintains
>> a reference to the page and it will never be freed.  However, it could
>> be useful to unmap and free pages in private mappings.
>>
>> The only thing preventing MADV_DONTNEED from working on hugetlb mappings
>> is a check in can_madv_lru_vma().  To allow support for hugetlb mappings
>> create and use a new routine madvise_dontneed_free_valid_vma() that will
>> allow hugetlb mappings.  Also, before calling zap_page_range in the
>> DONTNEED case align start and size to huge page size for hugetlb vmas.
>> madvise only requires PAGE_SIZE alignment, but the hugetlb unmap routine
>> requires huge page size alignment.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  mm/madvise.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 5604064df464..7ae891e030a4 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -796,10 +796,30 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
>>  static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
>>  					unsigned long start, unsigned long end)
>>  {
>> +	/*
>> +	 * start and size (end - start) must be huge page size aligned
>> +	 * for hugetlb vmas.
>> +	 */
>> +	if (is_vm_hugetlb_page(vma)) {
>> +		struct hstate *h = hstate_vma(vma);
>> +
>> +		start = ALIGN_DOWN(start, huge_page_size(h));
>> +		end = ALIGN(end, huge_page_size(h));
> 
> So you effectively extend the range silently. IIUC, if someone would zap
> a 4k range you would implicitly zap a whole 2M page and effectively zero
> out more data than requested.
> 
> 
> Looking at do_madvise(), we:
> (1) reject start addresses that are not page-aligned
> (2) shrink lengths that are not page-aligned and refuse if it turns 0

I believe length is extended (rounded up) by this line:
	len = PAGE_ALIGN(len_in);

but, I see your point.

> The man page documents (1) but doesn't really document (2).
> 
> Naturally I'd have assume that we apply the same logic to huge page
> sizes and documenting it in the man page accordingly.
> 
> 
> Why did you decide to extend the range? I'd assume MADV_REMOVE behaves
> like FALLOC_FL_PUNCH_HOLE:
>   "Within the specified range, partial filesystem blocks are zeroed, and
>    whole filesystem blocks are removed from the file.  After a
>    successful call, subsequent reads from  this  range will return
>    zeros."
> So we don't "discard more than requested".

Well.  hugetlbfs does not follow the man page. :(  It does not zero
partial blocks.  I assume a filesystem block would be a huge page.
Instead it does,

        /*
         * For hole punch round up the beginning offset of the hole and
         * round down the end.
         */
        hole_start = round_up(offset, hpage_size);
        hole_end = round_down(offset + len, hpage_size);

So, not only is this patch not following the man page.  It is not even
following the existing MADV_REMOVE hugetlb code.  Thanks for pointing
that out.  Part of my reason for adding this functionality was to make
hugetlb be more like 'normal' memory.  I clearly failed.

Related comment about madvise man page for PAGE_SIZE MADV_REMOVE.  The man
page says.

       MADV_REMOVE (since Linux 2.6.16)
              Free up a given range of pages and its associated backing store.
              This is equivalent to punching a hole in the corresponding  byte
              range  of  the backing store (see fallocate(2)).  Subsequent ac‐
              cesses in the specified address range will see bytes  containing
              zero.

This may need some clarification.  It says it will free pages.  We know
madvise only operates on pages (PAGE_ALIGN(len)).  Yet, the statement about
equivalent to a fallocate byte range may lead one to believe that length is
treated the same in madvise and fallocate.

> I see the following possible alternatives:
> (a) Fail if the range is not aligned
> -> Clear semantics
> (b) Fail if the start is not aligned, shrink the end if required
> -> Same rules as for PAGE_SIZE
> (c) Zero out the requested part
> -> Same semantics as FALLOC_FL_PUNCH_HOLE.
> 
> My preference would be a), properly documenting it in the man page.

However, a) would make hugetlb behave differently than other memory as
len does not need to be aligned.

I would prefer b) as it is more in line with PAGE_SIZE.  But, that does
make it different than MADV_REMOVE hugetlb alignment.

I thought this was simple. :)
-- 
Mike Kravetz

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[David Hildenbrand]

>>> +	/*
>>> +	 * start and size (end - start) must be huge page size aligned
>>> +	 * for hugetlb vmas.
>>> +	 */
>>> +	if (is_vm_hugetlb_page(vma)) {
>>> +		struct hstate *h = hstate_vma(vma);
>>> +
>>> +		start = ALIGN_DOWN(start, huge_page_size(h));
>>> +		end = ALIGN(end, huge_page_size(h));
>>
>> So you effectively extend the range silently. IIUC, if someone would zap
>> a 4k range you would implicitly zap a whole 2M page and effectively zero
>> out more data than requested.
>>
>>
>> Looking at do_madvise(), we:
>> (1) reject start addresses that are not page-aligned
>> (2) shrink lengths that are not page-aligned and refuse if it turns 0
> 
> I believe length is extended (rounded up) by this line:
> 	len = PAGE_ALIGN(len_in);

Ah, right. I was confused by the "!len" check below that, but the
comment explains how this applies to negative values only.

> 
> but, I see your point.
> 
>> The man page documents (1) but doesn't really document (2).
>>
>> Naturally I'd have assume that we apply the same logic to huge page
>> sizes and documenting it in the man page accordingly.
>>
>>
>> Why did you decide to extend the range? I'd assume MADV_REMOVE behaves
>> like FALLOC_FL_PUNCH_HOLE:
>>   "Within the specified range, partial filesystem blocks are zeroed, and
>>    whole filesystem blocks are removed from the file.  After a
>>    successful call, subsequent reads from  this  range will return
>>    zeros."
>> So we don't "discard more than requested".
> 
> Well.  hugetlbfs does not follow the man page. :(  It does not zero
> partial blocks.  I assume a filesystem block would be a huge page.
> Instead it does,
> 
>         /*
>          * For hole punch round up the beginning offset of the hole and
>          * round down the end.
>          */
>         hole_start = round_up(offset, hpage_size);
>         hole_end = round_down(offset + len, hpage_size);

Okay, so we skip any zeroing and only free completely covered blocks. We
might want to document that behavior. See below.

> 
> So, not only is this patch not following the man page.  It is not even
> following the existing MADV_REMOVE hugetlb code.  Thanks for pointing
> that out.  Part of my reason for adding this functionality was to make
> hugetlb be more like 'normal' memory.  I clearly failed.

:)

> 
> Related comment about madvise man page for PAGE_SIZE MADV_REMOVE.  The man
> page says.
> 
>        MADV_REMOVE (since Linux 2.6.16)
>               Free up a given range of pages and its associated backing store.
>               This is equivalent to punching a hole in the corresponding  byte
>               range  of  the backing store (see fallocate(2)).  Subsequent ac‐
>               cesses in the specified address range will see bytes  containing
>               zero.
> 
> This may need some clarification.  It says it will free pages.  We know
> madvise only operates on pages (PAGE_ALIGN(len)).  Yet, the statement about
> equivalent to a fallocate byte range may lead one to believe that length is
> treated the same in madvise and fallocate.

Yes

> 
>> I see the following possible alternatives:
>> (a) Fail if the range is not aligned
>> -> Clear semantics
>> (b) Fail if the start is not aligned, shrink the end if required
>> -> Same rules as for PAGE_SIZE
>> (c) Zero out the requested part
>> -> Same semantics as FALLOC_FL_PUNCH_HOLE.
>>
>> My preference would be a), properly documenting it in the man page.
> 
> However, a) would make hugetlb behave differently than other memory as
> len does not need to be aligned.
> 
> I would prefer b) as it is more in line with PAGE_SIZE.  But, that does
> make it different than MADV_REMOVE hugetlb alignment.
> 
> I thought this was simple. :)

It really bugs me that it's under-specified what's supposed to happen
when the length is not aligned.

BUT: in the posix world, "calling posix_madvise() shall not affect the
semantics of access to memory in the specified range". So we don't care
too much about if we align up/down, because it wouldn't affect the
semantics. Especially for MADV_DONTNEED/MADV_REMOVE as implemented by
Linux this is certainly different and the alignment handling matters.

So I guess especially for MADV_DONTNEED/MADV_REMOVE we need a clear
specification what's supposed to happen if the length falls into the
middle of a huge page. We should document alignment handling for
madvise() in general I assume.

IMHO we should have bailed out right from the start whenever something
is not properly aligned, but that ship has sailed. So I agree, maybe we
can make at least hugetlb MADV_DONTNEED obey the same (weird) rules as
ordinary pages.

So b) would mean, requiring start to be hugepage aligned and aligning-up
the end. Still feels wrong but at least matches existing semantics.

Hugetlb MADV_REMOVE semantics are unfortunate and we should document the
exception.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Mike Kravetz]

On 2/4/22 00:35, David Hildenbrand wrote:
>> I thought this was simple. :)
> 
> It really bugs me that it's under-specified what's supposed to happen
> when the length is not aligned.
> 
> BUT: in the posix world, "calling posix_madvise() shall not affect the
> semantics of access to memory in the specified range". So we don't care
> too much about if we align up/down, because it wouldn't affect the
> semantics. Especially for MADV_DONTNEED/MADV_REMOVE as implemented by
> Linux this is certainly different and the alignment handling matters.
> 
> So I guess especially for MADV_DONTNEED/MADV_REMOVE we need a clear
> specification what's supposed to happen if the length falls into the
> middle of a huge page. We should document alignment handling for
> madvise() in general I assume.
> 
> IMHO we should have bailed out right from the start whenever something
> is not properly aligned, but that ship has sailed. So I agree, maybe we
> can make at least hugetlb MADV_DONTNEED obey the same (weird) rules as
> ordinary pages.
> 
> So b) would mean, requiring start to be hugepage aligned and aligning-up
> the end. Still feels wrong but at least matches existing semantics.
> 
> Hugetlb MADV_REMOVE semantics are unfortunate and we should document the
> exception.

Thank you for all your comments David!

So, my plan was to make MADV_DONTNEED behave as described above:
- EINVAL if start address not huge page size aligned
- Align end/length up to huge page size.

The code I had for this was very specific to MADV_DONTNEED.  I then thought,
why not do the same for MADV_REMOVE as well?  Or even more general, add this
check and alignment to the vma parsing code in madvise.

It was then that I realized there are several madvise behaviors that take
non-huge page size aligned addresses for hugetlb mappings today.  Making
huge page size alignment a requirement for all madvise behaviors could break
existing code.  So, it seems like it could only be added to MADV_DONTNEED as
this functionality does not exist today.  We then end up with MADV_DONTNEED
as the only behavior requiring huge page size alignment for hugetlb mappings.
Sigh!!!

I am now rethinking the decision to proceed with b) as described above.

With the exception of MADV_REMOVE (which we may be able to change for
hugetlb),  madvise operations operate on huge page size pages for hugetlb
mappings.  If start address is in the middle of a hugetlb page, we essentially
align down to the beginning of the hugetlb page.  If length lands in the
middle of a hugetlb page, we essentially round up.

When adding MADV_REMOVE perhaps we should go with this align down start and
align up end strategy that is used everywhere else?  I really wish we could
go back and change things, but as you know it is too late for that.
-- 
Mike Kravetz

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Peter Xu]

(Sorry for the late comment)

On Tue, Feb 01, 2022 at 05:40:32PM -0800, Mike Kravetz wrote:
> MADV_DONTNEED is currently disabled for hugetlb mappings.  This
> certainly makes sense in shared file mappings as the pagecache maintains
> a reference to the page and it will never be freed.  However, it could
> be useful to unmap and free pages in private mappings.
> 
> The only thing preventing MADV_DONTNEED from working on hugetlb mappings
> is a check in can_madv_lru_vma().  To allow support for hugetlb mappings
> create and use a new routine madvise_dontneed_free_valid_vma() that will
> allow hugetlb mappings.  Also, before calling zap_page_range in the
> DONTNEED case align start and size to huge page size for hugetlb vmas.
> madvise only requires PAGE_SIZE alignment, but the hugetlb unmap routine
> requires huge page size alignment.
> 
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  mm/madvise.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 5604064df464..7ae891e030a4 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -796,10 +796,30 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
>  static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
>  					unsigned long start, unsigned long end)
>  {
> +	/*
> +	 * start and size (end - start) must be huge page size aligned
> +	 * for hugetlb vmas.
> +	 */
> +	if (is_vm_hugetlb_page(vma)) {
> +		struct hstate *h = hstate_vma(vma);
> +
> +		start = ALIGN_DOWN(start, huge_page_size(h));
> +		end = ALIGN(end, huge_page_size(h));
> +	}
> +

Maybe check the alignment before userfaultfd_remove()?  Otherwise there'll be a
fake message generated to the tracer.

>  	zap_page_range(vma, start, end - start);
>  	return 0;
>  }
>  
> +static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
> +						int behavior)
> +{
> +	if (is_vm_hugetlb_page(vma))
> +		return behavior == MADV_DONTNEED;
> +	else
> +		return can_madv_lru_vma(vma);
> +}

can_madv_lru_vma() will check hugetlb again which looks a bit weird.  Would it
look better to write it as:

madvise_dontneed_free_valid_vma()
{
    return !(vma->vm_flags & (VM_LOCKED|VM_PFNMAP));
}

can_madv_lru_vma()
{
    return madvise_dontneed_free_valid_vma() && !is_vm_hugetlb_page(vma);
}

?

Another use case of DONTNEED upon hugetlbfs could be uffd-minor, because afaiu
this is the only api that can force strip the hugetlb mapped pgtable without
losing pagecache data.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[David Hildenbrand]

On 08.02.22 00:47, Mike Kravetz wrote:
> On 2/4/22 00:35, David Hildenbrand wrote:
>>> I thought this was simple. :)
>>
>> It really bugs me that it's under-specified what's supposed to happen
>> when the length is not aligned.
>>
>> BUT: in the posix world, "calling posix_madvise() shall not affect the
>> semantics of access to memory in the specified range". So we don't care
>> too much about if we align up/down, because it wouldn't affect the
>> semantics. Especially for MADV_DONTNEED/MADV_REMOVE as implemented by
>> Linux this is certainly different and the alignment handling matters.
>>
>> So I guess especially for MADV_DONTNEED/MADV_REMOVE we need a clear
>> specification what's supposed to happen if the length falls into the
>> middle of a huge page. We should document alignment handling for
>> madvise() in general I assume.
>>
>> IMHO we should have bailed out right from the start whenever something
>> is not properly aligned, but that ship has sailed. So I agree, maybe we
>> can make at least hugetlb MADV_DONTNEED obey the same (weird) rules as
>> ordinary pages.
>>
>> So b) would mean, requiring start to be hugepage aligned and aligning-up
>> the end. Still feels wrong but at least matches existing semantics.
>>
>> Hugetlb MADV_REMOVE semantics are unfortunate and we should document the
>> exception.
> 
> Thank you for all your comments David!
> 
> So, my plan was to make MADV_DONTNEED behave as described above:
> - EINVAL if start address not huge page size aligned
> - Align end/length up to huge page size.
> 
> The code I had for this was very specific to MADV_DONTNEED.  I then thought,
> why not do the same for MADV_REMOVE as well?  Or even more general, add this
> check and alignment to the vma parsing code in madvise.
> 
> It was then that I realized there are several madvise behaviors that take
> non-huge page size aligned addresses for hugetlb mappings today.  Making
> huge page size alignment a requirement for all madvise behaviors could break
> existing code.  So, it seems like it could only be added to MADV_DONTNEED as
> this functionality does not exist today.  We then end up with MADV_DONTNEED
> as the only behavior requiring huge page size alignment for hugetlb mappings.
> Sigh!!!

:/

> 
> I am now rethinking the decision to proceed with b) as described above.
> 
> With the exception of MADV_REMOVE (which we may be able to change for
> hugetlb),  madvise operations operate on huge page size pages for hugetlb
> mappings.  If start address is in the middle of a hugetlb page, we essentially
> align down to the beginning of the hugetlb page.  If length lands in the
> middle of a hugetlb page, we essentially round up.

Which MADV calls would be affected?

The "bad" thing about MADV_DONTNEED and MADV_REMOVE are that they
destroy data, which is why they heavily diverge from the original posix
madvise odea.

> 
> When adding MADV_REMOVE perhaps we should go with this align down start and
> align up end strategy that is used everywhere else?  I really wish we could
> go back and change things, but as you know it is too late for that.

I assume whatever we do, we should document it at least cleanly in the
man page. Unfortunately, hugetlb is a gift that keeps on giving. Making
it at least somehow consistent, even if it's "hugtlb being consistent in
its own mess", that would be preferable I guess.

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Mike Kravetz]

On 2/9/22 19:21, Peter Xu wrote:
> (Sorry for the late comment)

Thanks for taking a look.

> 
> On Tue, Feb 01, 2022 at 05:40:32PM -0800, Mike Kravetz wrote:
>> MADV_DONTNEED is currently disabled for hugetlb mappings.  This
>> certainly makes sense in shared file mappings as the pagecache maintains
>> a reference to the page and it will never be freed.  However, it could
>> be useful to unmap and free pages in private mappings.
>>
>> The only thing preventing MADV_DONTNEED from working on hugetlb mappings
>> is a check in can_madv_lru_vma().  To allow support for hugetlb mappings
>> create and use a new routine madvise_dontneed_free_valid_vma() that will
>> allow hugetlb mappings.  Also, before calling zap_page_range in the
>> DONTNEED case align start and size to huge page size for hugetlb vmas.
>> madvise only requires PAGE_SIZE alignment, but the hugetlb unmap routine
>> requires huge page size alignment.
>>
>> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  mm/madvise.c | 24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 5604064df464..7ae891e030a4 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -796,10 +796,30 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
>>  static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
>>  					unsigned long start, unsigned long end)
>>  {
>> +	/*
>> +	 * start and size (end - start) must be huge page size aligned
>> +	 * for hugetlb vmas.
>> +	 */
>> +	if (is_vm_hugetlb_page(vma)) {
>> +		struct hstate *h = hstate_vma(vma);
>> +
>> +		start = ALIGN_DOWN(start, huge_page_size(h));
>> +		end = ALIGN(end, huge_page_size(h));
>> +	}
>> +
> 
> Maybe check the alignment before userfaultfd_remove()?  Otherwise there'll be a
> fake message generated to the tracer.

Yes, we should pass the aligned addresses to userfaultfd_remove.  We will
also need to potentially align again after the call.

> 
>>  	zap_page_range(vma, start, end - start);
>>  	return 0;
>>  }
>>  
>> +static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma,
>> +						int behavior)
>> +{
>> +	if (is_vm_hugetlb_page(vma))
>> +		return behavior == MADV_DONTNEED;
>> +	else
>> +		return can_madv_lru_vma(vma);
>> +}
> 
> can_madv_lru_vma() will check hugetlb again which looks a bit weird.  Would it
> look better to write it as:
> 
> madvise_dontneed_free_valid_vma()
> {
>     return !(vma->vm_flags & (VM_LOCKED|VM_PFNMAP));
> }
> 
> can_madv_lru_vma()
> {
>     return madvise_dontneed_free_valid_vma() && !is_vm_hugetlb_page(vma);
> }
> 
> ?

Yes, that would look better.

> 
> Another use case of DONTNEED upon hugetlbfs could be uffd-minor, because afaiu
> this is the only api that can force strip the hugetlb mapped pgtable without
> losing pagecache data.
> 

Correct.  However, I do not know if uffd-minor users would ever want to
do this.  Perhaps?
-- 
Mike Kravetz

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Mike Kravetz]

On 2/10/22 05:09, David Hildenbrand wrote:
> On 08.02.22 00:47, Mike Kravetz wrote:
>> On 2/4/22 00:35, David Hildenbrand wrote:
>>>> I thought this was simple. :)
>>>
>>> It really bugs me that it's under-specified what's supposed to happen
>>> when the length is not aligned.
>>>
>>> BUT: in the posix world, "calling posix_madvise() shall not affect the
>>> semantics of access to memory in the specified range". So we don't care
>>> too much about if we align up/down, because it wouldn't affect the
>>> semantics. Especially for MADV_DONTNEED/MADV_REMOVE as implemented by
>>> Linux this is certainly different and the alignment handling matters.
>>>
>>> So I guess especially for MADV_DONTNEED/MADV_REMOVE we need a clear
>>> specification what's supposed to happen if the length falls into the
>>> middle of a huge page. We should document alignment handling for
>>> madvise() in general I assume.
>>>
>>> IMHO we should have bailed out right from the start whenever something
>>> is not properly aligned, but that ship has sailed. So I agree, maybe we
>>> can make at least hugetlb MADV_DONTNEED obey the same (weird) rules as
>>> ordinary pages.
>>>
>>> So b) would mean, requiring start to be hugepage aligned and aligning-up
>>> the end. Still feels wrong but at least matches existing semantics.
>>>
>>> Hugetlb MADV_REMOVE semantics are unfortunate and we should document the
>>> exception.
>>
>> Thank you for all your comments David!
>>
>> So, my plan was to make MADV_DONTNEED behave as described above:
>> - EINVAL if start address not huge page size aligned
>> - Align end/length up to huge page size.
>>
>> The code I had for this was very specific to MADV_DONTNEED.  I then thought,
>> why not do the same for MADV_REMOVE as well?  Or even more general, add this
>> check and alignment to the vma parsing code in madvise.
>>
>> It was then that I realized there are several madvise behaviors that take
>> non-huge page size aligned addresses for hugetlb mappings today.  Making
>> huge page size alignment a requirement for all madvise behaviors could break
>> existing code.  So, it seems like it could only be added to MADV_DONTNEED as
>> this functionality does not exist today.  We then end up with MADV_DONTNEED
>> as the only behavior requiring huge page size alignment for hugetlb mappings.
>> Sigh!!!
> 
> :/
> 
>>
>> I am now rethinking the decision to proceed with b) as described above.
>>
>> With the exception of MADV_REMOVE (which we may be able to change for
>> hugetlb),  madvise operations operate on huge page size pages for hugetlb
>> mappings.  If start address is in the middle of a hugetlb page, we essentially
>> align down to the beginning of the hugetlb page.  If length lands in the
>> middle of a hugetlb page, we essentially round up.
> 
> Which MADV calls would be affected?

Not sure I understand the question.  I was saying that madvise calls which
operate on hugetlb mappings today only operate on huge pages.  So, this is
essentially align down starting address and align up end address.
For example consider the MADV_POPULATE calls you recently added.  They will
only fault in huge pages in a hugetlb vma.

> The "bad" thing about MADV_DONTNEED and MADV_REMOVE are that they
> destroy data, which is why they heavily diverge from the original posix
> madvise odea.

Hmmm.  That may be a good argument for strict alignment.  We do not want
to remove (or unmap) more than the user intended.  The unmap system call
has such alignment requirements.

Darn!  I'm thinking that I should go back to requiring alignment for
MADV_DONTNEED.

There really is no 'right' answer.

>>
>> When adding MADV_REMOVE perhaps we should go with this align down start and
>> align up end strategy that is used everywhere else?  I really wish we could
>> go back and change things, but as you know it is too late for that.
> 
> I assume whatever we do, we should document it at least cleanly in the
> man page. Unfortunately, hugetlb is a gift that keeps on giving. Making
> it at least somehow consistent, even if it's "hugtlb being consistent in
> its own mess", that would be preferable I guess.

Yes, more than anything we need to document behavior.
-- 
Mike Kravetz

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Peter Xu]

On Thu, Feb 10, 2022 at 01:36:57PM -0800, Mike Kravetz wrote:
> > Another use case of DONTNEED upon hugetlbfs could be uffd-minor, because afaiu
> > this is the only api that can force strip the hugetlb mapped pgtable without
> > losing pagecache data.
> 
> Correct.  However, I do not know if uffd-minor users would ever want to
> do this.  Perhaps?

My understanding is before this patch uffd-minor upon hugetlbfs requires the
huge file to be mapped twice, one to populate the content, then we'll be able
to trap MINOR faults via the other mapping.  Or we could munmap() the range and
remap it again on the same file offset to drop the pgtables, I think. But that
sounds tricky.  MINOR faults only works with pgtables dropped.

With DONTNEED upon hugetlbfs we can rely on one single mapping of the file,
because we can explicitly drop the pgtables of hugetlbfs files without any
other tricks.

However I have no real use case of it.  Initially I thought it could be useful
for QEMU because QEMU migration routine is run with the same mm context with
the hypervisor, so by default is doesn't have two mappings of the same guest
memory.  If QEMU wants to leverage minor faults, DONTNEED could help.

However when I was measuring bitmap transfer (assuming that's what minor fault
could help with qemu's postcopy) there some months ago I found it's not as slow
as I thought at all..  Either I could have missed something, or we're facing
different problems with what it is when uffd minor is firstly proposed by Axel.

This is probably too out of topic, though..  Let me go back..

Said that, one thing I'm not sure about DONTNEED on hugetlb is whether this
could further abuse DONTNEED, as the original POSIX definition is as simple as:

  The application expects that it will not access the specified address range
  in the near future.

Linux did it by tearing down pgtable, which looks okay so far.  It could be a
bit more weird to apply it to hugetlbfs because from its definition it's a hint
to page reclaims, however hugetlbfs is not a target of page reclaim, neither is
it LRU-aware.  It goes further into some MADV_ZAP styled syscall.

I think it could still be fine as posix doesn't define that behavior
specifically on hugetlb so it can be defined by Linux, but not sure whether
there can be other implications.

Thanks,

-- 
Peter Xu

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[David Hildenbrand]

>>>
>>> I am now rethinking the decision to proceed with b) as described above.
>>>
>>> With the exception of MADV_REMOVE (which we may be able to change for
>>> hugetlb),  madvise operations operate on huge page size pages for hugetlb
>>> mappings.  If start address is in the middle of a hugetlb page, we essentially
>>> align down to the beginning of the hugetlb page.  If length lands in the
>>> middle of a hugetlb page, we essentially round up.
>>
>> Which MADV calls would be affected?
> 
> Not sure I understand the question.  I was saying that madvise calls which
> operate on hugetlb mappings today only operate on huge pages.  So, this is
> essentially align down starting address and align up end address.

Let me clarify:

If you accidentially
MADV_NORMAL/MADV_RANDOM/MADV_SEQUENTIAL/MADV_WILLNEED a range that's
slightly bigger/smaller than the requested one you don't actually care,
because it will only slightly affect the performance of an application,
if at all.  MADV_COLD/MADV_PAGEOUT should be similar. I assume these
don't apply to hugetlb at all.

The effects of
MADV_MERGEABLE/MADV_UNMERGEABLE/MADV_HUGEPAGE/MADV_NOHUGEPAGE should in
theory be similar, however, there can be some user-space visible effects
when you get it wrong. I assume these don't apply to hugetlb at all.

However, for
MADV_DONTNEED/MADV_REMOVE/MADV_DONTFORK/MADV_DOFORK/MADV_FREE/MADV_WIPEONFORK/MADV_KEEPONFORK/MADV_DONTDUMP/MADV_DODUMP/....
the application could easily detect the difference of the actual range
handling.


> For example consider the MADV_POPULATE calls you recently added.  They will
> only fault in huge pages in a hugetlb vma.

On a related note: I don't see my man page updates upstream yet. And the
last update upstream seems to have happened 5 months ago ... not sure
why the man project seems to have stalled.

https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Axel Rasmussen]

On Thu, Feb 10, 2022 at 6:29 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Feb 10, 2022 at 01:36:57PM -0800, Mike Kravetz wrote:
> > > Another use case of DONTNEED upon hugetlbfs could be uffd-minor, because afaiu
> > > this is the only api that can force strip the hugetlb mapped pgtable without
> > > losing pagecache data.
> >
> > Correct.  However, I do not know if uffd-minor users would ever want to
> > do this.  Perhaps?

I talked with some colleagues, and I didn't come up with any
production *requirement* for it, but it may be a convenience in some
cases (make certain code cleaner, e.g. not having to unmap-and-remap
to tear down page tables as Peter mentioned). I think Peter's
assessment below is right.

>
> My understanding is before this patch uffd-minor upon hugetlbfs requires the
> huge file to be mapped twice, one to populate the content, then we'll be able
> to trap MINOR faults via the other mapping.  Or we could munmap() the range and
> remap it again on the same file offset to drop the pgtables, I think. But that
> sounds tricky.  MINOR faults only works with pgtables dropped.
>
> With DONTNEED upon hugetlbfs we can rely on one single mapping of the file,
> because we can explicitly drop the pgtables of hugetlbfs files without any
> other tricks.
>
> However I have no real use case of it.  Initially I thought it could be useful
> for QEMU because QEMU migration routine is run with the same mm context with
> the hypervisor, so by default is doesn't have two mappings of the same guest
> memory.  If QEMU wants to leverage minor faults, DONTNEED could help.).
>
> However when I was measuring bitmap transfer (assuming that's what minor fault
> could help with qemu's postcopy) there some months ago I found it's not as slow
> as I thought at all..  Either I could have missed something, or we're facing
> different problems with what it is when uffd minor is firstly proposed by Axel.

Re: the bitmap, that matters most on machines with lots of RAM. For
example, GCE offers some VMs with up to 12 *TB* of RAM
(https://cloud.google.com/compute/docs/memory-optimized-machines), I
think with this size of machine we see a significant benefit, as it
may take some significant time for the bitmap to arrive over the
network.

But I think that's a bit of an edge case, most machines are not that
big. :) I think the benefit is more often seen just in avoiding
copies. E.g. if we find a page is already up-to-date after precopy, we
just install PTEs, no copying or page allocation needed. And even when
we have to go fetch a page over the network, one can imagine an RDMA
setup where we can avoid any copies/allocations at all even in that
case. I suppose this also has a bigger effect on larger machines, e.g.
ones that are backed by 1G pages instead of 4k.

>
> This is probably too out of topic, though..  Let me go back..
>
> Said that, one thing I'm not sure about DONTNEED on hugetlb is whether this
> could further abuse DONTNEED, as the original POSIX definition is as simple as:
>
>   The application expects that it will not access the specified address range
>   in the near future.
>
> Linux did it by tearing down pgtable, which looks okay so far.  It could be a
> bit more weird to apply it to hugetlbfs because from its definition it's a hint
> to page reclaims, however hugetlbfs is not a target of page reclaim, neither is
> it LRU-aware.  It goes further into some MADV_ZAP styled syscall.
>
> I think it could still be fine as posix doesn't define that behavior
> specifically on hugetlb so it can be defined by Linux, but not sure whether
> there can be other implications.
>
> Thanks,
>
> --
> Peter Xu
>

Re: [PATCH v2 1/3] mm: enable MADV_DONTNEED for hugetlb mappings

[Mike Kravetz]

On 2/11/22 11:08, Axel Rasmussen wrote:
> On Thu, Feb 10, 2022 at 6:29 PM Peter Xu <peterx@redhat.com> wrote:
>>
>> On Thu, Feb 10, 2022 at 01:36:57PM -0800, Mike Kravetz wrote:
>>>> Another use case of DONTNEED upon hugetlbfs could be uffd-minor, because afaiu
>>>> this is the only api that can force strip the hugetlb mapped pgtable without
>>>> losing pagecache data.
>>>
>>> Correct.  However, I do not know if uffd-minor users would ever want to
>>> do this.  Perhaps?
> 
> I talked with some colleagues, and I didn't come up with any
> production *requirement* for it, but it may be a convenience in some
> cases (make certain code cleaner, e.g. not having to unmap-and-remap
> to tear down page tables as Peter mentioned). I think Peter's
> assessment below is right.
> 
>>
>> My understanding is before this patch uffd-minor upon hugetlbfs requires the
>> huge file to be mapped twice, one to populate the content, then we'll be able
>> to trap MINOR faults via the other mapping.  Or we could munmap() the range and
>> remap it again on the same file offset to drop the pgtables, I think. But that
>> sounds tricky.  MINOR faults only works with pgtables dropped.
>>
>> With DONTNEED upon hugetlbfs we can rely on one single mapping of the file,
>> because we can explicitly drop the pgtables of hugetlbfs files without any
>> other tricks.
>>
>> However I have no real use case of it.  Initially I thought it could be useful
>> for QEMU because QEMU migration routine is run with the same mm context with
>> the hypervisor, so by default is doesn't have two mappings of the same guest
>> memory.  If QEMU wants to leverage minor faults, DONTNEED could help.).
>>
>> However when I was measuring bitmap transfer (assuming that's what minor fault
>> could help with qemu's postcopy) there some months ago I found it's not as slow
>> as I thought at all..  Either I could have missed something, or we're facing
>> different problems with what it is when uffd minor is firstly proposed by Axel.
> 
> Re: the bitmap, that matters most on machines with lots of RAM. For
> example, GCE offers some VMs with up to 12 *TB* of RAM
> (https://cloud.google.com/compute/docs/memory-optimized-machines), I
> think with this size of machine we see a significant benefit, as it
> may take some significant time for the bitmap to arrive over the
> network.
> 
> But I think that's a bit of an edge case, most machines are not that
> big. :) I think the benefit is more often seen just in avoiding
> copies. E.g. if we find a page is already up-to-date after precopy, we
> just install PTEs, no copying or page allocation needed. And even when
> we have to go fetch a page over the network, one can imagine an RDMA
> setup where we can avoid any copies/allocations at all even in that
> case. I suppose this also has a bigger effect on larger machines, e.g.
> ones that are backed by 1G pages instead of 4k.
> 

Thanks Peter and Axel!

As mentioned, my primary motivation was to simply clean up the userfaultfd
selftest.  Glad to see there might be more use cases.  If we can simplify
other code as in the case of userfaultfd selftest, that would be a win.

-- 
Mike Kravetz