Skip to contentSkip to conversation

HMM tests and minor fixes

PATCH Superseded Ralph Campbell 2 participants 9 messages Sep 11, 2019 – Sep 12, 2019

Subsystems (4)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

cover HMM tests and minor fixes

These changes are based on Jason's latest hmm branch.
Patch 1 was previously posted here [1] but was dropped from the orginal series. Hopefully, the tests will reduce concerns about edge conditions. I'm sure more tests could be usefully added but I thought this was a good starting point.

[1] https://lore.kernel.org/linux-mm/[email protected]/

Ralph Campbell (4):
mm/hmm: make full use of walk_page_range() mm/hmm: allow snapshot of the special zero page mm/hmm: allow hmm_range_fault() of mmap(PROT_NONE) mm/hmm/test: add self tests for HMM

 MAINTAINERS                            |    3 +
 drivers/char/Kconfig                   |   11 +
 drivers/char/Makefile                  |    1 +
 drivers/char/hmm_dmirror.c             | 1504 ++++++++++++++++++++++++
 include/Kbuild                         |    1 +
 include/uapi/linux/hmm_dmirror.h       |   74 ++
 mm/hmm.c                               |  117 +-
 tools/testing/selftests/vm/.gitignore  |    1 +
 tools/testing/selftests/vm/Makefile    |    3 +
 tools/testing/selftests/vm/config      |    3 +
 tools/testing/selftests/vm/hmm-tests.c | 1304 ++++++++++++++++++++
 tools/testing/selftests/vm/run_vmtests |   16 +
 tools/testing/selftests/vm/test_hmm.sh |  105 ++
 13 files changed, 3079 insertions(+), 64 deletions(-)
 create mode 100644 drivers/char/hmm_dmirror.c
 create mode 100644 include/uapi/linux/hmm_dmirror.h
 create mode 100644 tools/testing/selftests/vm/hmm-tests.c
 create mode 100755 tools/testing/selftests/vm/test_hmm.sh
Signature · 1 line
2.20.1
#1 lore raw

PATCH 1/4 mm/hmm: make full use of walk_page_range()

hmm_range_fault() calls find_vma() and walk_page_range() in a loop. This is unnecessary duplication since walk_page_range() calls find_vma() in a loop already.
Simplify hmm_range_fault() by defining a walk_test() callback function to filter unhandled vmas.
This also fixes a bug where hmm_range_fault() was not checking start >= vma->vm_start before checking vma->vm_flags so hmm_range_fault() could return an error based on the wrong vma for the requested range.

Cc "Jérôme Glisse" <[email protected]>
Cc Jason Gunthorpe <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 mm/hmm.c | 113 ++++++++++++++++++++++++-------------------------------
 1 file changed, 50 insertions(+), 63 deletions(-)
mm/hmm.c +50 -63
index 902f5fa6bf93..06041d4399ff 100644
@@ -252,18 +252,17 @@ static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr,
 	return -EFAULT;
 }
 
-static int hmm_pfns_bad(unsigned long addr,
-			unsigned long end,
-			struct mm_walk *walk)
+static int hmm_pfns_fill(unsigned long addr,
+			 unsigned long end,
+			 struct hmm_range *range,
+			 enum hmm_pfn_value_e value)
 {
-	struct hmm_vma_walk *hmm_vma_walk = walk->private;
-	struct hmm_range *range = hmm_vma_walk->range;
 	uint64_t *pfns = range->pfns;
 	unsigned long i;
 
 	i = (addr - range->start) >> PAGE_SHIFT;
 	for (; addr < end; addr += PAGE_SIZE, i++)
-		pfns[i] = range->values[HMM_PFN_ERROR];
+		pfns[i] = range->values[value];
 
 	return 0;
 }
@@ -584,7 +583,7 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
 		}
 		return 0;
 	} else if (!pmd_present(pmd))
-		return hmm_pfns_bad(start, end, walk);
+		return hmm_pfns_fill(start, end, range, HMM_PFN_ERROR);
 
 	if (pmd_devmap(pmd) || pmd_trans_huge(pmd)) {
 		/*
@@ -612,7 +611,7 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
 	 * recover.
 	 */
 	if (pmd_bad(pmd))
-		return hmm_pfns_bad(start, end, walk);
+		return hmm_pfns_fill(start, end, range, HMM_PFN_ERROR);
 
 	ptep = pte_offset_map(pmdp, addr);
 	i = (addr - range->start) >> PAGE_SHIFT;
@@ -770,13 +769,36 @@ static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask,
 #define hmm_vma_walk_hugetlb_entry NULL
 #endif /* CONFIG_HUGETLB_PAGE */
 
-static void hmm_pfns_clear(struct hmm_range *range,
-			   uint64_t *pfns,
-			   unsigned long addr,
-			   unsigned long end)
+static int hmm_vma_walk_test(unsigned long start,
+			     unsigned long end,
+			     struct mm_walk *walk)
 {
-	for (; addr < end; addr += PAGE_SIZE, pfns++)
-		*pfns = range->values[HMM_PFN_NONE];
+	struct hmm_vma_walk *hmm_vma_walk = walk->private;
+	struct hmm_range *range = hmm_vma_walk->range;
+	struct vm_area_struct *vma = walk->vma;
+
+	/* If range is no longer valid, force retry. */
+	if (!range->valid)
+		return -EBUSY;
+
+	/*
+	 * Skip vma ranges that don't have struct page backing them or
+	 * map I/O devices directly.
+	 */
+	if (vma->vm_flags & (VM_IO | VM_PFNMAP | VM_MIXEDMAP))
+		return -EFAULT;
+
+	/*
+	 * If the vma does not allow read access, then assume that it does not
+	 * allow write access either. HMM does not support architectures
+	 * that allow write without read.
+	 */
+	if (!(vma->vm_flags & VM_READ)) {
+		(void) hmm_pfns_fill(start, end, range, HMM_PFN_NONE);
+		return -EPERM;
+	}
+
+	return 0;
 }
 
 /*
@@ -857,6 +879,7 @@ static const struct mm_walk_ops hmm_walk_ops = {
 	.pmd_entry	= hmm_vma_walk_pmd,
 	.pte_hole	= hmm_vma_walk_hole,
 	.hugetlb_entry	= hmm_vma_walk_hugetlb_entry,
+	.test_walk	= hmm_vma_walk_test,
 };
 
 /**
@@ -889,63 +912,27 @@ static const struct mm_walk_ops hmm_walk_ops = {
  */
 long hmm_range_fault(struct hmm_range *range, unsigned int flags)
 {
-	const unsigned long device_vma = VM_IO | VM_PFNMAP | VM_MIXEDMAP;
-	unsigned long start = range->start, end;
-	struct hmm_vma_walk hmm_vma_walk;
+	unsigned long start = range->start;
+	struct hmm_vma_walk hmm_vma_walk = {
+		.range = range,
+		.last = start,
+		.flags = flags,
+	};
 	struct hmm *hmm = range->hmm;
-	struct vm_area_struct *vma;
 	int ret;
 
 	lockdep_assert_held(&hmm->mmu_notifier.mm->mmap_sem);
 
 	do {
-		/* If range is no longer valid force retry. */
-		if (!range->valid)
-			return -EBUSY;
-
-		vma = find_vma(hmm->mmu_notifier.mm, start);
-		if (vma == NULL || (vma->vm_flags & device_vma))
-			return -EFAULT;
-
-		if (!(vma->vm_flags & VM_READ)) {
-			/*
-			 * If vma do not allow read access, then assume that it
-			 * does not allow write access, either. HMM does not
-			 * support architecture that allow write without read.
-			 */
-			hmm_pfns_clear(range, range->pfns,
-				range->start, range->end);
-			return -EPERM;
-		}
+		ret = walk_page_range(hmm->mmu_notifier.mm, start, range->end,
+				      &hmm_walk_ops, &hmm_vma_walk);
+		start = hmm_vma_walk.last;
 
-		hmm_vma_walk.pgmap = NULL;
-		hmm_vma_walk.last = start;
-		hmm_vma_walk.flags = flags;
-		hmm_vma_walk.range = range;
-		end = min(range->end, vma->vm_end);
+		/* Keep trying while the range is valid. */
+	} while (ret == -EBUSY && range->valid);
 
-		walk_page_range(vma->vm_mm, start, end, &hmm_walk_ops,
-				&hmm_vma_walk);
-
-		do {
-			ret = walk_page_range(vma->vm_mm, start, end,
-					&hmm_walk_ops, &hmm_vma_walk);
-			start = hmm_vma_walk.last;
-
-			/* Keep trying while the range is valid. */
-		} while (ret == -EBUSY && range->valid);
-
-		if (ret) {
-			unsigned long i;
-
-			i = (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
-			hmm_pfns_clear(range, &range->pfns[i],
-				hmm_vma_walk.last, range->end);
-			return ret;
-		}
-		start = end;
-
-	} while (start < range->end);
+	if (ret)
+		return ret;
 
 	return (hmm_vma_walk.last - range->start) >> PAGE_SHIFT;
 }
Signature · 1 line
2.20.1
#2 lore raw

Re: [PATCH 1/4] mm/hmm: make full use of walk_page_range()

mm/hmm.c static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, · line 258

Quoted 4 lines
mm/hmm.c +4 -0
+static int hmm_pfns_fill(unsigned long addr,
+			 unsigned long end,
+			 struct hmm_range *range,
+			 enum hmm_pfn_value_e value)

Nit: can we use the space a little more efficient, e.g.:

static int hmm_pfns_fill(unsigned long addr, unsigned long end,
struct hmm_range *range, enum hmm_pfn_value_e value)

mm/hmm.c static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask, · line 774

Quoted 3 lines
mm/hmm.c +3 -0
+static int hmm_vma_walk_test(unsigned long start,
+			     unsigned long end,
+			     struct mm_walk *walk)

Same here.

mm/hmm.c static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask, · line 797

Quoted 2 lines
mm/hmm.c +2 -0
+	if (!(vma->vm_flags & VM_READ)) {
+		(void) hmm_pfns_fill(start, end, range, HMM_PFN_NONE);

There should be no need for the void cast here.

#3 lore raw

Re: [PATCH 1/4] mm/hmm: make full use of walk_page_range()

Quoted 16 lines · Christoph Hellwig

mm/hmm.c static int hmm_vma_do_fault(struct mm_walk *walk, unsigned long addr, · line 258

Quoted 4 lines
mm/hmm.c +4 -0
+static int hmm_pfns_fill(unsigned long addr,
+			 unsigned long end,
+			 struct hmm_range *range,
+			 enum hmm_pfn_value_e value)

Nit: can we use the space a little more efficient, e.g.:

static int hmm_pfns_fill(unsigned long addr, unsigned long end,
struct hmm_range *range, enum hmm_pfn_value_e value)

mm/hmm.c static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask, · line 774

Quoted 3 lines
mm/hmm.c +3 -0
+static int hmm_vma_walk_test(unsigned long start,
+			     unsigned long end,
+			     struct mm_walk *walk)

Same here.

Quoted 5 lines

mm/hmm.c static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask, · line 797

Quoted 2 lines
mm/hmm.c +2 -0
+	if (!(vma->vm_flags & VM_READ)) {
+		(void) hmm_pfns_fill(start, end, range, HMM_PFN_NONE);

There should be no need for the void cast here.

OK. I'll post a v2 with the these changes.
Thanks for the reviews.

#4 lore raw

PATCH 2/4 mm/hmm: allow snapshot of the special zero page

In reply to Ralph Campbell

Allow hmm_range_fault() to return success (0) when the CPU pagetable entry points to the special shared zero page.
The caller can then handle the zero page by possibly clearing device private memory instead of DMAing a zero page.

Cc "Jérôme Glisse" <[email protected]>
Cc Jason Gunthorpe <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 mm/hmm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/hmm.c +1 -1
index 06041d4399ff..7217912bef13 100644
@@ -532,7 +532,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
 			return -EBUSY;
 	} else if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL) && pte_special(pte)) {
 		*pfn = range->values[HMM_PFN_SPECIAL];
-		return -EFAULT;
+		return is_zero_pfn(pte_pfn(pte)) ? 0 : -EFAULT;
 	}
 
 	*pfn = hmm_device_entry_from_pfn(range, pte_pfn(pte)) | cpu_flags;
Signature · 1 line
2.20.1
#5 lore raw

Re: [PATCH 2/4] mm/hmm: allow snapshot of the special zero page

Quoted 18 lines · Ralph Campbell

Allow hmm_range_fault() to return success (0) when the CPU pagetable entry points to the special shared zero page.
The caller can then handle the zero page by possibly clearing device private memory instead of DMAing a zero page.

Cc "Jérôme Glisse" <[email protected]>
Cc Jason Gunthorpe <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 mm/hmm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/hmm.c +0 -0
index 06041d4399ff..7217912bef13 100644

mm/hmm.c static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, · line 535

Quoted 5 lines
mm/hmm.c +1 -1
@@ -532,7 +532,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
 			return -EBUSY;
 	} else if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL) && pte_special(pte)) {
 		*pfn = range->values[HMM_PFN_SPECIAL];
-		return -EFAULT;
+		return is_zero_pfn(pte_pfn(pte)) ? 0 : -EFAULT;

Any chance to just use a normal if here:

		if (!is_zero_pfn(pte_pfn(pte)))
			return -EFAULT;
		return 0;

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

#6 lore raw

Re: [PATCH 2/4] mm/hmm: allow snapshot of the special zero page

Quoted 27 lines · Christoph Hellwig
Quoted 18 lines · Ralph Campbell

Allow hmm_range_fault() to return success (0) when the CPU pagetable entry points to the special shared zero page.
The caller can then handle the zero page by possibly clearing device private memory instead of DMAing a zero page.

Cc "Jérôme Glisse" <[email protected]>
Cc Jason Gunthorpe <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
  mm/hmm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
mm/hmm.c +0 -0
index 06041d4399ff..7217912bef13 100644

mm/hmm.c static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, · line 535

Quoted 5 lines
mm/hmm.c +1 -1
@@ -532,7 +532,7 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
  			return -EBUSY;
  	} else if (IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL) && pte_special(pte)) {
  		*pfn = range->values[HMM_PFN_SPECIAL];
-		return -EFAULT;
+		return is_zero_pfn(pte_pfn(pte)) ? 0 : -EFAULT;

Any chance to just use a normal if here:

Quoted 4 lines
		if (!is_zero_pfn(pte_pfn(pte)))
			return -EFAULT;
		return 0;

Sure, no problem.

#7 lore raw

PATCH 3/4 mm/hmm: allow hmm_range_fault() of mmap(PROT_NONE)

In reply to Ralph Campbell

Allow hmm_range_fault() to return success (0) when the range has no access (!(vma->vm_flags & VM_READ)). The range->pfns[] array will be filled with range->values[HMM_PFN_NONE] in this case.
This allows the caller to get a snapshot of a range without having to lookup the vma before calling hmm_range_fault().
If the call to hmm_range_fault() is not a snapshot, the caller can still check that pfns have the desired access permissions.

Cc "Jérôme Glisse" <[email protected]>
Cc Jason Gunthorpe <[email protected]>
Cc Christoph Hellwig <[email protected]>
---
 mm/hmm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
mm/hmm.c +3 -1
index 7217912bef13..16c834e5d1c0 100644
@@ -795,7 +795,9 @@ static int hmm_vma_walk_test(unsigned long start,
 	 */
 	if (!(vma->vm_flags & VM_READ)) {
 		(void) hmm_pfns_fill(start, end, range, HMM_PFN_NONE);
-		return -EPERM;
+
+		/* Skip this vma and continue processing the next vma. */
+		return 1;
 	}
 
 	return 0;
Signature · 1 line
2.20.1
#8 lore raw

PATCH 4/4 mm/hmm/test: add self tests for HMM

In reply to Ralph Campbell

Add self tests for HMM.

---
 MAINTAINERS                            |    3 +
 drivers/char/Kconfig                   |   11 +
 drivers/char/Makefile                  |    1 +
 drivers/char/hmm_dmirror.c             | 1504 ++++++++++++++++++++++++
 include/Kbuild                         |    1 +
 include/uapi/linux/hmm_dmirror.h       |   74 ++
 tools/testing/selftests/vm/.gitignore  |    1 +
 tools/testing/selftests/vm/Makefile    |    3 +
 tools/testing/selftests/vm/config      |    3 +
 tools/testing/selftests/vm/hmm-tests.c | 1304 ++++++++++++++++++++
 tools/testing/selftests/vm/run_vmtests |   16 +
 tools/testing/selftests/vm/test_hmm.sh |  105 ++
 12 files changed, 3026 insertions(+)
 create mode 100644 drivers/char/hmm_dmirror.c
 create mode 100644 include/uapi/linux/hmm_dmirror.h
 create mode 100644 tools/testing/selftests/vm/hmm-tests.c
 create mode 100755 tools/testing/selftests/vm/test_hmm.sh
MAINTAINERS +3 -0
index 43604d6ab96c..8ab242d91876 100644
@@ -7318,8 +7318,11 @@ M:	Jérôme Glisse <[email protected]>
 L:	[email protected]
 S:	Maintained
 F:	mm/hmm*
+F:	drivers/char/hmm*
 F:	include/linux/hmm*
+F:	include/uapi/linux/hmm*
 F:	Documentation/vm/hmm.rst
+F:	tools/testing/selftests/vm/*hmm*
 
 HOST AP DRIVER
 M:	Jouni Malinen <[email protected]>
drivers/char/Kconfig +11 -0
index 3e866885a405..b4ad868ead63 100644
@@ -557,6 +557,17 @@ config ADI
 	  and SSM (Silicon Secured Memory).  Intended consumers of this
 	  driver include crash and makedumpfile.
 
+config HMM_DMIRROR
+	tristate "HMM driver for testing Heterogeneous Memory Management"
+	depends on HMM_MIRROR
+	depends on DEVICE_PRIVATE
+	help
+	  This is a pseudo device driver solely for testing HMM.
+	  Say Y here if you want to build the HMM test driver.
+	  Doing so will allow you to run tools/testing/selftest/vm/hmm-tests.
+
+	  If in doubt, say "N".
+
 endmenu
 
 config RANDOM_TRUST_CPU
drivers/char/Makefile +1 -0
index fbea7dd12932..c9ddd8e550c5 100644
@@ -54,3 +54,4 @@ js-rtc-y = rtc.o
 obj-$(CONFIG_XILLYBUS)		+= xillybus/
 obj-$(CONFIG_POWERNV_OP_PANEL)	+= powernv-op-panel.o
 obj-$(CONFIG_ADI)		+= adi.o
+obj-$(CONFIG_HMM_DMIRROR)	+= hmm_dmirror.o
drivers/char/hmm_dmirror.c new file +1504 -0

1,504 lines not on this page · raw message

include/Kbuild +1 -0
index c38f0d46b267..972c168629cd 100644
@@ -1134,6 +1134,7 @@ header-test-			+= uapi/linux/coda_psdev.h
 header-test-			+= uapi/linux/errqueue.h
 header-test-			+= uapi/linux/eventpoll.h
 header-test-			+= uapi/linux/hdlc/ioctl.h
+header-test-			+= uapi/linux/hmm_dmirror.h
 header-test-			+= uapi/linux/input.h
 header-test-			+= uapi/linux/kvm.h
 header-test-			+= uapi/linux/kvm_para.h
include/uapi/linux/hmm_dmirror.h new file +74 -0

74 lines not on this page · raw message

tools/testing/selftests/vm/.gitignore +1 -0
index 31b3c98b6d34..3054565b3f07 100644
@@ -14,3 +14,4 @@ virtual_address_range
 gup_benchmark
 va_128TBswitch
 map_fixed_noreplace
+hmm-tests
tools/testing/selftests/vm/Makefile +3 -0
index 9534dc2bc929..5643cfb5e3d6 100644
@@ -5,6 +5,7 @@ CFLAGS = -Wall -I ../../../../usr/include $(EXTRA_CFLAGS)
 LDLIBS = -lrt
 TEST_GEN_FILES = compaction_test
 TEST_GEN_FILES += gup_benchmark
+TEST_GEN_FILES += hmm-tests
 TEST_GEN_FILES += hugepage-mmap
 TEST_GEN_FILES += hugepage-shm
 TEST_GEN_FILES += map_hugetlb
@@ -26,6 +27,8 @@ TEST_FILES := test_vmalloc.sh
 KSFT_KHDR_INSTALL := 1
 include ../lib.mk
 
+$(OUTPUT)/hmm-tests: LDLIBS += -lhugetlbfs -lpthread
+
 $(OUTPUT)/userfaultfd: LDLIBS += -lpthread
 
 $(OUTPUT)/mlock-random-test: LDLIBS += -lcap
tools/testing/selftests/vm/config +3 -0
index 1c0d76cb5adf..34cfab18e737 100644
@@ -1,2 +1,5 @@
 CONFIG_SYSVIPC=y
 CONFIG_USERFAULTFD=y
+CONFIG_HMM_MIRROR=y
+CONFIG_DEVICE_PRIVATE=y
+CONFIG_HMM_DMIRROR=m
tools/testing/selftests/vm/hmm-tests.c new file +1304 -0

1,304 lines not on this page · raw message

tools/testing/selftests/vm/run_vmtests +16 -0
index 951c507a27f7..634cfefdaffd 100755
@@ -227,4 +227,20 @@ else
 	exitcode=1
 fi
 
+echo "------------------------------------"
+echo "running HMM smoke test"
+echo "------------------------------------"
+./test_hmm.sh smoke
+ret_val=$?
+
+if [ $ret_val -eq 0 ]; then
+	echo "[PASS]"
+elif [ $ret_val -eq $ksft_skip ]; then
+	echo "[SKIP]"
+	exitcode=$ksft_skip
+else
+	echo "[FAIL]"
+	exitcode=1
+fi
+
 exit $exitcode
tools/testing/selftests/vm/test_hmm.sh new file +105 -0

35 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line
2.20.1

Part of the diff is not shown here. View the raw message.