[PATCH 0/9] Emulated coherent graphics memory

[PATCH 0/9] Emulated coherent graphics memory

[Thomas Hellstrom]

Graphics APIs like OpenGL 4.4 and Vulkan require the graphics driver
to provide coherent graphics memory, meaning that the GPU sees any
content written to the coherent memory on the next GPU operation that
touches that memory, and the CPU sees any content written by the GPU
to that memory immediately after any fence object trailing the GPU
operation has signaled.

Paravirtual drivers that otherwise require explicit synchronization
needs to do this by hooking up dirty tracking to pagefault handlers
and buffer object validation. This is a first attempt to do that for
the vmwgfx driver.

The mm patches has been out for RFC. I think I have addressed all the
feedback I got, except a possible softdirty breakage. But although the
dirty-tracking and softdirty may write-protect PTEs both care about,
that shouldn't really cause any operation interference. In particular
since we use the hardware dirty PTE bits and softdirty uses other PTE bits.

For the TTM changes they are hopefully in line with the long-term
strategy of making helpers out of what's left of TTM.

The code has been tested and excercised by a tailored version of mesa
where we disable all explicit synchronization and assume graphics memory
is coherent. The performance loss varies of course; a typical number is
around 5%.

Any feedback greatly appreciated.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: linux-mm@kvack.org

[PATCH 1/9] mm: Allow the [page|pfn]_mkwrite callbacks to drop the mmap_sem

[Thomas Hellstrom]

Driver fault callbacks are allowed to drop the mmap_sem when expecting
long hardware waits to avoid blocking other mm users. Allow the mkwrite
callbacks to do the same by returning early on VM_FAULT_RETRY.

In particular we want to be able to drop the mmap_sem when waiting for
a reservation object lock on a GPU buffer object. These locks may be
held while waiting for the GPU.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 mm/memory.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index e11ca9dd823f..a95b4a3b1ae2 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2144,7 +2144,7 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
 	ret = vmf->vma->vm_ops->page_mkwrite(vmf);
 	/* Restore original flags so that caller is not surprised */
 	vmf->flags = old_flags;
-	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
+	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_RETRY | VM_FAULT_NOPAGE)))
 		return ret;
 	if (unlikely(!(ret & VM_FAULT_LOCKED))) {
 		lock_page(page);
@@ -2419,7 +2419,7 @@ static vm_fault_t wp_pfn_shared(struct vm_fault *vmf)
 		pte_unmap_unlock(vmf->pte, vmf->ptl);
 		vmf->flags |= FAULT_FLAG_MKWRITE;
 		ret = vma->vm_ops->pfn_mkwrite(vmf);
-		if (ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE))
+		if (ret & (VM_FAULT_ERROR | VM_FAULT_RETRY | VM_FAULT_NOPAGE))
 			return ret;
 		return finish_mkwrite_fault(vmf);
 	}
@@ -2440,7 +2440,8 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf)
 		pte_unmap_unlock(vmf->pte, vmf->ptl);
 		tmp = do_page_mkwrite(vmf);
 		if (unlikely(!tmp || (tmp &
-				      (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) {
+				      (VM_FAULT_ERROR | VM_FAULT_RETRY |
+				       VM_FAULT_NOPAGE)))) {
 			put_page(vmf->page);
 			return tmp;
 		}
@@ -3494,7 +3495,8 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
 		unlock_page(vmf->page);
 		tmp = do_page_mkwrite(vmf);
 		if (unlikely(!tmp ||
-				(tmp & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) {
+				(tmp & (VM_FAULT_ERROR | VM_FAULT_RETRY |
+					VM_FAULT_NOPAGE)))) {
 			put_page(vmf->page);
 			return tmp;
 		}
-- 
2.20.1

[PATCH 2/9] mm: Add an apply_to_pfn_range interface

[Thomas Hellstrom]

This is basically apply_to_page_range with added functionality:
Allocating missing parts of the page table becomes optional, which
means that the function can be guaranteed not to error if allocation
is disabled. Also passing of the closure struct and callback function
becomes different and more in line with how things are done elsewhere.

Finally we keep apply_to_page_range as a wrapper around apply_to_pfn_range

The reason for not using the page-walk code is that we want to perform
the page-walk on vmas pointing to an address space without requiring the
mmap_sem to be held rather thand on vmas belonging to a process with the
mmap_sem held.

Notable changes since RFC:
Don't export apply_to_pfn range.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 include/linux/mm.h |  10 ++++
 mm/memory.c        | 130 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 108 insertions(+), 32 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 80bb6408fe73..b7dd4ddd6efb 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2632,6 +2632,16 @@ typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
 extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
 			       unsigned long size, pte_fn_t fn, void *data);
 
+struct pfn_range_apply;
+typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
+			 struct pfn_range_apply *closure);
+struct pfn_range_apply {
+	struct mm_struct *mm;
+	pter_fn_t ptefn;
+	unsigned int alloc;
+};
+extern int apply_to_pfn_range(struct pfn_range_apply *closure,
+			      unsigned long address, unsigned long size);
 
 #ifdef CONFIG_PAGE_POISONING
 extern bool page_poisoning_enabled(void);
diff --git a/mm/memory.c b/mm/memory.c
index a95b4a3b1ae2..60d67158964f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1938,18 +1938,17 @@ int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long
 }
 EXPORT_SYMBOL(vm_iomap_memory);
 
-static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
-				     unsigned long addr, unsigned long end,
-				     pte_fn_t fn, void *data)
+static int apply_to_pte_range(struct pfn_range_apply *closure, pmd_t *pmd,
+			      unsigned long addr, unsigned long end)
 {
 	pte_t *pte;
 	int err;
 	pgtable_t token;
 	spinlock_t *uninitialized_var(ptl);
 
-	pte = (mm == &init_mm) ?
+	pte = (closure->mm == &init_mm) ?
 		pte_alloc_kernel(pmd, addr) :
-		pte_alloc_map_lock(mm, pmd, addr, &ptl);
+		pte_alloc_map_lock(closure->mm, pmd, addr, &ptl);
 	if (!pte)
 		return -ENOMEM;
 
@@ -1960,86 +1959,107 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
 	token = pmd_pgtable(*pmd);
 
 	do {
-		err = fn(pte++, token, addr, data);
+		err = closure->ptefn(pte++, token, addr, closure);
 		if (err)
 			break;
 	} while (addr += PAGE_SIZE, addr != end);
 
 	arch_leave_lazy_mmu_mode();
 
-	if (mm != &init_mm)
+	if (closure->mm != &init_mm)
 		pte_unmap_unlock(pte-1, ptl);
 	return err;
 }
 
-static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
-				     unsigned long addr, unsigned long end,
-				     pte_fn_t fn, void *data)
+static int apply_to_pmd_range(struct pfn_range_apply *closure, pud_t *pud,
+			      unsigned long addr, unsigned long end)
 {
 	pmd_t *pmd;
 	unsigned long next;
-	int err;
+	int err = 0;
 
 	BUG_ON(pud_huge(*pud));
 
-	pmd = pmd_alloc(mm, pud, addr);
+	pmd = pmd_alloc(closure->mm, pud, addr);
 	if (!pmd)
 		return -ENOMEM;
+
 	do {
 		next = pmd_addr_end(addr, end);
-		err = apply_to_pte_range(mm, pmd, addr, next, fn, data);
+		if (!closure->alloc && pmd_none_or_clear_bad(pmd))
+			continue;
+		err = apply_to_pte_range(closure, pmd, addr, next);
 		if (err)
 			break;
 	} while (pmd++, addr = next, addr != end);
 	return err;
 }
 
-static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
-				     unsigned long addr, unsigned long end,
-				     pte_fn_t fn, void *data)
+static int apply_to_pud_range(struct pfn_range_apply *closure, p4d_t *p4d,
+			      unsigned long addr, unsigned long end)
 {
 	pud_t *pud;
 	unsigned long next;
-	int err;
+	int err = 0;
 
-	pud = pud_alloc(mm, p4d, addr);
+	pud = pud_alloc(closure->mm, p4d, addr);
 	if (!pud)
 		return -ENOMEM;
+
 	do {
 		next = pud_addr_end(addr, end);
-		err = apply_to_pmd_range(mm, pud, addr, next, fn, data);
+		if (!closure->alloc && pud_none_or_clear_bad(pud))
+			continue;
+		err = apply_to_pmd_range(closure, pud, addr, next);
 		if (err)
 			break;
 	} while (pud++, addr = next, addr != end);
 	return err;
 }
 
-static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
-				     unsigned long addr, unsigned long end,
-				     pte_fn_t fn, void *data)
+static int apply_to_p4d_range(struct pfn_range_apply *closure, pgd_t *pgd,
+			      unsigned long addr, unsigned long end)
 {
 	p4d_t *p4d;
 	unsigned long next;
-	int err;
+	int err = 0;
 
-	p4d = p4d_alloc(mm, pgd, addr);
+	p4d = p4d_alloc(closure->mm, pgd, addr);
 	if (!p4d)
 		return -ENOMEM;
+
 	do {
 		next = p4d_addr_end(addr, end);
-		err = apply_to_pud_range(mm, p4d, addr, next, fn, data);
+		if (!closure->alloc && p4d_none_or_clear_bad(p4d))
+			continue;
+		err = apply_to_pud_range(closure, p4d, addr, next);
 		if (err)
 			break;
 	} while (p4d++, addr = next, addr != end);
 	return err;
 }
 
-/*
- * Scan a region of virtual memory, filling in page tables as necessary
- * and calling a provided function on each leaf page table.
+/**
+ * apply_to_pfn_range - Scan a region of virtual memory, calling a provided
+ * function on each leaf page table entry
+ * @closure: Details about how to scan and what function to apply
+ * @addr: Start virtual address
+ * @size: Size of the region
+ *
+ * If @closure->alloc is set to 1, the function will fill in the page table
+ * as necessary. Otherwise it will skip non-present parts.
+ * Note: The caller must ensure that the range does not contain huge pages.
+ * The caller must also assure that the proper mmu_notifier functions are
+ * called. Either in the pte leaf function or before and after the call to
+ * apply_to_pfn_range.
+ *
+ * Returns: Zero on success. If the provided function returns a non-zero status,
+ * the page table walk will terminate and that status will be returned.
+ * If @closure->alloc is set to 1, then this function may also return memory
+ * allocation errors arising from allocating page table memory.
  */
-int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
-			unsigned long size, pte_fn_t fn, void *data)
+int apply_to_pfn_range(struct pfn_range_apply *closure,
+		       unsigned long addr, unsigned long size)
 {
 	pgd_t *pgd;
 	unsigned long next;
@@ -2049,16 +2069,62 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
 	if (WARN_ON(addr >= end))
 		return -EINVAL;
 
-	pgd = pgd_offset(mm, addr);
+	pgd = pgd_offset(closure->mm, addr);
 	do {
 		next = pgd_addr_end(addr, end);
-		err = apply_to_p4d_range(mm, pgd, addr, next, fn, data);
+		if (!closure->alloc && pgd_none_or_clear_bad(pgd))
+			continue;
+		err = apply_to_p4d_range(closure, pgd, addr, next);
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
 
 	return err;
 }
+
+/**
+ * struct page_range_apply - Closure structure for apply_to_page_range()
+ * @pter: The base closure structure we derive from
+ * @fn: The leaf pte function to call
+ * @data: The leaf pte function closure
+ */
+struct page_range_apply {
+	struct pfn_range_apply pter;
+	pte_fn_t fn;
+	void *data;
+};
+
+/*
+ * Callback wrapper to enable use of apply_to_pfn_range for
+ * the apply_to_page_range interface
+ */
+static int apply_to_page_range_wrapper(pte_t *pte, pgtable_t token,
+				       unsigned long addr,
+				       struct pfn_range_apply *pter)
+{
+	struct page_range_apply *pra =
+		container_of(pter, typeof(*pra), pter);
+
+	return pra->fn(pte, token, addr, pra->data);
+}
+
+/*
+ * Scan a region of virtual memory, filling in page tables as necessary
+ * and calling a provided function on each leaf page table.
+ */
+int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
+			unsigned long size, pte_fn_t fn, void *data)
+{
+	struct page_range_apply pra = {
+		.pter = {.mm = mm,
+			 .alloc = 1,
+			 .ptefn = apply_to_page_range_wrapper },
+		.fn = fn,
+		.data = data
+	};
+
+	return apply_to_pfn_range(&pra.pter, addr, size);
+}
 EXPORT_SYMBOL_GPL(apply_to_page_range);
 
 /*
-- 
2.20.1

[PATCH 3/9] mm: Add write-protect and clean utilities for address space ranges

[Thomas Hellstrom]

Add two utilities to a) write-protect and b) clean all ptes pointing into
a range of an address space
The utilities are intended to aid in tracking dirty pages (either
driver-allocated system memory or pci device memory).
The write-protect utility should be used in conjunction with
page_mkwrite() and pfn_mkwrite() to trigger write page-faults on page
accesses. Typically one would want to use this on sparse accesses into
large memory regions. The clean utility should be used to utilize
hardware dirtying functionality and avoid the overhead of page-faults,
typically on large accesses into small memory regions.

The added file "apply_as_range.c" is initially listed as maintained by
VMware under our DRM driver. If somebody would like it elsewhere,
that's of course no problem.

Notable changes since RFC:
- Added comments to help avoid the usage of these function for VMAs
  it's not intended for. We also do advisory checks on the vm_flags and
  warn on illegal usage.
- Perform the pte modifications the same way softdirty does.
- Add mmu_notifier range invalidation calls.
- Add a config option so that this code is not unconditionally included.
- Tell the mmu_gather code about pending tlb flushes.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 MAINTAINERS         |   1 +
 include/linux/mm.h  |   9 +-
 mm/Kconfig          |   3 +
 mm/Makefile         |   3 +-
 mm/apply_as_range.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 309 insertions(+), 2 deletions(-)
 create mode 100644 mm/apply_as_range.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 35e6357f9d30..bc243ffcb840 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4971,6 +4971,7 @@ T:	git git://people.freedesktop.org/~thomash/linux
 S:	Supported
 F:	drivers/gpu/drm/vmwgfx/
 F:	include/uapi/drm/vmwgfx_drm.h
+F:	mm/apply_as_range.c
 
 DRM DRIVERS
 M:	David Airlie <airlied@linux.ie>
diff --git a/include/linux/mm.h b/include/linux/mm.h
index b7dd4ddd6efb..62f24dd0bfa0 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2642,7 +2642,14 @@ struct pfn_range_apply {
 };
 extern int apply_to_pfn_range(struct pfn_range_apply *closure,
 			      unsigned long address, unsigned long size);
-
+unsigned long apply_as_wrprotect(struct address_space *mapping,
+				 pgoff_t first_index, pgoff_t nr);
+unsigned long apply_as_clean(struct address_space *mapping,
+			     pgoff_t first_index, pgoff_t nr,
+			     pgoff_t bitmap_pgoff,
+			     unsigned long *bitmap,
+			     pgoff_t *start,
+			     pgoff_t *end);
 #ifdef CONFIG_PAGE_POISONING
 extern bool page_poisoning_enabled(void);
 extern void kernel_poison_pages(struct page *page, int numpages, int enable);
diff --git a/mm/Kconfig b/mm/Kconfig
index 25c71eb8a7db..80e41cdbb4ae 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -758,4 +758,7 @@ config GUP_BENCHMARK
 config ARCH_HAS_PTE_SPECIAL
 	bool
 
+config AS_DIRTY_HELPERS
+        bool
+
 endmenu
diff --git a/mm/Makefile b/mm/Makefile
index d210cc9d6f80..b295717be856 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -39,7 +39,7 @@ obj-y			:= filemap.o mempool.o oom_kill.o fadvise.o \
 			   mm_init.o mmu_context.o percpu.o slab_common.o \
 			   compaction.o vmacache.o \
 			   interval_tree.o list_lru.o workingset.o \
-			   debug.o $(mmu-y)
+			   debug.o apply_as_range.o $(mmu-y)
 
 obj-y += init-mm.o
 obj-y += memblock.o
@@ -99,3 +99,4 @@ obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o
 obj-$(CONFIG_PERCPU_STATS) += percpu-stats.o
 obj-$(CONFIG_HMM) += hmm.o
 obj-$(CONFIG_MEMFD_CREATE) += memfd.o
+obj-$(CONFIG_AS_DIRTY_HELPERS) += apply_as_range.o
diff --git a/mm/apply_as_range.c b/mm/apply_as_range.c
new file mode 100644
index 000000000000..32d28619aec5
--- /dev/null
+++ b/mm/apply_as_range.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/mm.h>
+#include <linux/mm_types.h>
+#include <linux/hugetlb.h>
+#include <linux/bitops.h>
+#include <linux/mmu_notifier.h>
+#include <asm/cacheflush.h>
+#include <asm/tlbflush.h>
+
+/**
+ * struct apply_as - Closure structure for apply_as_range
+ * @base: struct pfn_range_apply we derive from
+ * @start: Address of first modified pte
+ * @end: Address of last modified pte + 1
+ * @total: Total number of modified ptes
+ * @vma: Pointer to the struct vm_area_struct we're currently operating on
+ */
+struct apply_as {
+	struct pfn_range_apply base;
+	unsigned long start, end;
+	unsigned long total;
+	const struct vm_area_struct *vma;
+};
+
+/**
+ * apply_pt_wrprotect - Leaf pte callback to write-protect a pte
+ * @pte: Pointer to the pte
+ * @token: Page table token, see apply_to_pfn_range()
+ * @addr: The virtual page address
+ * @closure: Pointer to a struct pfn_range_apply embedded in a
+ * struct apply_as
+ *
+ * The function write-protects a pte and records the range in
+ * virtual address space of touched ptes for efficient range TLB flushes.
+ *
+ * Return: Always zero.
+ */
+static int apply_pt_wrprotect(pte_t *pte, pgtable_t token,
+			      unsigned long addr,
+			      struct pfn_range_apply *closure)
+{
+	struct apply_as *aas = container_of(closure, typeof(*aas), base);
+	pte_t ptent = *pte;
+
+	if (pte_write(ptent)) {
+		ptent = ptep_modify_prot_start(closure->mm, addr, pte);
+		ptent = pte_wrprotect(ptent);
+		ptep_modify_prot_commit(closure->mm, addr, pte, ptent);
+		aas->total++;
+		aas->start = min(aas->start, addr);
+		aas->end = max(aas->end, addr + PAGE_SIZE);
+	}
+
+	return 0;
+}
+
+/**
+ * struct apply_as_clean - Closure structure for apply_as_clean
+ * @base: struct apply_as we derive from
+ * @bitmap_pgoff: Address_space Page offset of the first bit in @bitmap
+ * @bitmap: Bitmap with one bit for each page offset in the address_space range
+ * covered.
+ * @start: Address_space page offset of first modified pte relative
+ * to @bitmap_pgoff
+ * @end: Address_space page offset of last modified pte relative
+ * to @bitmap_pgoff
+ */
+struct apply_as_clean {
+	struct apply_as base;
+	pgoff_t bitmap_pgoff;
+	unsigned long *bitmap;
+	pgoff_t start, end;
+};
+
+/**
+ * apply_pt_clean - Leaf pte callback to clean a pte
+ * @pte: Pointer to the pte
+ * @token: Page table token, see apply_to_pfn_range()
+ * @addr: The virtual page address
+ * @closure: Pointer to a struct pfn_range_apply embedded in a
+ * struct apply_as_clean
+ *
+ * The function cleans a pte and records the range in
+ * virtual address space of touched ptes for efficient TLB flushes.
+ * It also records dirty ptes in a bitmap representing page offsets
+ * in the address_space, as well as the first and last of the bits
+ * touched.
+ *
+ * Return: Always zero.
+ */
+static int apply_pt_clean(pte_t *pte, pgtable_t token,
+			  unsigned long addr,
+			  struct pfn_range_apply *closure)
+{
+	struct apply_as *aas = container_of(closure, typeof(*aas), base);
+	struct apply_as_clean *clean = container_of(aas, typeof(*clean), base);
+	pte_t ptent = *pte;
+
+	if (pte_dirty(ptent)) {
+		pgoff_t pgoff = ((addr - aas->vma->vm_start) >> PAGE_SHIFT) +
+			aas->vma->vm_pgoff - clean->bitmap_pgoff;
+
+		ptent = ptep_modify_prot_start(closure->mm, addr, pte);
+		ptent = pte_mkclean(ptent);
+		ptep_modify_prot_commit(closure->mm, addr, pte, ptent);
+
+		aas->total++;
+		aas->start = min(aas->start, addr);
+		aas->end = max(aas->end, addr + PAGE_SIZE);
+
+		__set_bit(pgoff, clean->bitmap);
+		clean->start = min(clean->start, pgoff);
+		clean->end = max(clean->end, pgoff + 1);
+	}
+
+	return 0;
+}
+
+/**
+ * apply_as_range - Apply a pte callback to all PTEs pointing into a range
+ * of an address_space.
+ * @mapping: Pointer to the struct address_space
+ * @aas: Closure structure
+ * @first_index: First page offset in the address_space
+ * @nr: Number of incremental page offsets to cover
+ *
+ * Return: Number of ptes touched. Note that this number might be larger
+ * than @nr if there are overlapping vmas
+ */
+static unsigned long apply_as_range(struct address_space *mapping,
+				    struct apply_as *aas,
+				    pgoff_t first_index, pgoff_t nr)
+{
+	struct vm_area_struct *vma;
+	pgoff_t vba, vea, cba, cea;
+	unsigned long start_addr, end_addr;
+	struct mmu_notifier_range range;
+
+	i_mmap_lock_read(mapping);
+	vma_interval_tree_foreach(vma, &mapping->i_mmap, first_index,
+				  first_index + nr - 1) {
+		unsigned long vm_flags = READ_ONCE(vma->vm_flags);
+
+		/*
+		 * We can only do advisory flag tests below, since we can't
+		 * require the vm's mmap_sem to be held to protect the flags.
+		 * Therefore, callers that strictly depend on specific mmap
+		 * flags to remain constant throughout the operation must
+		 * either ensure those flags are immutable for all relevant
+		 * vmas or can't use this function. Fixing this properly would
+		 * require the vma::vm_flags to be protected by a separate
+		 * lock taken after the i_mmap_lock
+		 */
+
+		/* Skip non-applicable VMAs */
+		if ((vm_flags & (VM_SHARED | VM_WRITE)) !=
+		    (VM_SHARED | VM_WRITE))
+			continue;
+
+		/* Warn on and skip VMAs whose flags indicate illegal usage */
+		if (WARN_ON((vm_flags & (VM_HUGETLB | VM_IO)) != VM_IO))
+			continue;
+
+		/* Clip to the vma */
+		vba = vma->vm_pgoff;
+		vea = vba + vma_pages(vma);
+		cba = first_index;
+		cba = max(cba, vba);
+		cea = first_index + nr;
+		cea = min(cea, vea);
+
+		/* Translate to virtual address */
+		start_addr = ((cba - vba) << PAGE_SHIFT) + vma->vm_start;
+		end_addr = ((cea - vba) << PAGE_SHIFT) + vma->vm_start;
+		if (start_addr >= end_addr)
+			continue;
+
+		aas->base.mm = vma->vm_mm;
+		aas->vma = vma;
+		aas->start = end_addr;
+		aas->end = start_addr;
+
+		mmu_notifier_range_init(&range, vma->vm_mm,
+					start_addr, end_addr);
+		mmu_notifier_invalidate_range_start(&range);
+
+		/* Needed when we only change protection? */
+		flush_cache_range(vma, start_addr, end_addr);
+
+		/*
+		 * We're not using tlb_gather_mmu() since typically
+		 * only a small subrange of PTEs are affected.
+		 */
+		inc_tlb_flush_pending(vma->vm_mm);
+
+		/* Should not error since aas->base.alloc == 0 */
+		WARN_ON(apply_to_pfn_range(&aas->base, start_addr,
+					   end_addr - start_addr));
+		if (aas->end > aas->start)
+			flush_tlb_range(vma, aas->start, aas->end);
+
+		mmu_notifier_invalidate_range_end(&range);
+		dec_tlb_flush_pending(vma->vm_mm);
+	}
+	i_mmap_unlock_read(mapping);
+
+	return aas->total;
+}
+
+/**
+ * apply_as_wrprotect - Write-protect all ptes in an address_space range
+ * @mapping: The address_space we want to write protect
+ * @first_index: The first page offset in the range
+ * @nr: Number of incremental page offsets to cover
+ *
+ * WARNING: This function should only be used for address spaces that
+ * completely own the pages / memory the page table points to. Typically a
+ * device file.
+ *
+ * Return: The number of ptes actually write-protected. Note that
+ * already write-protected ptes are not counted.
+ */
+unsigned long apply_as_wrprotect(struct address_space *mapping,
+				 pgoff_t first_index, pgoff_t nr)
+{
+	struct apply_as aas = {
+		.base = {
+			.alloc = 0,
+			.ptefn = apply_pt_wrprotect,
+		},
+		.total = 0,
+	};
+
+	return apply_as_range(mapping, &aas, first_index, nr);
+}
+EXPORT_SYMBOL(apply_as_wrprotect);
+
+/**
+ * apply_as_clean - Clean all ptes in an address_space range
+ * @mapping: The address_space we want to clean
+ * @first_index: The first page offset in the range
+ * @nr: Number of incremental page offsets to cover
+ * @bitmap_pgoff: The page offset of the first bit in @bitmap
+ * @bitmap: Pointer to a bitmap of at least @nr bits. The bitmap needs to
+ * cover the whole range @first_index..@first_index + @nr.
+ * @start: Pointer to number of the first set bit in @bitmap.
+ * is modified as new bits are set by the function.
+ * @end: Pointer to the number of the last set bit in @bitmap.
+ * none set. The value is modified as new bets are set by the function.
+ *
+ * Note: When this function returns there is no guarantee that a CPU has
+ * not already dirtied new ptes. However it will not clean any ptes not
+ * reported in the bitmap.
+ *
+ * If a caller needs to make sure all dirty ptes are picked up and none
+ * additional are added, it first needs to write-protect the address-space
+ * range and make sure new writers are blocked in page_mkwrite() or
+ * pfn_mkwrite(). And then after a TLB flush following the write-protection
+ * pick upp all dirty bits.
+ *
+ * WARNING: This function should only be used for address spaces that
+ * completely own the pages / memory the page table points to. Typically a
+ * device file.
+ *
+ * Return: The number of dirty ptes actually cleaned.
+ */
+unsigned long apply_as_clean(struct address_space *mapping,
+			     pgoff_t first_index, pgoff_t nr,
+			     pgoff_t bitmap_pgoff,
+			     unsigned long *bitmap,
+			     pgoff_t *start,
+			     pgoff_t *end)
+{
+	bool none_set = (*start >= *end);
+	struct apply_as_clean clean = {
+		.base = {
+			.base = {
+				.alloc = 0,
+				.ptefn = apply_pt_clean,
+			},
+			.total = 0,
+		},
+		.bitmap_pgoff = bitmap_pgoff,
+		.bitmap = bitmap,
+		.start = none_set ? nr : *start,
+		.end = none_set ? 0 : *end,
+	};
+	unsigned long ret = apply_as_range(mapping, &clean.base, first_index,
+					   nr);
+
+	*start = clean.start;
+	*end = clean.end;
+	return ret;
+}
+EXPORT_SYMBOL(apply_as_clean);
-- 
2.20.1

Re: [PATCH 1/9] mm: Allow the [page|pfn]_mkwrite callbacks to drop the mmap_sem

[Ralph Campbell]

On 4/12/19 9:04 AM, Thomas Hellstrom wrote:
> Driver fault callbacks are allowed to drop the mmap_sem when expecting
> long hardware waits to avoid blocking other mm users. Allow the mkwrite
> callbacks to do the same by returning early on VM_FAULT_RETRY.
> 
> In particular we want to be able to drop the mmap_sem when waiting for
> a reservation object lock on a GPU buffer object. These locks may be
> held while waiting for the GPU.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> 
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>

Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>

> ---
>   mm/memory.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index e11ca9dd823f..a95b4a3b1ae2 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2144,7 +2144,7 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
>   	ret = vmf->vma->vm_ops->page_mkwrite(vmf);
>   	/* Restore original flags so that caller is not surprised */
>   	vmf->flags = old_flags;
> -	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
> +	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_RETRY | VM_FAULT_NOPAGE)))

A very minor nit, for consistency elsewhere in mm/memory.c,
could you make this be:
	(VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)

>   		return ret;
>   	if (unlikely(!(ret & VM_FAULT_LOCKED))) {
>   		lock_page(page);
> @@ -2419,7 +2419,7 @@ static vm_fault_t wp_pfn_shared(struct vm_fault *vmf)
>   		pte_unmap_unlock(vmf->pte, vmf->ptl);
>   		vmf->flags |= FAULT_FLAG_MKWRITE;
>   		ret = vma->vm_ops->pfn_mkwrite(vmf);
> -		if (ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE))
> +		if (ret & (VM_FAULT_ERROR | VM_FAULT_RETRY | VM_FAULT_NOPAGE))
>   			return ret;
>   		return finish_mkwrite_fault(vmf);
>   	}
> @@ -2440,7 +2440,8 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf)
>   		pte_unmap_unlock(vmf->pte, vmf->ptl);
>   		tmp = do_page_mkwrite(vmf);
>   		if (unlikely(!tmp || (tmp &
> -				      (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) {
> +				      (VM_FAULT_ERROR | VM_FAULT_RETRY |
> +				       VM_FAULT_NOPAGE)))) {
>   			put_page(vmf->page);
>   			return tmp;
>   		}
> @@ -3494,7 +3495,8 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
>   		unlock_page(vmf->page);
>   		tmp = do_page_mkwrite(vmf);
>   		if (unlikely(!tmp ||
> -				(tmp & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) {
> +				(tmp & (VM_FAULT_ERROR | VM_FAULT_RETRY |
> +					VM_FAULT_NOPAGE)))) {
>   			put_page(vmf->page);
>   			return tmp;
>   		}
> 

Re: [PATCH 2/9] mm: Add an apply_to_pfn_range interface

[Ralph Campbell]

On 4/12/19 9:04 AM, Thomas Hellstrom wrote:
> This is basically apply_to_page_range with added functionality:
> Allocating missing parts of the page table becomes optional, which
> means that the function can be guaranteed not to error if allocation
> is disabled. Also passing of the closure struct and callback function
> becomes different and more in line with how things are done elsewhere.
> 
> Finally we keep apply_to_page_range as a wrapper around apply_to_pfn_range
> 
> The reason for not using the page-walk code is that we want to perform
> the page-walk on vmas pointing to an address space without requiring the
> mmap_sem to be held rather thand on vmas belonging to a process with the

s/thand/than/

> mmap_sem held.
> 
> Notable changes since RFC:
> Don't export apply_to_pfn range.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>

Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>

> ---
>   include/linux/mm.h |  10 ++++
>   mm/memory.c        | 130 ++++++++++++++++++++++++++++++++++-----------
>   2 files changed, 108 insertions(+), 32 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 80bb6408fe73..b7dd4ddd6efb 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2632,6 +2632,16 @@ typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
>   extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
>   			       unsigned long size, pte_fn_t fn, void *data);
>   
> +struct pfn_range_apply;
> +typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
> +			 struct pfn_range_apply *closure);
> +struct pfn_range_apply {
> +	struct mm_struct *mm;
> +	pter_fn_t ptefn;
> +	unsigned int alloc;
> +};
> +extern int apply_to_pfn_range(struct pfn_range_apply *closure,
> +			      unsigned long address, unsigned long size);
>   
>   #ifdef CONFIG_PAGE_POISONING
>   extern bool page_poisoning_enabled(void);
> diff --git a/mm/memory.c b/mm/memory.c
> index a95b4a3b1ae2..60d67158964f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1938,18 +1938,17 @@ int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long
>   }
>   EXPORT_SYMBOL(vm_iomap_memory);
>   
> -static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pte_range(struct pfn_range_apply *closure, pmd_t *pmd,
> +			      unsigned long addr, unsigned long end)
>   {
>   	pte_t *pte;
>   	int err;
>   	pgtable_t token;
>   	spinlock_t *uninitialized_var(ptl);
>   
> -	pte = (mm == &init_mm) ?
> +	pte = (closure->mm == &init_mm) ?
>   		pte_alloc_kernel(pmd, addr) :
> -		pte_alloc_map_lock(mm, pmd, addr, &ptl);
> +		pte_alloc_map_lock(closure->mm, pmd, addr, &ptl);
>   	if (!pte)
>   		return -ENOMEM;
>   
> @@ -1960,86 +1959,107 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
>   	token = pmd_pgtable(*pmd);
>   
>   	do {
> -		err = fn(pte++, token, addr, data);
> +		err = closure->ptefn(pte++, token, addr, closure);
>   		if (err)
>   			break;
>   	} while (addr += PAGE_SIZE, addr != end);
>   
>   	arch_leave_lazy_mmu_mode();
>   
> -	if (mm != &init_mm)
> +	if (closure->mm != &init_mm)
>   		pte_unmap_unlock(pte-1, ptl);
>   	return err;
>   }
>   
> -static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pmd_range(struct pfn_range_apply *closure, pud_t *pud,
> +			      unsigned long addr, unsigned long end)
>   {
>   	pmd_t *pmd;
>   	unsigned long next;
> -	int err;
> +	int err = 0;
>   
>   	BUG_ON(pud_huge(*pud));
>   
> -	pmd = pmd_alloc(mm, pud, addr);
> +	pmd = pmd_alloc(closure->mm, pud, addr);
>   	if (!pmd)
>   		return -ENOMEM;
> +
>   	do {
>   		next = pmd_addr_end(addr, end);
> -		err = apply_to_pte_range(mm, pmd, addr, next, fn, data);
> +		if (!closure->alloc && pmd_none_or_clear_bad(pmd))
> +			continue;
> +		err = apply_to_pte_range(closure, pmd, addr, next);
>   		if (err)
>   			break;
>   	} while (pmd++, addr = next, addr != end);
>   	return err;
>   }
>   
> -static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pud_range(struct pfn_range_apply *closure, p4d_t *p4d,
> +			      unsigned long addr, unsigned long end)
>   {
>   	pud_t *pud;
>   	unsigned long next;
> -	int err;
> +	int err = 0;
>   
> -	pud = pud_alloc(mm, p4d, addr);
> +	pud = pud_alloc(closure->mm, p4d, addr);
>   	if (!pud)
>   		return -ENOMEM;
> +
>   	do {
>   		next = pud_addr_end(addr, end);
> -		err = apply_to_pmd_range(mm, pud, addr, next, fn, data);
> +		if (!closure->alloc && pud_none_or_clear_bad(pud))
> +			continue;
> +		err = apply_to_pmd_range(closure, pud, addr, next);
>   		if (err)
>   			break;
>   	} while (pud++, addr = next, addr != end);
>   	return err;
>   }
>   
> -static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_p4d_range(struct pfn_range_apply *closure, pgd_t *pgd,
> +			      unsigned long addr, unsigned long end)
>   {
>   	p4d_t *p4d;
>   	unsigned long next;
> -	int err;
> +	int err = 0;
>   
> -	p4d = p4d_alloc(mm, pgd, addr);
> +	p4d = p4d_alloc(closure->mm, pgd, addr);
>   	if (!p4d)
>   		return -ENOMEM;
> +
>   	do {
>   		next = p4d_addr_end(addr, end);
> -		err = apply_to_pud_range(mm, p4d, addr, next, fn, data);
> +		if (!closure->alloc && p4d_none_or_clear_bad(p4d))
> +			continue;
> +		err = apply_to_pud_range(closure, p4d, addr, next);
>   		if (err)
>   			break;
>   	} while (p4d++, addr = next, addr != end);
>   	return err;
>   }
>   
> -/*
> - * Scan a region of virtual memory, filling in page tables as necessary
> - * and calling a provided function on each leaf page table.
> +/**
> + * apply_to_pfn_range - Scan a region of virtual memory, calling a provided
> + * function on each leaf page table entry
> + * @closure: Details about how to scan and what function to apply
> + * @addr: Start virtual address
> + * @size: Size of the region
> + *
> + * If @closure->alloc is set to 1, the function will fill in the page table
> + * as necessary. Otherwise it will skip non-present parts.
> + * Note: The caller must ensure that the range does not contain huge pages.
> + * The caller must also assure that the proper mmu_notifier functions are
> + * called. Either in the pte leaf function or before and after the call to
> + * apply_to_pfn_range.
> + *
> + * Returns: Zero on success. If the provided function returns a non-zero status,

s/Returns/Return/
See Documentation/kernel-guide/kernel-doc.rst

> + * the page table walk will terminate and that status will be returned.
> + * If @closure->alloc is set to 1, then this function may also return memory
> + * allocation errors arising from allocating page table memory.
>    */
> -int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> -			unsigned long size, pte_fn_t fn, void *data)
> +int apply_to_pfn_range(struct pfn_range_apply *closure,
> +		       unsigned long addr, unsigned long size)
>   {
>   	pgd_t *pgd;
>   	unsigned long next;
> @@ -2049,16 +2069,62 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
>   	if (WARN_ON(addr >= end))
>   		return -EINVAL;
>   
> -	pgd = pgd_offset(mm, addr);
> +	pgd = pgd_offset(closure->mm, addr);
>   	do {
>   		next = pgd_addr_end(addr, end);
> -		err = apply_to_p4d_range(mm, pgd, addr, next, fn, data);
> +		if (!closure->alloc && pgd_none_or_clear_bad(pgd))
> +			continue;
> +		err = apply_to_p4d_range(closure, pgd, addr, next);
>   		if (err)
>   			break;
>   	} while (pgd++, addr = next, addr != end);
>   
>   	return err;
>   }
> +
> +/**
> + * struct page_range_apply - Closure structure for apply_to_page_range()
> + * @pter: The base closure structure we derive from
> + * @fn: The leaf pte function to call
> + * @data: The leaf pte function closure
> + */
> +struct page_range_apply {
> +	struct pfn_range_apply pter;
> +	pte_fn_t fn;
> +	void *data;
> +};
> +
> +/*
> + * Callback wrapper to enable use of apply_to_pfn_range for
> + * the apply_to_page_range interface
> + */
> +static int apply_to_page_range_wrapper(pte_t *pte, pgtable_t token,
> +				       unsigned long addr,
> +				       struct pfn_range_apply *pter)
> +{
> +	struct page_range_apply *pra =
> +		container_of(pter, typeof(*pra), pter);
> +
> +	return pra->fn(pte, token, addr, pra->data);
> +}
> +
> +/*
> + * Scan a region of virtual memory, filling in page tables as necessary
> + * and calling a provided function on each leaf page table.
> + */
> +int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> +			unsigned long size, pte_fn_t fn, void *data)
> +{
> +	struct page_range_apply pra = {
> +		.pter = {.mm = mm,
> +			 .alloc = 1,
> +			 .ptefn = apply_to_page_range_wrapper },
> +		.fn = fn,
> +		.data = data
> +	};
> +
> +	return apply_to_pfn_range(&pra.pter, addr, size);
> +}
>   EXPORT_SYMBOL_GPL(apply_to_page_range);
>   
>   /*
> 

Re: [PATCH 3/9] mm: Add write-protect and clean utilities for address space ranges

[Ralph Campbell]

On 4/12/19 9:04 AM, Thomas Hellstrom wrote:
> Add two utilities to a) write-protect and b) clean all ptes pointing into
> a range of an address space

A period at the end, please.

> The utilities are intended to aid in tracking dirty pages (either
> driver-allocated system memory or pci device memory).
> The write-protect utility should be used in conjunction with
> page_mkwrite() and pfn_mkwrite() to trigger write page-faults on page
> accesses. Typically one would want to use this on sparse accesses into
> large memory regions. The clean utility should be used to utilize
> hardware dirtying functionality and avoid the overhead of page-faults,
> typically on large accesses into small memory regions.
> 
> The added file "apply_as_range.c" is initially listed as maintained by
> VMware under our DRM driver. If somebody would like it elsewhere,
> that's of course no problem.
> 
> Notable changes since RFC:
> - Added comments to help avoid the usage of these function for VMAs
>    it's not intended for. We also do advisory checks on the vm_flags and
>    warn on illegal usage.
> - Perform the pte modifications the same way softdirty does.
> - Add mmu_notifier range invalidation calls.
> - Add a config option so that this code is not unconditionally included.
> - Tell the mmu_gather code about pending tlb flushes.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>

Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>

> ---
>   MAINTAINERS         |   1 +
>   include/linux/mm.h  |   9 +-
>   mm/Kconfig          |   3 +
>   mm/Makefile         |   3 +-
>   mm/apply_as_range.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 309 insertions(+), 2 deletions(-)
>   create mode 100644 mm/apply_as_range.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 35e6357f9d30..bc243ffcb840 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4971,6 +4971,7 @@ T:	git git://people.freedesktop.org/~thomash/linux
>   S:	Supported
>   F:	drivers/gpu/drm/vmwgfx/
>   F:	include/uapi/drm/vmwgfx_drm.h
> +F:	mm/apply_as_range.c
>   
>   DRM DRIVERS
>   M:	David Airlie <airlied@linux.ie>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b7dd4ddd6efb..62f24dd0bfa0 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2642,7 +2642,14 @@ struct pfn_range_apply {
>   };
>   extern int apply_to_pfn_range(struct pfn_range_apply *closure,
>   			      unsigned long address, unsigned long size);
> -
> +unsigned long apply_as_wrprotect(struct address_space *mapping,
> +				 pgoff_t first_index, pgoff_t nr);
> +unsigned long apply_as_clean(struct address_space *mapping,
> +			     pgoff_t first_index, pgoff_t nr,
> +			     pgoff_t bitmap_pgoff,
> +			     unsigned long *bitmap,
> +			     pgoff_t *start,
> +			     pgoff_t *end);
>   #ifdef CONFIG_PAGE_POISONING
>   extern bool page_poisoning_enabled(void);
>   extern void kernel_poison_pages(struct page *page, int numpages, int enable);
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 25c71eb8a7db..80e41cdbb4ae 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -758,4 +758,7 @@ config GUP_BENCHMARK
>   config ARCH_HAS_PTE_SPECIAL
>   	bool
>   
> +config AS_DIRTY_HELPERS
> +        bool
> +
>   endmenu
> diff --git a/mm/Makefile b/mm/Makefile
> index d210cc9d6f80..b295717be856 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -39,7 +39,7 @@ obj-y			:= filemap.o mempool.o oom_kill.o fadvise.o \
>   			   mm_init.o mmu_context.o percpu.o slab_common.o \
>   			   compaction.o vmacache.o \
>   			   interval_tree.o list_lru.o workingset.o \
> -			   debug.o $(mmu-y)
> +			   debug.o apply_as_range.o $(mmu-y)
>   
>   obj-y += init-mm.o
>   obj-y += memblock.o
> @@ -99,3 +99,4 @@ obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o
>   obj-$(CONFIG_PERCPU_STATS) += percpu-stats.o
>   obj-$(CONFIG_HMM) += hmm.o
>   obj-$(CONFIG_MEMFD_CREATE) += memfd.o
> +obj-$(CONFIG_AS_DIRTY_HELPERS) += apply_as_range.o
> diff --git a/mm/apply_as_range.c b/mm/apply_as_range.c
> new file mode 100644
> index 000000000000..32d28619aec5
> --- /dev/null
> +++ b/mm/apply_as_range.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/mm.h>
> +#include <linux/mm_types.h>
> +#include <linux/hugetlb.h>
> +#include <linux/bitops.h>
> +#include <linux/mmu_notifier.h>
> +#include <asm/cacheflush.h>
> +#include <asm/tlbflush.h>
> +
> +/**
> + * struct apply_as - Closure structure for apply_as_range
> + * @base: struct pfn_range_apply we derive from
> + * @start: Address of first modified pte
> + * @end: Address of last modified pte + 1
> + * @total: Total number of modified ptes
> + * @vma: Pointer to the struct vm_area_struct we're currently operating on
> + */
> +struct apply_as {
> +	struct pfn_range_apply base;
> +	unsigned long start, end;

One variable defined per line, please.

> +	unsigned long total;
> +	const struct vm_area_struct *vma;
> +};
> +
> +/**
> + * apply_pt_wrprotect - Leaf pte callback to write-protect a pte
> + * @pte: Pointer to the pte
> + * @token: Page table token, see apply_to_pfn_range()
> + * @addr: The virtual page address
> + * @closure: Pointer to a struct pfn_range_apply embedded in a
> + * struct apply_as
> + *
> + * The function write-protects a pte and records the range in
> + * virtual address space of touched ptes for efficient range TLB flushes.
> + *
> + * Return: Always zero.
> + */
> +static int apply_pt_wrprotect(pte_t *pte, pgtable_t token,
> +			      unsigned long addr,
> +			      struct pfn_range_apply *closure)
> +{
> +	struct apply_as *aas = container_of(closure, typeof(*aas), base);
> +	pte_t ptent = *pte;
> +
> +	if (pte_write(ptent)) {
> +		ptent = ptep_modify_prot_start(closure->mm, addr, pte);
> +		ptent = pte_wrprotect(ptent);
> +		ptep_modify_prot_commit(closure->mm, addr, pte, ptent);
> +		aas->total++;
> +		aas->start = min(aas->start, addr);
> +		aas->end = max(aas->end, addr + PAGE_SIZE);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * struct apply_as_clean - Closure structure for apply_as_clean
> + * @base: struct apply_as we derive from
> + * @bitmap_pgoff: Address_space Page offset of the first bit in @bitmap
> + * @bitmap: Bitmap with one bit for each page offset in the address_space range
> + * covered.
> + * @start: Address_space page offset of first modified pte relative
> + * to @bitmap_pgoff
> + * @end: Address_space page offset of last modified pte relative
> + * to @bitmap_pgoff
> + */
> +struct apply_as_clean {
> +	struct apply_as base;
> +	pgoff_t bitmap_pgoff;
> +	unsigned long *bitmap;
> +	pgoff_t start, end;

One variable defined per line, please.

> +};
> +
> +/**
> + * apply_pt_clean - Leaf pte callback to clean a pte
> + * @pte: Pointer to the pte
> + * @token: Page table token, see apply_to_pfn_range()
> + * @addr: The virtual page address
> + * @closure: Pointer to a struct pfn_range_apply embedded in a
> + * struct apply_as_clean
> + *
> + * The function cleans a pte and records the range in
> + * virtual address space of touched ptes for efficient TLB flushes.
> + * It also records dirty ptes in a bitmap representing page offsets
> + * in the address_space, as well as the first and last of the bits
> + * touched.
> + *
> + * Return: Always zero.
> + */
> +static int apply_pt_clean(pte_t *pte, pgtable_t token,
> +			  unsigned long addr,
> +			  struct pfn_range_apply *closure)
> +{
> +	struct apply_as *aas = container_of(closure, typeof(*aas), base);
> +	struct apply_as_clean *clean = container_of(aas, typeof(*clean), base);
> +	pte_t ptent = *pte;
> +
> +	if (pte_dirty(ptent)) {
> +		pgoff_t pgoff = ((addr - aas->vma->vm_start) >> PAGE_SHIFT) +
> +			aas->vma->vm_pgoff - clean->bitmap_pgoff;
> +
> +		ptent = ptep_modify_prot_start(closure->mm, addr, pte);
> +		ptent = pte_mkclean(ptent);
> +		ptep_modify_prot_commit(closure->mm, addr, pte, ptent);
> +
> +		aas->total++;
> +		aas->start = min(aas->start, addr);
> +		aas->end = max(aas->end, addr + PAGE_SIZE);
> +
> +		__set_bit(pgoff, clean->bitmap);
> +		clean->start = min(clean->start, pgoff);
> +		clean->end = max(clean->end, pgoff + 1);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * apply_as_range - Apply a pte callback to all PTEs pointing into a range
> + * of an address_space.
> + * @mapping: Pointer to the struct address_space
> + * @aas: Closure structure
> + * @first_index: First page offset in the address_space
> + * @nr: Number of incremental page offsets to cover
> + *
> + * Return: Number of ptes touched. Note that this number might be larger
> + * than @nr if there are overlapping vmas
> + */
> +static unsigned long apply_as_range(struct address_space *mapping,
> +				    struct apply_as *aas,
> +				    pgoff_t first_index, pgoff_t nr)
> +{
> +	struct vm_area_struct *vma;
> +	pgoff_t vba, vea, cba, cea;
> +	unsigned long start_addr, end_addr;
> +	struct mmu_notifier_range range;
> +
> +	i_mmap_lock_read(mapping);
> +	vma_interval_tree_foreach(vma, &mapping->i_mmap, first_index,
> +				  first_index + nr - 1) {
> +		unsigned long vm_flags = READ_ONCE(vma->vm_flags);
> +
> +		/*
> +		 * We can only do advisory flag tests below, since we can't
> +		 * require the vm's mmap_sem to be held to protect the flags.
> +		 * Therefore, callers that strictly depend on specific mmap
> +		 * flags to remain constant throughout the operation must
> +		 * either ensure those flags are immutable for all relevant
> +		 * vmas or can't use this function. Fixing this properly would
> +		 * require the vma::vm_flags to be protected by a separate
> +		 * lock taken after the i_mmap_lock
> +		 */
> +
> +		/* Skip non-applicable VMAs */
> +		if ((vm_flags & (VM_SHARED | VM_WRITE)) !=
> +		    (VM_SHARED | VM_WRITE))
> +			continue;
> +
> +		/* Warn on and skip VMAs whose flags indicate illegal usage */
> +		if (WARN_ON((vm_flags & (VM_HUGETLB | VM_IO)) != VM_IO))
> +			continue;
> +
> +		/* Clip to the vma */
> +		vba = vma->vm_pgoff;
> +		vea = vba + vma_pages(vma);
> +		cba = first_index;
> +		cba = max(cba, vba);
> +		cea = first_index + nr;
> +		cea = min(cea, vea);
> +
> +		/* Translate to virtual address */
> +		start_addr = ((cba - vba) << PAGE_SHIFT) + vma->vm_start;
> +		end_addr = ((cea - vba) << PAGE_SHIFT) + vma->vm_start;
> +		if (start_addr >= end_addr)
> +			continue;
> +
> +		aas->base.mm = vma->vm_mm;
> +		aas->vma = vma;
> +		aas->start = end_addr;
> +		aas->end = start_addr;
> +
> +		mmu_notifier_range_init(&range, vma->vm_mm,
> +					start_addr, end_addr);
> +		mmu_notifier_invalidate_range_start(&range);
> +
> +		/* Needed when we only change protection? */
> +		flush_cache_range(vma, start_addr, end_addr);
> +
> +		/*
> +		 * We're not using tlb_gather_mmu() since typically
> +		 * only a small subrange of PTEs are affected.
> +		 */
> +		inc_tlb_flush_pending(vma->vm_mm);
> +
> +		/* Should not error since aas->base.alloc == 0 */
> +		WARN_ON(apply_to_pfn_range(&aas->base, start_addr,
> +					   end_addr - start_addr));
> +		if (aas->end > aas->start)
> +			flush_tlb_range(vma, aas->start, aas->end);
> +
> +		mmu_notifier_invalidate_range_end(&range);
> +		dec_tlb_flush_pending(vma->vm_mm);
> +	}
> +	i_mmap_unlock_read(mapping);
> +
> +	return aas->total;
> +}
> +
> +/**
> + * apply_as_wrprotect - Write-protect all ptes in an address_space range
> + * @mapping: The address_space we want to write protect
> + * @first_index: The first page offset in the range
> + * @nr: Number of incremental page offsets to cover
> + *
> + * WARNING: This function should only be used for address spaces that
> + * completely own the pages / memory the page table points to. Typically a
> + * device file.
> + *
> + * Return: The number of ptes actually write-protected. Note that
> + * already write-protected ptes are not counted.
> + */
> +unsigned long apply_as_wrprotect(struct address_space *mapping,
> +				 pgoff_t first_index, pgoff_t nr)
> +{
> +	struct apply_as aas = {
> +		.base = {
> +			.alloc = 0,
> +			.ptefn = apply_pt_wrprotect,
> +		},
> +		.total = 0,
> +	};
> +
> +	return apply_as_range(mapping, &aas, first_index, nr);
> +}
> +EXPORT_SYMBOL(apply_as_wrprotect);
> +
> +/**
> + * apply_as_clean - Clean all ptes in an address_space range
> + * @mapping: The address_space we want to clean
> + * @first_index: The first page offset in the range
> + * @nr: Number of incremental page offsets to cover
> + * @bitmap_pgoff: The page offset of the first bit in @bitmap
> + * @bitmap: Pointer to a bitmap of at least @nr bits. The bitmap needs to
> + * cover the whole range @first_index..@first_index + @nr.
> + * @start: Pointer to number of the first set bit in @bitmap.
> + * is modified as new bits are set by the function.
> + * @end: Pointer to the number of the last set bit in @bitmap.
> + * none set. The value is modified as new bets are set by the function.

s/bets/bits/

> + *
> + * Note: When this function returns there is no guarantee that a CPU has
> + * not already dirtied new ptes. However it will not clean any ptes not
> + * reported in the bitmap.
> + *
> + * If a caller needs to make sure all dirty ptes are picked up and none
> + * additional are added, it first needs to write-protect the address-space
> + * range and make sure new writers are blocked in page_mkwrite() or
> + * pfn_mkwrite(). And then after a TLB flush following the write-protection
> + * pick upp all dirty bits.

s/upp/up/

> + *
> + * WARNING: This function should only be used for address spaces that
> + * completely own the pages / memory the page table points to. Typically a
> + * device file.
> + *
> + * Return: The number of dirty ptes actually cleaned.
> + */
> +unsigned long apply_as_clean(struct address_space *mapping,
> +			     pgoff_t first_index, pgoff_t nr,
> +			     pgoff_t bitmap_pgoff,
> +			     unsigned long *bitmap,
> +			     pgoff_t *start,
> +			     pgoff_t *end)
> +{
> +	bool none_set = (*start >= *end);
> +	struct apply_as_clean clean = {
> +		.base = {
> +			.base = {
> +				.alloc = 0,
> +				.ptefn = apply_pt_clean,
> +			},
> +			.total = 0,
> +		},
> +		.bitmap_pgoff = bitmap_pgoff,
> +		.bitmap = bitmap,
> +		.start = none_set ? nr : *start,
> +		.end = none_set ? 0 : *end,
> +	};
> +	unsigned long ret = apply_as_range(mapping, &clean.base, first_index,
> +					   nr);
> +
> +	*start = clean.start;
> +	*end = clean.end;
> +	return ret;
> +}
> +EXPORT_SYMBOL(apply_as_clean);
> 

Re: [PATCH 2/9] mm: Add an apply_to_pfn_range interface

[Jerome Glisse]

On Fri, Apr 12, 2019 at 04:04:18PM +0000, Thomas Hellstrom wrote:
> This is basically apply_to_page_range with added functionality:
> Allocating missing parts of the page table becomes optional, which
> means that the function can be guaranteed not to error if allocation
> is disabled. Also passing of the closure struct and callback function
> becomes different and more in line with how things are done elsewhere.
> 
> Finally we keep apply_to_page_range as a wrapper around apply_to_pfn_range
> 
> The reason for not using the page-walk code is that we want to perform
> the page-walk on vmas pointing to an address space without requiring the
> mmap_sem to be held rather thand on vmas belonging to a process with the
> mmap_sem held.
> 
> Notable changes since RFC:
> Don't export apply_to_pfn range.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  include/linux/mm.h |  10 ++++
>  mm/memory.c        | 130 ++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 108 insertions(+), 32 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 80bb6408fe73..b7dd4ddd6efb 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2632,6 +2632,16 @@ typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
>  extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
>  			       unsigned long size, pte_fn_t fn, void *data);
>  
> +struct pfn_range_apply;
> +typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
> +			 struct pfn_range_apply *closure);
> +struct pfn_range_apply {
> +	struct mm_struct *mm;
> +	pter_fn_t ptefn;
> +	unsigned int alloc;
> +};
> +extern int apply_to_pfn_range(struct pfn_range_apply *closure,
> +			      unsigned long address, unsigned long size);
>  
>  #ifdef CONFIG_PAGE_POISONING
>  extern bool page_poisoning_enabled(void);
> diff --git a/mm/memory.c b/mm/memory.c
> index a95b4a3b1ae2..60d67158964f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1938,18 +1938,17 @@ int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long
>  }
>  EXPORT_SYMBOL(vm_iomap_memory);
>  
> -static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pte_range(struct pfn_range_apply *closure, pmd_t *pmd,
> +			      unsigned long addr, unsigned long end)
>  {
>  	pte_t *pte;
>  	int err;
>  	pgtable_t token;
>  	spinlock_t *uninitialized_var(ptl);
>  
> -	pte = (mm == &init_mm) ?
> +	pte = (closure->mm == &init_mm) ?
>  		pte_alloc_kernel(pmd, addr) :
> -		pte_alloc_map_lock(mm, pmd, addr, &ptl);
> +		pte_alloc_map_lock(closure->mm, pmd, addr, &ptl);
>  	if (!pte)
>  		return -ENOMEM;
>  
> @@ -1960,86 +1959,107 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
>  	token = pmd_pgtable(*pmd);
>  
>  	do {
> -		err = fn(pte++, token, addr, data);
> +		err = closure->ptefn(pte++, token, addr, closure);
>  		if (err)
>  			break;
>  	} while (addr += PAGE_SIZE, addr != end);
>  
>  	arch_leave_lazy_mmu_mode();
>  
> -	if (mm != &init_mm)
> +	if (closure->mm != &init_mm)
>  		pte_unmap_unlock(pte-1, ptl);
>  	return err;
>  }
>  
> -static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pmd_range(struct pfn_range_apply *closure, pud_t *pud,
> +			      unsigned long addr, unsigned long end)
>  {
>  	pmd_t *pmd;
>  	unsigned long next;
> -	int err;
> +	int err = 0;
>  
>  	BUG_ON(pud_huge(*pud));
>  
> -	pmd = pmd_alloc(mm, pud, addr);
> +	pmd = pmd_alloc(closure->mm, pud, addr);
>  	if (!pmd)
>  		return -ENOMEM;
> +
>  	do {
>  		next = pmd_addr_end(addr, end);
> -		err = apply_to_pte_range(mm, pmd, addr, next, fn, data);
> +		if (!closure->alloc && pmd_none_or_clear_bad(pmd))
> +			continue;
> +		err = apply_to_pte_range(closure, pmd, addr, next);
>  		if (err)
>  			break;
>  	} while (pmd++, addr = next, addr != end);
>  	return err;
>  }
>  
> -static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pud_range(struct pfn_range_apply *closure, p4d_t *p4d,
> +			      unsigned long addr, unsigned long end)
>  {
>  	pud_t *pud;
>  	unsigned long next;
> -	int err;
> +	int err = 0;
>  
> -	pud = pud_alloc(mm, p4d, addr);
> +	pud = pud_alloc(closure->mm, p4d, addr);
>  	if (!pud)
>  		return -ENOMEM;
> +
>  	do {
>  		next = pud_addr_end(addr, end);
> -		err = apply_to_pmd_range(mm, pud, addr, next, fn, data);
> +		if (!closure->alloc && pud_none_or_clear_bad(pud))
> +			continue;
> +		err = apply_to_pmd_range(closure, pud, addr, next);
>  		if (err)
>  			break;
>  	} while (pud++, addr = next, addr != end);
>  	return err;
>  }
>  
> -static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_p4d_range(struct pfn_range_apply *closure, pgd_t *pgd,
> +			      unsigned long addr, unsigned long end)
>  {
>  	p4d_t *p4d;
>  	unsigned long next;
> -	int err;
> +	int err = 0;
>  
> -	p4d = p4d_alloc(mm, pgd, addr);
> +	p4d = p4d_alloc(closure->mm, pgd, addr);
>  	if (!p4d)
>  		return -ENOMEM;
> +
>  	do {
>  		next = p4d_addr_end(addr, end);
> -		err = apply_to_pud_range(mm, p4d, addr, next, fn, data);
> +		if (!closure->alloc && p4d_none_or_clear_bad(p4d))
> +			continue;
> +		err = apply_to_pud_range(closure, p4d, addr, next);
>  		if (err)
>  			break;
>  	} while (p4d++, addr = next, addr != end);
>  	return err;
>  }
>  
> -/*
> - * Scan a region of virtual memory, filling in page tables as necessary
> - * and calling a provided function on each leaf page table.
> +/**
> + * apply_to_pfn_range - Scan a region of virtual memory, calling a provided
> + * function on each leaf page table entry
> + * @closure: Details about how to scan and what function to apply
> + * @addr: Start virtual address
> + * @size: Size of the region
> + *
> + * If @closure->alloc is set to 1, the function will fill in the page table
> + * as necessary. Otherwise it will skip non-present parts.
> + * Note: The caller must ensure that the range does not contain huge pages.
> + * The caller must also assure that the proper mmu_notifier functions are
> + * called. Either in the pte leaf function or before and after the call to
> + * apply_to_pfn_range.

This is wrong there should be a big FAT warning that this can only be use
against mmap of device file. The page table walking above is broken for
various thing you might find in any other vma like THP, device pte, hugetlbfs,
...

Also the mmu notifier can not be call from the pfn callback as that callback
happens under page table lock (the change_pte notifier callback is useless
and not enough). So it _must_ happen around the call to apply_to_pfn_range

apply_to_page_range was really not meant to be use in that way ... it was not
for regular vma.

Using this function for anything else is dangerous and having its uses spread
more increase that risk. So there must be a big FAT warning saying that you
should not use this lightly and that it should only be only on mmap of device
file.


> + *
> + * Returns: Zero on success. If the provided function returns a non-zero status,
> + * the page table walk will terminate and that status will be returned.
> + * If @closure->alloc is set to 1, then this function may also return memory
> + * allocation errors arising from allocating page table memory.
>   */
> -int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> -			unsigned long size, pte_fn_t fn, void *data)
> +int apply_to_pfn_range(struct pfn_range_apply *closure,
> +		       unsigned long addr, unsigned long size)
>  {
>  	pgd_t *pgd;
>  	unsigned long next;
> @@ -2049,16 +2069,62 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
>  	if (WARN_ON(addr >= end))
>  		return -EINVAL;
>  
> -	pgd = pgd_offset(mm, addr);
> +	pgd = pgd_offset(closure->mm, addr);
>  	do {
>  		next = pgd_addr_end(addr, end);
> -		err = apply_to_p4d_range(mm, pgd, addr, next, fn, data);
> +		if (!closure->alloc && pgd_none_or_clear_bad(pgd))
> +			continue;
> +		err = apply_to_p4d_range(closure, pgd, addr, next);
>  		if (err)
>  			break;
>  	} while (pgd++, addr = next, addr != end);
>  
>  	return err;
>  }
> +
> +/**
> + * struct page_range_apply - Closure structure for apply_to_page_range()
> + * @pter: The base closure structure we derive from
> + * @fn: The leaf pte function to call
> + * @data: The leaf pte function closure
> + */
> +struct page_range_apply {
> +	struct pfn_range_apply pter;
> +	pte_fn_t fn;
> +	void *data;
> +};
> +
> +/*
> + * Callback wrapper to enable use of apply_to_pfn_range for
> + * the apply_to_page_range interface
> + */
> +static int apply_to_page_range_wrapper(pte_t *pte, pgtable_t token,
> +				       unsigned long addr,
> +				       struct pfn_range_apply *pter)
> +{
> +	struct page_range_apply *pra =
> +		container_of(pter, typeof(*pra), pter);
> +
> +	return pra->fn(pte, token, addr, pra->data);
> +}
> +
> +/*
> + * Scan a region of virtual memory, filling in page tables as necessary
> + * and calling a provided function on each leaf page table.
> + */

It would be good to improve that comment too and make it a warning of
DO NOT USE ! THIS IS NOT SAFE ON REGULAR VMA !

> +int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> +			unsigned long size, pte_fn_t fn, void *data)
> +{
> +	struct page_range_apply pra = {
> +		.pter = {.mm = mm,
> +			 .alloc = 1,
> +			 .ptefn = apply_to_page_range_wrapper },
> +		.fn = fn,
> +		.data = data
> +	};
> +
> +	return apply_to_pfn_range(&pra.pter, addr, size);
> +}
>  EXPORT_SYMBOL_GPL(apply_to_page_range);
>  
>  /*
> -- 
> 2.20.1
> 

Re: [PATCH 2/9] mm: Add an apply_to_pfn_range interface

[Thomas Hellstrom]

Hi, Jérôme

On Fri, 2019-04-12 at 17:07 -0400, Jerome Glisse wrote:
> On Fri, Apr 12, 2019 at 04:04:18PM +0000, Thomas Hellstrom wrote:
> > This is basically apply_to_page_range with added functionality:
> > Allocating missing parts of the page table becomes optional, which
> > means that the function can be guaranteed not to error if
> > allocation
> > is disabled. Also passing of the closure struct and callback
> > function
> > becomes different and more in line with how things are done
> > elsewhere.
> > 
> > Finally we keep apply_to_page_range as a wrapper around
> > apply_to_pfn_range
> > 
> > The reason for not using the page-walk code is that we want to
> > perform
> > the page-walk on vmas pointing to an address space without
> > requiring the
> > mmap_sem to be held rather thand on vmas belonging to a process
> > with the
> > mmap_sem held.
> > 
> > Notable changes since RFC:
> > Don't export apply_to_pfn range.
> > 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rik van Riel <riel@surriel.com>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Huang Ying <ying.huang@intel.com>
> > Cc: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > ---
> >  include/linux/mm.h |  10 ++++
> >  mm/memory.c        | 130 ++++++++++++++++++++++++++++++++++-------
> > ----
> >  2 files changed, 108 insertions(+), 32 deletions(-)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 80bb6408fe73..b7dd4ddd6efb 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2632,6 +2632,16 @@ typedef int (*pte_fn_t)(pte_t *pte,
> > pgtable_t token, unsigned long addr,
> >  extern int apply_to_page_range(struct mm_struct *mm, unsigned long
> > address,
> >  			       unsigned long size, pte_fn_t fn, void
> > *data);
> >  
> > +struct pfn_range_apply;
> > +typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned
> > long addr,
> > +			 struct pfn_range_apply *closure);
> > +struct pfn_range_apply {
> > +	struct mm_struct *mm;
> > +	pter_fn_t ptefn;
> > +	unsigned int alloc;
> > +};
> > +extern int apply_to_pfn_range(struct pfn_range_apply *closure,
> > +			      unsigned long address, unsigned long
> > size);
> >  
> >  #ifdef CONFIG_PAGE_POISONING
> >  extern bool page_poisoning_enabled(void);
> > diff --git a/mm/memory.c b/mm/memory.c
> > index a95b4a3b1ae2..60d67158964f 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1938,18 +1938,17 @@ int vm_iomap_memory(struct vm_area_struct
> > *vma, phys_addr_t start, unsigned long
> >  }
> >  EXPORT_SYMBOL(vm_iomap_memory);
> >  
> > -static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
> > -				     unsigned long addr, unsigned long
> > end,
> > -				     pte_fn_t fn, void *data)
> > +static int apply_to_pte_range(struct pfn_range_apply *closure,
> > pmd_t *pmd,
> > +			      unsigned long addr, unsigned long end)
> >  {
> >  	pte_t *pte;
> >  	int err;
> >  	pgtable_t token;
> >  	spinlock_t *uninitialized_var(ptl);
> >  
> > -	pte = (mm == &init_mm) ?
> > +	pte = (closure->mm == &init_mm) ?
> >  		pte_alloc_kernel(pmd, addr) :
> > -		pte_alloc_map_lock(mm, pmd, addr, &ptl);
> > +		pte_alloc_map_lock(closure->mm, pmd, addr, &ptl);
> >  	if (!pte)
> >  		return -ENOMEM;
> >  
> > @@ -1960,86 +1959,107 @@ static int apply_to_pte_range(struct
> > mm_struct *mm, pmd_t *pmd,
> >  	token = pmd_pgtable(*pmd);
> >  
> >  	do {
> > -		err = fn(pte++, token, addr, data);
> > +		err = closure->ptefn(pte++, token, addr, closure);
> >  		if (err)
> >  			break;
> >  	} while (addr += PAGE_SIZE, addr != end);
> >  
> >  	arch_leave_lazy_mmu_mode();
> >  
> > -	if (mm != &init_mm)
> > +	if (closure->mm != &init_mm)
> >  		pte_unmap_unlock(pte-1, ptl);
> >  	return err;
> >  }
> >  
> > -static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
> > -				     unsigned long addr, unsigned long
> > end,
> > -				     pte_fn_t fn, void *data)
> > +static int apply_to_pmd_range(struct pfn_range_apply *closure,
> > pud_t *pud,
> > +			      unsigned long addr, unsigned long end)
> >  {
> >  	pmd_t *pmd;
> >  	unsigned long next;
> > -	int err;
> > +	int err = 0;
> >  
> >  	BUG_ON(pud_huge(*pud));
> >  
> > -	pmd = pmd_alloc(mm, pud, addr);
> > +	pmd = pmd_alloc(closure->mm, pud, addr);
> >  	if (!pmd)
> >  		return -ENOMEM;
> > +
> >  	do {
> >  		next = pmd_addr_end(addr, end);
> > -		err = apply_to_pte_range(mm, pmd, addr, next, fn,
> > data);
> > +		if (!closure->alloc && pmd_none_or_clear_bad(pmd))
> > +			continue;
> > +		err = apply_to_pte_range(closure, pmd, addr, next);
> >  		if (err)
> >  			break;
> >  	} while (pmd++, addr = next, addr != end);
> >  	return err;
> >  }
> >  
> > -static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
> > -				     unsigned long addr, unsigned long
> > end,
> > -				     pte_fn_t fn, void *data)
> > +static int apply_to_pud_range(struct pfn_range_apply *closure,
> > p4d_t *p4d,
> > +			      unsigned long addr, unsigned long end)
> >  {
> >  	pud_t *pud;
> >  	unsigned long next;
> > -	int err;
> > +	int err = 0;
> >  
> > -	pud = pud_alloc(mm, p4d, addr);
> > +	pud = pud_alloc(closure->mm, p4d, addr);
> >  	if (!pud)
> >  		return -ENOMEM;
> > +
> >  	do {
> >  		next = pud_addr_end(addr, end);
> > -		err = apply_to_pmd_range(mm, pud, addr, next, fn,
> > data);
> > +		if (!closure->alloc && pud_none_or_clear_bad(pud))
> > +			continue;
> > +		err = apply_to_pmd_range(closure, pud, addr, next);
> >  		if (err)
> >  			break;
> >  	} while (pud++, addr = next, addr != end);
> >  	return err;
> >  }
> >  
> > -static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
> > -				     unsigned long addr, unsigned long
> > end,
> > -				     pte_fn_t fn, void *data)
> > +static int apply_to_p4d_range(struct pfn_range_apply *closure,
> > pgd_t *pgd,
> > +			      unsigned long addr, unsigned long end)
> >  {
> >  	p4d_t *p4d;
> >  	unsigned long next;
> > -	int err;
> > +	int err = 0;
> >  
> > -	p4d = p4d_alloc(mm, pgd, addr);
> > +	p4d = p4d_alloc(closure->mm, pgd, addr);
> >  	if (!p4d)
> >  		return -ENOMEM;
> > +
> >  	do {
> >  		next = p4d_addr_end(addr, end);
> > -		err = apply_to_pud_range(mm, p4d, addr, next, fn,
> > data);
> > +		if (!closure->alloc && p4d_none_or_clear_bad(p4d))
> > +			continue;
> > +		err = apply_to_pud_range(closure, p4d, addr, next);
> >  		if (err)
> >  			break;
> >  	} while (p4d++, addr = next, addr != end);
> >  	return err;
> >  }
> >  
> > -/*
> > - * Scan a region of virtual memory, filling in page tables as
> > necessary
> > - * and calling a provided function on each leaf page table.
> > +/**
> > + * apply_to_pfn_range - Scan a region of virtual memory, calling a
> > provided
> > + * function on each leaf page table entry
> > + * @closure: Details about how to scan and what function to apply
> > + * @addr: Start virtual address
> > + * @size: Size of the region
> > + *
> > + * If @closure->alloc is set to 1, the function will fill in the
> > page table
> > + * as necessary. Otherwise it will skip non-present parts.
> > + * Note: The caller must ensure that the range does not contain
> > huge pages.
> > + * The caller must also assure that the proper mmu_notifier
> > functions are
> > + * called. Either in the pte leaf function or before and after the
> > call to
> > + * apply_to_pfn_range.
> 
> This is wrong there should be a big FAT warning that this can only be
> use
> against mmap of device file. The page table walking above is broken
> for
> various thing you might find in any other vma like THP, device pte,
> hugetlbfs,

I was figuring since we didn't export the function anymore, the warning
and checks could be left to its users, assuming that any other future
usage of this function would require mm people audit anyway. But I can
of course add that warning also to this function if you still want
that?

> ...
> 
> Also the mmu notifier can not be call from the pfn callback as that
> callback
> happens under page table lock (the change_pte notifier callback is
> useless
> and not enough). So it _must_ happen around the call to
> apply_to_pfn_range


In the comments I was having in mind usage of, for example
ptep_clear_flush_notify(). But you're the mmu_notifier expert here. Are
you saying that function by itself would not be sufficient?
In that case, should I just scratch the text mentioning the pte leaf
function?


> 
> apply_to_page_range was really not meant to be use in that way ... it
> was not
> for regular vma.
> 
> Using this function for anything else is dangerous and having its
> uses spread
> more increase that risk. So there must be a big FAT warning saying
> that you
> should not use this lightly and that it should only be only on mmap
> of device
> file.

Understood.

/Thomas


> 
> 
> > + *
> > + * Returns: Zero on success. If the provided function returns a
> > non-zero status,
> > + * the page table walk will terminate and that status will be
> > returned.
> > + * If @closure->alloc is set to 1, then this function may also
> > return memory
> > + * allocation errors arising from allocating page table memory.
> >   */
> > -int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> > -			unsigned long size, pte_fn_t fn, void *data)
> > +int apply_to_pfn_range(struct pfn_range_apply *closure,
> > +		       unsigned long addr, unsigned long size)
> >  {
> >  	pgd_t *pgd;
> >  	unsigned long next;
> > @@ -2049,16 +2069,62 @@ int apply_to_page_range(struct mm_struct
> > *mm, unsigned long addr,
> >  	if (WARN_ON(addr >= end))
> >  		return -EINVAL;
> >  
> > -	pgd = pgd_offset(mm, addr);
> > +	pgd = pgd_offset(closure->mm, addr);
> >  	do {
> >  		next = pgd_addr_end(addr, end);
> > -		err = apply_to_p4d_range(mm, pgd, addr, next, fn,
> > data);
> > +		if (!closure->alloc && pgd_none_or_clear_bad(pgd))
> > +			continue;
> > +		err = apply_to_p4d_range(closure, pgd, addr, next);
> >  		if (err)
> >  			break;
> >  	} while (pgd++, addr = next, addr != end);
> >  
> >  	return err;
> >  }
> > +
> > +/**
> > + * struct page_range_apply - Closure structure for
> > apply_to_page_range()
> > + * @pter: The base closure structure we derive from
> > + * @fn: The leaf pte function to call
> > + * @data: The leaf pte function closure
> > + */
> > +struct page_range_apply {
> > +	struct pfn_range_apply pter;
> > +	pte_fn_t fn;
> > +	void *data;
> > +};
> > +
> > +/*
> > + * Callback wrapper to enable use of apply_to_pfn_range for
> > + * the apply_to_page_range interface
> > + */
> > +static int apply_to_page_range_wrapper(pte_t *pte, pgtable_t
> > token,
> > +				       unsigned long addr,
> > +				       struct pfn_range_apply *pter)
> > +{
> > +	struct page_range_apply *pra =
> > +		container_of(pter, typeof(*pra), pter);
> > +
> > +	return pra->fn(pte, token, addr, pra->data);
> > +}
> > +
> > +/*
> > + * Scan a region of virtual memory, filling in page tables as
> > necessary
> > + * and calling a provided function on each leaf page table.
> > + */
> 
> It would be good to improve that comment too and make it a warning of
> DO NOT USE ! THIS IS NOT SAFE ON REGULAR VMA !
> 
> > +int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> > +			unsigned long size, pte_fn_t fn, void *data)
> > +{
> > +	struct page_range_apply pra = {
> > +		.pter = {.mm = mm,
> > +			 .alloc = 1,
> > +			 .ptefn = apply_to_page_range_wrapper },
> > +		.fn = fn,
> > +		.data = data
> > +	};
> > +
> > +	return apply_to_pfn_range(&pra.pter, addr, size);
> > +}
> >  EXPORT_SYMBOL_GPL(apply_to_page_range);
> >  
> >  /*
> > -- 
> > 2.20.1
> > 

Re: [PATCH 3/9] mm: Add write-protect and clean utilities for address space ranges

[Thomas Hellstrom]

Hi, Ralph,

On Fri, 2019-04-12 at 11:52 -0700, Ralph Campbell wrote:
> On 4/12/19 9:04 AM, Thomas Hellstrom wrote:
> > Add two utilities to a) write-protect and b) clean all ptes
> > pointing into
> > a range of an address space
> 
> A period at the end, please.
> 
> > The utilities are intended to aid in tracking dirty pages (either
> > driver-allocated system memory or pci device memory).
> > The write-protect utility should be used in conjunction with
> > page_mkwrite() and pfn_mkwrite() to trigger write page-faults on
> > page
> > accesses. Typically one would want to use this on sparse accesses
> > into
> > large memory regions. The clean utility should be used to utilize
> > hardware dirtying functionality and avoid the overhead of page-
> > faults,
> > typically on large accesses into small memory regions.
> > 
> > The added file "apply_as_range.c" is initially listed as maintained
> > by
> > VMware under our DRM driver. If somebody would like it elsewhere,
> > that's of course no problem.
> > 
> > Notable changes since RFC:
> > - Added comments to help avoid the usage of these function for VMAs
> >    it's not intended for. We also do advisory checks on the
> > vm_flags and
> >    warn on illegal usage.
> > - Perform the pte modifications the same way softdirty does.
> > - Add mmu_notifier range invalidation calls.
> > - Add a config option so that this code is not unconditionally
> > included.
> > - Tell the mmu_gather code about pending tlb flushes.
> > 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rik van Riel <riel@surriel.com>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Huang Ying <ying.huang@intel.com>
> > Cc: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> 
> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>

Thanks for reviewing the patches. I'll incorporate your suggestions in
v2.

Re: [PATCH 1/9] mm: Allow the [page|pfn]_mkwrite callbacks to drop the mmap_sem

[Souptick Joarder]

On Fri, Apr 12, 2019 at 9:34 PM Thomas Hellstrom <thellstrom@vmware.com> wrote:
>
> Driver fault callbacks are allowed to drop the mmap_sem when expecting
> long hardware waits to avoid blocking other mm users. Allow the mkwrite
> callbacks to do the same by returning early on VM_FAULT_RETRY.
>
> In particular we want to be able to drop the mmap_sem when waiting for
> a reservation object lock on a GPU buffer object. These locks may be
> held while waiting for the GPU.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
>
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  mm/memory.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index e11ca9dd823f..a95b4a3b1ae2 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2144,7 +2144,7 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
>         ret = vmf->vma->vm_ops->page_mkwrite(vmf);
>         /* Restore original flags so that caller is not surprised */
>         vmf->flags = old_flags;
> -       if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
> +       if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_RETRY | VM_FAULT_NOPAGE)))

With this patch there will multiple instances of (VM_FAULT_ERROR |
VM_FAULT_RETRY | VM_FAULT_NOPAGE)
in mm/memory.c. Does it make sense to wrap it in a macro and use it ?

>                 return ret;
>         if (unlikely(!(ret & VM_FAULT_LOCKED))) {
>                 lock_page(page);
> @@ -2419,7 +2419,7 @@ static vm_fault_t wp_pfn_shared(struct vm_fault *vmf)
>                 pte_unmap_unlock(vmf->pte, vmf->ptl);
>                 vmf->flags |= FAULT_FLAG_MKWRITE;
>                 ret = vma->vm_ops->pfn_mkwrite(vmf);
> -               if (ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE))
> +               if (ret & (VM_FAULT_ERROR | VM_FAULT_RETRY | VM_FAULT_NOPAGE))
>                         return ret;
>                 return finish_mkwrite_fault(vmf);
>         }
> @@ -2440,7 +2440,8 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf)
>                 pte_unmap_unlock(vmf->pte, vmf->ptl);
>                 tmp = do_page_mkwrite(vmf);
>                 if (unlikely(!tmp || (tmp &
> -                                     (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) {
> +                                     (VM_FAULT_ERROR | VM_FAULT_RETRY |
> +                                      VM_FAULT_NOPAGE)))) {
>                         put_page(vmf->page);
>                         return tmp;
>                 }
> @@ -3494,7 +3495,8 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
>                 unlock_page(vmf->page);
>                 tmp = do_page_mkwrite(vmf);
>                 if (unlikely(!tmp ||
> -                               (tmp & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))) {
> +                               (tmp & (VM_FAULT_ERROR | VM_FAULT_RETRY |
> +                                       VM_FAULT_NOPAGE)))) {
>                         put_page(vmf->page);
>                         return tmp;
>                 }
> --
> 2.20.1
>

Re: [PATCH 2/9] mm: Add an apply_to_pfn_range interface

[Jerome Glisse]

On Sat, Apr 13, 2019 at 08:34:02AM +0000, Thomas Hellstrom wrote:
> Hi, Jérôme
> 
> On Fri, 2019-04-12 at 17:07 -0400, Jerome Glisse wrote:
> > On Fri, Apr 12, 2019 at 04:04:18PM +0000, Thomas Hellstrom wrote:
> > > This is basically apply_to_page_range with added functionality:
> > > Allocating missing parts of the page table becomes optional, which
> > > means that the function can be guaranteed not to error if
> > > allocation
> > > is disabled. Also passing of the closure struct and callback
> > > function
> > > becomes different and more in line with how things are done
> > > elsewhere.
> > > 
> > > Finally we keep apply_to_page_range as a wrapper around
> > > apply_to_pfn_range
> > > 
> > > The reason for not using the page-walk code is that we want to
> > > perform
> > > the page-walk on vmas pointing to an address space without
> > > requiring the
> > > mmap_sem to be held rather thand on vmas belonging to a process
> > > with the
> > > mmap_sem held.
> > > 
> > > Notable changes since RFC:
> > > Don't export apply_to_pfn range.
> > > 
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Matthew Wilcox <willy@infradead.org>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Rik van Riel <riel@surriel.com>
> > > Cc: Minchan Kim <minchan@kernel.org>
> > > Cc: Michal Hocko <mhocko@suse.com>
> > > Cc: Huang Ying <ying.huang@intel.com>
> > > Cc: Souptick Joarder <jrdr.linux@gmail.com>
> > > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > > Cc: linux-mm@kvack.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > > ---
> > >  include/linux/mm.h |  10 ++++
> > >  mm/memory.c        | 130 ++++++++++++++++++++++++++++++++++-------
> > > ----
> > >  2 files changed, 108 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 80bb6408fe73..b7dd4ddd6efb 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -2632,6 +2632,16 @@ typedef int (*pte_fn_t)(pte_t *pte,
> > > pgtable_t token, unsigned long addr,
> > >  extern int apply_to_page_range(struct mm_struct *mm, unsigned long
> > > address,
> > >  			       unsigned long size, pte_fn_t fn, void
> > > *data);
> > >  
> > > +struct pfn_range_apply;
> > > +typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned
> > > long addr,
> > > +			 struct pfn_range_apply *closure);
> > > +struct pfn_range_apply {
> > > +	struct mm_struct *mm;
> > > +	pter_fn_t ptefn;
> > > +	unsigned int alloc;
> > > +};
> > > +extern int apply_to_pfn_range(struct pfn_range_apply *closure,
> > > +			      unsigned long address, unsigned long
> > > size);
> > >  
> > >  #ifdef CONFIG_PAGE_POISONING
> > >  extern bool page_poisoning_enabled(void);
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index a95b4a3b1ae2..60d67158964f 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -1938,18 +1938,17 @@ int vm_iomap_memory(struct vm_area_struct
> > > *vma, phys_addr_t start, unsigned long
> > >  }
> > >  EXPORT_SYMBOL(vm_iomap_memory);
> > >  
> > > -static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
> > > -				     unsigned long addr, unsigned long
> > > end,
> > > -				     pte_fn_t fn, void *data)
> > > +static int apply_to_pte_range(struct pfn_range_apply *closure,
> > > pmd_t *pmd,
> > > +			      unsigned long addr, unsigned long end)
> > >  {
> > >  	pte_t *pte;
> > >  	int err;
> > >  	pgtable_t token;
> > >  	spinlock_t *uninitialized_var(ptl);
> > >  
> > > -	pte = (mm == &init_mm) ?
> > > +	pte = (closure->mm == &init_mm) ?
> > >  		pte_alloc_kernel(pmd, addr) :
> > > -		pte_alloc_map_lock(mm, pmd, addr, &ptl);
> > > +		pte_alloc_map_lock(closure->mm, pmd, addr, &ptl);
> > >  	if (!pte)
> > >  		return -ENOMEM;
> > >  
> > > @@ -1960,86 +1959,107 @@ static int apply_to_pte_range(struct
> > > mm_struct *mm, pmd_t *pmd,
> > >  	token = pmd_pgtable(*pmd);
> > >  
> > >  	do {
> > > -		err = fn(pte++, token, addr, data);
> > > +		err = closure->ptefn(pte++, token, addr, closure);
> > >  		if (err)
> > >  			break;
> > >  	} while (addr += PAGE_SIZE, addr != end);
> > >  
> > >  	arch_leave_lazy_mmu_mode();
> > >  
> > > -	if (mm != &init_mm)
> > > +	if (closure->mm != &init_mm)
> > >  		pte_unmap_unlock(pte-1, ptl);
> > >  	return err;
> > >  }
> > >  
> > > -static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
> > > -				     unsigned long addr, unsigned long
> > > end,
> > > -				     pte_fn_t fn, void *data)
> > > +static int apply_to_pmd_range(struct pfn_range_apply *closure,
> > > pud_t *pud,
> > > +			      unsigned long addr, unsigned long end)
> > >  {
> > >  	pmd_t *pmd;
> > >  	unsigned long next;
> > > -	int err;
> > > +	int err = 0;
> > >  
> > >  	BUG_ON(pud_huge(*pud));
> > >  
> > > -	pmd = pmd_alloc(mm, pud, addr);
> > > +	pmd = pmd_alloc(closure->mm, pud, addr);
> > >  	if (!pmd)
> > >  		return -ENOMEM;
> > > +
> > >  	do {
> > >  		next = pmd_addr_end(addr, end);
> > > -		err = apply_to_pte_range(mm, pmd, addr, next, fn,
> > > data);
> > > +		if (!closure->alloc && pmd_none_or_clear_bad(pmd))
> > > +			continue;
> > > +		err = apply_to_pte_range(closure, pmd, addr, next);
> > >  		if (err)
> > >  			break;
> > >  	} while (pmd++, addr = next, addr != end);
> > >  	return err;
> > >  }
> > >  
> > > -static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
> > > -				     unsigned long addr, unsigned long
> > > end,
> > > -				     pte_fn_t fn, void *data)
> > > +static int apply_to_pud_range(struct pfn_range_apply *closure,
> > > p4d_t *p4d,
> > > +			      unsigned long addr, unsigned long end)
> > >  {
> > >  	pud_t *pud;
> > >  	unsigned long next;
> > > -	int err;
> > > +	int err = 0;
> > >  
> > > -	pud = pud_alloc(mm, p4d, addr);
> > > +	pud = pud_alloc(closure->mm, p4d, addr);
> > >  	if (!pud)
> > >  		return -ENOMEM;
> > > +
> > >  	do {
> > >  		next = pud_addr_end(addr, end);
> > > -		err = apply_to_pmd_range(mm, pud, addr, next, fn,
> > > data);
> > > +		if (!closure->alloc && pud_none_or_clear_bad(pud))
> > > +			continue;
> > > +		err = apply_to_pmd_range(closure, pud, addr, next);
> > >  		if (err)
> > >  			break;
> > >  	} while (pud++, addr = next, addr != end);
> > >  	return err;
> > >  }
> > >  
> > > -static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
> > > -				     unsigned long addr, unsigned long
> > > end,
> > > -				     pte_fn_t fn, void *data)
> > > +static int apply_to_p4d_range(struct pfn_range_apply *closure,
> > > pgd_t *pgd,
> > > +			      unsigned long addr, unsigned long end)
> > >  {
> > >  	p4d_t *p4d;
> > >  	unsigned long next;
> > > -	int err;
> > > +	int err = 0;
> > >  
> > > -	p4d = p4d_alloc(mm, pgd, addr);
> > > +	p4d = p4d_alloc(closure->mm, pgd, addr);
> > >  	if (!p4d)
> > >  		return -ENOMEM;
> > > +
> > >  	do {
> > >  		next = p4d_addr_end(addr, end);
> > > -		err = apply_to_pud_range(mm, p4d, addr, next, fn,
> > > data);
> > > +		if (!closure->alloc && p4d_none_or_clear_bad(p4d))
> > > +			continue;
> > > +		err = apply_to_pud_range(closure, p4d, addr, next);
> > >  		if (err)
> > >  			break;
> > >  	} while (p4d++, addr = next, addr != end);
> > >  	return err;
> > >  }
> > >  
> > > -/*
> > > - * Scan a region of virtual memory, filling in page tables as
> > > necessary
> > > - * and calling a provided function on each leaf page table.
> > > +/**
> > > + * apply_to_pfn_range - Scan a region of virtual memory, calling a
> > > provided
> > > + * function on each leaf page table entry
> > > + * @closure: Details about how to scan and what function to apply
> > > + * @addr: Start virtual address
> > > + * @size: Size of the region
> > > + *
> > > + * If @closure->alloc is set to 1, the function will fill in the
> > > page table
> > > + * as necessary. Otherwise it will skip non-present parts.
> > > + * Note: The caller must ensure that the range does not contain
> > > huge pages.
> > > + * The caller must also assure that the proper mmu_notifier
> > > functions are
> > > + * called. Either in the pte leaf function or before and after the
> > > call to
> > > + * apply_to_pfn_range.
> > 
> > This is wrong there should be a big FAT warning that this can only be
> > use
> > against mmap of device file. The page table walking above is broken
> > for
> > various thing you might find in any other vma like THP, device pte,
> > hugetlbfs,
> 
> I was figuring since we didn't export the function anymore, the warning
> and checks could be left to its users, assuming that any other future
> usage of this function would require mm people audit anyway. But I can
> of course add that warning also to this function if you still want
> that?

Yeah more warning are better, people might start using this, i know
some poeple use unexported symbol and then report bugs while they
just were doing something illegal.

> 
> > ...
> > 
> > Also the mmu notifier can not be call from the pfn callback as that
> > callback
> > happens under page table lock (the change_pte notifier callback is
> > useless
> > and not enough). So it _must_ happen around the call to
> > apply_to_pfn_range
> 
> 
> In the comments I was having in mind usage of, for example
> ptep_clear_flush_notify(). But you're the mmu_notifier expert here. Are
> you saying that function by itself would not be sufficient?
> In that case, should I just scratch the text mentioning the pte leaf
> function?

ptep_clear_flush_notify() is useless ... i have posted patches to either
restore it or remove it. In any case you must call mmu notifier range and
they can not happen under lock. You usage looked fine (in the next patch)
but i would rather have a bit of comment here to make sure people are also
aware of that.

While we can hope that people would cc mm when using mm function, it is
not always the case. So i rather be cautious and warn in comment as much
as possible.

Cheers,
Jérôme

Re: [PATCH 2/9] mm: Add an apply_to_pfn_range interface

[Thomas Hellstrom]

On Tue, 2019-04-16 at 10:46 -0400, Jerome Glisse wrote:
> On Sat, Apr 13, 2019 at 08:34:02AM +0000, Thomas Hellstrom wrote:
> > Hi, Jérôme
> > 
> > On Fri, 2019-04-12 at 17:07 -0400, Jerome Glisse wrote:
> > > On Fri, Apr 12, 2019 at 04:04:18PM +0000, Thomas Hellstrom wrote:
> > > > This is basically apply_to_page_range with added functionality:
> > > > Allocating missing parts of the page table becomes optional,
> > > > which
> > > > means that the function can be guaranteed not to error if
> > > > allocation
> > > > is disabled. Also passing of the closure struct and callback
> > > > function
> > > > becomes different and more in line with how things are done
> > > > elsewhere.
> > > > 
> > > > Finally we keep apply_to_page_range as a wrapper around
> > > > apply_to_pfn_range
> > > > 
> > > > The reason for not using the page-walk code is that we want to
> > > > perform
> > > > the page-walk on vmas pointing to an address space without
> > > > requiring the
> > > > mmap_sem to be held rather thand on vmas belonging to a process
> > > > with the
> > > > mmap_sem held.
> > > > 
> > > > Notable changes since RFC:
> > > > Don't export apply_to_pfn range.
> > > > 
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > Cc: Matthew Wilcox <willy@infradead.org>
> > > > Cc: Will Deacon <will.deacon@arm.com>
> > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > Cc: Rik van Riel <riel@surriel.com>
> > > > Cc: Minchan Kim <minchan@kernel.org>
> > > > Cc: Michal Hocko <mhocko@suse.com>
> > > > Cc: Huang Ying <ying.huang@intel.com>
> > > > Cc: Souptick Joarder <jrdr.linux@gmail.com>
> > > > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > > > Cc: linux-mm@kvack.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > > > ---
> > > >  include/linux/mm.h |  10 ++++
> > > >  mm/memory.c        | 130 ++++++++++++++++++++++++++++++++++---
> > > > ----
> > > > ----
> > > >  2 files changed, 108 insertions(+), 32 deletions(-)
> > > > 
> > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > index 80bb6408fe73..b7dd4ddd6efb 100644
> > > > --- a/include/linux/mm.h
> > > > +++ b/include/linux/mm.h
> > > > @@ -2632,6 +2632,16 @@ typedef int (*pte_fn_t)(pte_t *pte,
> > > > pgtable_t token, unsigned long addr,
> > > >  extern int apply_to_page_range(struct mm_struct *mm, unsigned
> > > > long
> > > > address,
> > > >  			       unsigned long size, pte_fn_t fn,
> > > > void
> > > > *data);
> > > >  
> > > > +struct pfn_range_apply;
> > > > +typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned
> > > > long addr,
> > > > +			 struct pfn_range_apply *closure);
> > > > +struct pfn_range_apply {
> > > > +	struct mm_struct *mm;
> > > > +	pter_fn_t ptefn;
> > > > +	unsigned int alloc;
> > > > +};
> > > > +extern int apply_to_pfn_range(struct pfn_range_apply *closure,
> > > > +			      unsigned long address, unsigned
> > > > long
> > > > size);
> > > >  
> > > >  #ifdef CONFIG_PAGE_POISONING
> > > >  extern bool page_poisoning_enabled(void);
> > > > diff --git a/mm/memory.c b/mm/memory.c
> > > > index a95b4a3b1ae2..60d67158964f 100644
> > > > --- a/mm/memory.c
> > > > +++ b/mm/memory.c
> > > > @@ -1938,18 +1938,17 @@ int vm_iomap_memory(struct
> > > > vm_area_struct
> > > > *vma, phys_addr_t start, unsigned long
> > > >  }
> > > >  EXPORT_SYMBOL(vm_iomap_memory);
> > > >  
> > > > -static int apply_to_pte_range(struct mm_struct *mm, pmd_t
> > > > *pmd,
> > > > -				     unsigned long addr,
> > > > unsigned long
> > > > end,
> > > > -				     pte_fn_t fn, void *data)
> > > > +static int apply_to_pte_range(struct pfn_range_apply *closure,
> > > > pmd_t *pmd,
> > > > +			      unsigned long addr, unsigned long
> > > > end)
> > > >  {
> > > >  	pte_t *pte;
> > > >  	int err;
> > > >  	pgtable_t token;
> > > >  	spinlock_t *uninitialized_var(ptl);
> > > >  
> > > > -	pte = (mm == &init_mm) ?
> > > > +	pte = (closure->mm == &init_mm) ?
> > > >  		pte_alloc_kernel(pmd, addr) :
> > > > -		pte_alloc_map_lock(mm, pmd, addr, &ptl);
> > > > +		pte_alloc_map_lock(closure->mm, pmd, addr,
> > > > &ptl);
> > > >  	if (!pte)
> > > >  		return -ENOMEM;
> > > >  
> > > > @@ -1960,86 +1959,107 @@ static int apply_to_pte_range(struct
> > > > mm_struct *mm, pmd_t *pmd,
> > > >  	token = pmd_pgtable(*pmd);
> > > >  
> > > >  	do {
> > > > -		err = fn(pte++, token, addr, data);
> > > > +		err = closure->ptefn(pte++, token, addr,
> > > > closure);
> > > >  		if (err)
> > > >  			break;
> > > >  	} while (addr += PAGE_SIZE, addr != end);
> > > >  
> > > >  	arch_leave_lazy_mmu_mode();
> > > >  
> > > > -	if (mm != &init_mm)
> > > > +	if (closure->mm != &init_mm)
> > > >  		pte_unmap_unlock(pte-1, ptl);
> > > >  	return err;
> > > >  }
> > > >  
> > > > -static int apply_to_pmd_range(struct mm_struct *mm, pud_t
> > > > *pud,
> > > > -				     unsigned long addr,
> > > > unsigned long
> > > > end,
> > > > -				     pte_fn_t fn, void *data)
> > > > +static int apply_to_pmd_range(struct pfn_range_apply *closure,
> > > > pud_t *pud,
> > > > +			      unsigned long addr, unsigned long
> > > > end)
> > > >  {
> > > >  	pmd_t *pmd;
> > > >  	unsigned long next;
> > > > -	int err;
> > > > +	int err = 0;
> > > >  
> > > >  	BUG_ON(pud_huge(*pud));
> > > >  
> > > > -	pmd = pmd_alloc(mm, pud, addr);
> > > > +	pmd = pmd_alloc(closure->mm, pud, addr);
> > > >  	if (!pmd)
> > > >  		return -ENOMEM;
> > > > +
> > > >  	do {
> > > >  		next = pmd_addr_end(addr, end);
> > > > -		err = apply_to_pte_range(mm, pmd, addr, next,
> > > > fn,
> > > > data);
> > > > +		if (!closure->alloc &&
> > > > pmd_none_or_clear_bad(pmd))
> > > > +			continue;
> > > > +		err = apply_to_pte_range(closure, pmd, addr,
> > > > next);
> > > >  		if (err)
> > > >  			break;
> > > >  	} while (pmd++, addr = next, addr != end);
> > > >  	return err;
> > > >  }
> > > >  
> > > > -static int apply_to_pud_range(struct mm_struct *mm, p4d_t
> > > > *p4d,
> > > > -				     unsigned long addr,
> > > > unsigned long
> > > > end,
> > > > -				     pte_fn_t fn, void *data)
> > > > +static int apply_to_pud_range(struct pfn_range_apply *closure,
> > > > p4d_t *p4d,
> > > > +			      unsigned long addr, unsigned long
> > > > end)
> > > >  {
> > > >  	pud_t *pud;
> > > >  	unsigned long next;
> > > > -	int err;
> > > > +	int err = 0;
> > > >  
> > > > -	pud = pud_alloc(mm, p4d, addr);
> > > > +	pud = pud_alloc(closure->mm, p4d, addr);
> > > >  	if (!pud)
> > > >  		return -ENOMEM;
> > > > +
> > > >  	do {
> > > >  		next = pud_addr_end(addr, end);
> > > > -		err = apply_to_pmd_range(mm, pud, addr, next,
> > > > fn,
> > > > data);
> > > > +		if (!closure->alloc &&
> > > > pud_none_or_clear_bad(pud))
> > > > +			continue;
> > > > +		err = apply_to_pmd_range(closure, pud, addr,
> > > > next);
> > > >  		if (err)
> > > >  			break;
> > > >  	} while (pud++, addr = next, addr != end);
> > > >  	return err;
> > > >  }
> > > >  
> > > > -static int apply_to_p4d_range(struct mm_struct *mm, pgd_t
> > > > *pgd,
> > > > -				     unsigned long addr,
> > > > unsigned long
> > > > end,
> > > > -				     pte_fn_t fn, void *data)
> > > > +static int apply_to_p4d_range(struct pfn_range_apply *closure,
> > > > pgd_t *pgd,
> > > > +			      unsigned long addr, unsigned long
> > > > end)
> > > >  {
> > > >  	p4d_t *p4d;
> > > >  	unsigned long next;
> > > > -	int err;
> > > > +	int err = 0;
> > > >  
> > > > -	p4d = p4d_alloc(mm, pgd, addr);
> > > > +	p4d = p4d_alloc(closure->mm, pgd, addr);
> > > >  	if (!p4d)
> > > >  		return -ENOMEM;
> > > > +
> > > >  	do {
> > > >  		next = p4d_addr_end(addr, end);
> > > > -		err = apply_to_pud_range(mm, p4d, addr, next,
> > > > fn,
> > > > data);
> > > > +		if (!closure->alloc &&
> > > > p4d_none_or_clear_bad(p4d))
> > > > +			continue;
> > > > +		err = apply_to_pud_range(closure, p4d, addr,
> > > > next);
> > > >  		if (err)
> > > >  			break;
> > > >  	} while (p4d++, addr = next, addr != end);
> > > >  	return err;
> > > >  }
> > > >  
> > > > -/*
> > > > - * Scan a region of virtual memory, filling in page tables as
> > > > necessary
> > > > - * and calling a provided function on each leaf page table.
> > > > +/**
> > > > + * apply_to_pfn_range - Scan a region of virtual memory,
> > > > calling a
> > > > provided
> > > > + * function on each leaf page table entry
> > > > + * @closure: Details about how to scan and what function to
> > > > apply
> > > > + * @addr: Start virtual address
> > > > + * @size: Size of the region
> > > > + *
> > > > + * If @closure->alloc is set to 1, the function will fill in
> > > > the
> > > > page table
> > > > + * as necessary. Otherwise it will skip non-present parts.
> > > > + * Note: The caller must ensure that the range does not
> > > > contain
> > > > huge pages.
> > > > + * The caller must also assure that the proper mmu_notifier
> > > > functions are
> > > > + * called. Either in the pte leaf function or before and after
> > > > the
> > > > call to
> > > > + * apply_to_pfn_range.
> > > 
> > > This is wrong there should be a big FAT warning that this can
> > > only be
> > > use
> > > against mmap of device file. The page table walking above is
> > > broken
> > > for
> > > various thing you might find in any other vma like THP, device
> > > pte,
> > > hugetlbfs,
> > 
> > I was figuring since we didn't export the function anymore, the
> > warning
> > and checks could be left to its users, assuming that any other
> > future
> > usage of this function would require mm people audit anyway. But I
> > can
> > of course add that warning also to this function if you still want
> > that?
> 
> Yeah more warning are better, people might start using this, i know
> some poeple use unexported symbol and then report bugs while they
> just were doing something illegal.
> 
> > > ...
> > > 
> > > Also the mmu notifier can not be call from the pfn callback as
> > > that
> > > callback
> > > happens under page table lock (the change_pte notifier callback
> > > is
> > > useless
> > > and not enough). So it _must_ happen around the call to
> > > apply_to_pfn_range
> > 
> > In the comments I was having in mind usage of, for example
> > ptep_clear_flush_notify(). But you're the mmu_notifier expert here.
> > Are
> > you saying that function by itself would not be sufficient?
> > In that case, should I just scratch the text mentioning the pte
> > leaf
> > function?
> 
> ptep_clear_flush_notify() is useless ... i have posted patches to
> either
> restore it or remove it. In any case you must call mmu notifier range
> and
> they can not happen under lock. You usage looked fine (in the next
> patch)
> but i would rather have a bit of comment here to make sure people are
> also
> aware of that.
> 
> While we can hope that people would cc mm when using mm function, it
> is
> not always the case. So i rather be cautious and warn in comment as
> much
> as possible.
> 

OK. Understood. All this actually makes me tend to want to try a bit
harder using a slight modification to the pagewalk code instead. Don't
really want to encourage two parallel code paths doing essentially the
same thing; one good and one bad.

One thing that confuses me a bit with the pagewalk code is that callers
(for example softdirty) typically call
mmu_notifier_invalidate_range_start() around the pagewalk, but then if
it ends up splitting a pmd, mmu_notifier_invalidate_range is called
again, within the first range. Docs aren't really clear whether that's
permitted or not. Is it?

Thanks,
Thomas

> Cheers,
> Jérôme

Re: [PATCH 1/9] mm: Allow the [page|pfn]_mkwrite callbacks to drop the mmap_sem

[Thomas Hellstrom]

Hi, Souptick,

On Sat, 2019-04-13 at 20:41 +0530, Souptick Joarder wrote:
> On Fri, Apr 12, 2019 at 9:34 PM Thomas Hellstrom <
> thellstrom@vmware.com> wrote:
> > Driver fault callbacks are allowed to drop the mmap_sem when
> > expecting
> > long hardware waits to avoid blocking other mm users. Allow the
> > mkwrite
> > callbacks to do the same by returning early on VM_FAULT_RETRY.
> > 
> > In particular we want to be able to drop the mmap_sem when waiting
> > for
> > a reservation object lock on a GPU buffer object. These locks may
> > be
> > held while waiting for the GPU.
> > 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rik van Riel <riel@surriel.com>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Huang Ying <ying.huang@intel.com>
> > Cc: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > 
> > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > ---
> >  mm/memory.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index e11ca9dd823f..a95b4a3b1ae2 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2144,7 +2144,7 @@ static vm_fault_t do_page_mkwrite(struct
> > vm_fault *vmf)
> >         ret = vmf->vma->vm_ops->page_mkwrite(vmf);
> >         /* Restore original flags so that caller is not surprised
> > */
> >         vmf->flags = old_flags;
> > -       if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
> > +       if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_RETRY |
> > VM_FAULT_NOPAGE)))
> 
> With this patch there will multiple instances of (VM_FAULT_ERROR |
> VM_FAULT_RETRY | VM_FAULT_NOPAGE)
> in mm/memory.c. Does it make sense to wrap it in a macro and use it ?

Even though the code will look neater, it might be trickier to follow a
particular error path. Could we perhaps postpone to a follow-up patch?

Thomas



> 
> >                 return ret;
> >         if (unlikely(!(ret & VM_FAULT_LOCKED))) {
> >                 lock_page(page);
> > @@ -2419,7 +2419,7 @@ static vm_fault_t wp_pfn_shared(struct
> > vm_fault *vmf)
> >                 pte_unmap_unlock(vmf->pte, vmf->ptl);
> >                 vmf->flags |= FAULT_FLAG_MKWRITE;
> >                 ret = vma->vm_ops->pfn_mkwrite(vmf);
> > -               if (ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE))
> > +               if (ret & (VM_FAULT_ERROR | VM_FAULT_RETRY |
> > VM_FAULT_NOPAGE))
> >                         return ret;
> >                 return finish_mkwrite_fault(vmf);
> >         }
> > @@ -2440,7 +2440,8 @@ static vm_fault_t wp_page_shared(struct
> > vm_fault *vmf)
> >                 pte_unmap_unlock(vmf->pte, vmf->ptl);
> >                 tmp = do_page_mkwrite(vmf);
> >                 if (unlikely(!tmp || (tmp &
> > -                                     (VM_FAULT_ERROR |
> > VM_FAULT_NOPAGE)))) {
> > +                                     (VM_FAULT_ERROR |
> > VM_FAULT_RETRY |
> > +                                      VM_FAULT_NOPAGE)))) {
> >                         put_page(vmf->page);
> >                         return tmp;
> >                 }
> > @@ -3494,7 +3495,8 @@ static vm_fault_t do_shared_fault(struct
> > vm_fault *vmf)
> >                 unlock_page(vmf->page);
> >                 tmp = do_page_mkwrite(vmf);
> >                 if (unlikely(!tmp ||
> > -                               (tmp & (VM_FAULT_ERROR |
> > VM_FAULT_NOPAGE)))) {
> > +                               (tmp & (VM_FAULT_ERROR |
> > VM_FAULT_RETRY |
> > +                                       VM_FAULT_NOPAGE)))) {
> >                         put_page(vmf->page);
> >                         return tmp;
> >                 }
> > --
> > 2.20.1
> > 

Re: [PATCH 1/9] mm: Allow the [page|pfn]_mkwrite callbacks to drop the mmap_sem

[Souptick Joarder]

On Wed, Apr 17, 2019 at 4:28 PM Thomas Hellstrom <thellstrom@vmware.com> wrote:
>
> Hi, Souptick,
>
> On Sat, 2019-04-13 at 20:41 +0530, Souptick Joarder wrote:
> > On Fri, Apr 12, 2019 at 9:34 PM Thomas Hellstrom <
> > thellstrom@vmware.com> wrote:
> > > Driver fault callbacks are allowed to drop the mmap_sem when
> > > expecting
> > > long hardware waits to avoid blocking other mm users. Allow the
> > > mkwrite
> > > callbacks to do the same by returning early on VM_FAULT_RETRY.
> > >
> > > In particular we want to be able to drop the mmap_sem when waiting
> > > for
> > > a reservation object lock on a GPU buffer object. These locks may
> > > be
> > > held while waiting for the GPU.
> > >
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Matthew Wilcox <willy@infradead.org>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Cc: Rik van Riel <riel@surriel.com>
> > > Cc: Minchan Kim <minchan@kernel.org>
> > > Cc: Michal Hocko <mhocko@suse.com>
> > > Cc: Huang Ying <ying.huang@intel.com>
> > > Cc: Souptick Joarder <jrdr.linux@gmail.com>
> > > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > > Cc: linux-mm@kvack.org
> > > Cc: linux-kernel@vger.kernel.org
> > >
> > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > > ---
> > >  mm/memory.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index e11ca9dd823f..a95b4a3b1ae2 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -2144,7 +2144,7 @@ static vm_fault_t do_page_mkwrite(struct
> > > vm_fault *vmf)
> > >         ret = vmf->vma->vm_ops->page_mkwrite(vmf);
> > >         /* Restore original flags so that caller is not surprised
> > > */
> > >         vmf->flags = old_flags;
> > > -       if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
> > > +       if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_RETRY |
> > > VM_FAULT_NOPAGE)))
> >
> > With this patch there will multiple instances of (VM_FAULT_ERROR |
> > VM_FAULT_RETRY | VM_FAULT_NOPAGE)
> > in mm/memory.c. Does it make sense to wrap it in a macro and use it ?
>
> Even though the code will look neater, it might be trickier to follow a
> particular error path. Could we perhaps postpone to a follow-up patch?

Sure. follow-up-patch is fine.

>
> Thomas
>
>
>
> >
> > >                 return ret;
> > >         if (unlikely(!(ret & VM_FAULT_LOCKED))) {
> > >                 lock_page(page);
> > > @@ -2419,7 +2419,7 @@ static vm_fault_t wp_pfn_shared(struct
> > > vm_fault *vmf)
> > >                 pte_unmap_unlock(vmf->pte, vmf->ptl);
> > >                 vmf->flags |= FAULT_FLAG_MKWRITE;
> > >                 ret = vma->vm_ops->pfn_mkwrite(vmf);
> > > -               if (ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE))
> > > +               if (ret & (VM_FAULT_ERROR | VM_FAULT_RETRY |
> > > VM_FAULT_NOPAGE))
> > >                         return ret;
> > >                 return finish_mkwrite_fault(vmf);
> > >         }
> > > @@ -2440,7 +2440,8 @@ static vm_fault_t wp_page_shared(struct
> > > vm_fault *vmf)
> > >                 pte_unmap_unlock(vmf->pte, vmf->ptl);
> > >                 tmp = do_page_mkwrite(vmf);
> > >                 if (unlikely(!tmp || (tmp &
> > > -                                     (VM_FAULT_ERROR |
> > > VM_FAULT_NOPAGE)))) {
> > > +                                     (VM_FAULT_ERROR |
> > > VM_FAULT_RETRY |
> > > +                                      VM_FAULT_NOPAGE)))) {
> > >                         put_page(vmf->page);
> > >                         return tmp;
> > >                 }
> > > @@ -3494,7 +3495,8 @@ static vm_fault_t do_shared_fault(struct
> > > vm_fault *vmf)
> > >                 unlock_page(vmf->page);
> > >                 tmp = do_page_mkwrite(vmf);
> > >                 if (unlikely(!tmp ||
> > > -                               (tmp & (VM_FAULT_ERROR |
> > > VM_FAULT_NOPAGE)))) {
> > > +                               (tmp & (VM_FAULT_ERROR |
> > > VM_FAULT_RETRY |
> > > +                                       VM_FAULT_NOPAGE)))) {
> > >                         put_page(vmf->page);
> > >                         return tmp;
> > >                 }
> > > --
> > > 2.20.1
> > >

Re: [PATCH 2/9] mm: Add an apply_to_pfn_range interface

[Jerome Glisse]

On Wed, Apr 17, 2019 at 09:15:52AM +0000, Thomas Hellstrom wrote:
> On Tue, 2019-04-16 at 10:46 -0400, Jerome Glisse wrote:
> > On Sat, Apr 13, 2019 at 08:34:02AM +0000, Thomas Hellstrom wrote:
> > > Hi, Jérôme
> > > 
> > > On Fri, 2019-04-12 at 17:07 -0400, Jerome Glisse wrote:
> > > > On Fri, Apr 12, 2019 at 04:04:18PM +0000, Thomas Hellstrom wrote:

[...]

> > > > > -/*
> > > > > - * Scan a region of virtual memory, filling in page tables as
> > > > > necessary
> > > > > - * and calling a provided function on each leaf page table.
> > > > > +/**
> > > > > + * apply_to_pfn_range - Scan a region of virtual memory,
> > > > > calling a
> > > > > provided
> > > > > + * function on each leaf page table entry
> > > > > + * @closure: Details about how to scan and what function to
> > > > > apply
> > > > > + * @addr: Start virtual address
> > > > > + * @size: Size of the region
> > > > > + *
> > > > > + * If @closure->alloc is set to 1, the function will fill in
> > > > > the
> > > > > page table
> > > > > + * as necessary. Otherwise it will skip non-present parts.
> > > > > + * Note: The caller must ensure that the range does not
> > > > > contain
> > > > > huge pages.
> > > > > + * The caller must also assure that the proper mmu_notifier
> > > > > functions are
> > > > > + * called. Either in the pte leaf function or before and after
> > > > > the
> > > > > call to
> > > > > + * apply_to_pfn_range.
> > > > 
> > > > This is wrong there should be a big FAT warning that this can
> > > > only be
> > > > use
> > > > against mmap of device file. The page table walking above is
> > > > broken
> > > > for
> > > > various thing you might find in any other vma like THP, device
> > > > pte,
> > > > hugetlbfs,
> > > 
> > > I was figuring since we didn't export the function anymore, the
> > > warning
> > > and checks could be left to its users, assuming that any other
> > > future
> > > usage of this function would require mm people audit anyway. But I
> > > can
> > > of course add that warning also to this function if you still want
> > > that?
> > 
> > Yeah more warning are better, people might start using this, i know
> > some poeple use unexported symbol and then report bugs while they
> > just were doing something illegal.
> > 
> > > > ...
> > > > 
> > > > Also the mmu notifier can not be call from the pfn callback as
> > > > that
> > > > callback
> > > > happens under page table lock (the change_pte notifier callback
> > > > is
> > > > useless
> > > > and not enough). So it _must_ happen around the call to
> > > > apply_to_pfn_range
> > > 
> > > In the comments I was having in mind usage of, for example
> > > ptep_clear_flush_notify(). But you're the mmu_notifier expert here.
> > > Are
> > > you saying that function by itself would not be sufficient?
> > > In that case, should I just scratch the text mentioning the pte
> > > leaf
> > > function?
> > 
> > ptep_clear_flush_notify() is useless ... i have posted patches to
> > either
> > restore it or remove it. In any case you must call mmu notifier range
> > and
> > they can not happen under lock. You usage looked fine (in the next
> > patch)
> > but i would rather have a bit of comment here to make sure people are
> > also
> > aware of that.
> > 
> > While we can hope that people would cc mm when using mm function, it
> > is
> > not always the case. So i rather be cautious and warn in comment as
> > much
> > as possible.
> > 
> 
> OK. Understood. All this actually makes me tend to want to try a bit
> harder using a slight modification to the pagewalk code instead. Don't
> really want to encourage two parallel code paths doing essentially the
> same thing; one good and one bad.
> 
> One thing that confuses me a bit with the pagewalk code is that callers
> (for example softdirty) typically call
> mmu_notifier_invalidate_range_start() around the pagewalk, but then if
> it ends up splitting a pmd, mmu_notifier_invalidate_range is called
> again, within the first range. Docs aren't really clear whether that's
> permitted or not. Is it?

It is mandatory ie you have to call mmu_notifier_invalidate_range()
in some cases. This is all documented in mmu_notifier.h see struct
mmu_notifier_ops comments and also Documentation/vm/mmu_notifier.rst

Roughly anytime you go from one valid pte (pmd/pud/p4d) to another
valid pte (pmd/pud/p4d) with a different page then you have to call
after clearing pte (pmd/pud/p4d) and before replacing it with its
new value. Changing permission on same page ie going from read and
write to read only, or read only to read and write, does not require
any extra call to mmu_notifier_invalidate_range()

The mmu_notifier_invalidate_range() is important for IOMMU with ATS/
PASID as it is when the flush the TLB and remote device TLB. So you
must flush those secondary TLB after clearing entry so that it can
not race to repopulate the TLB and before setting the new entry so
that at no point in time any hardware can wrongly access old page
while a new page is just now active.

Hopes that clarify it, between if you see any improvement to mmu-
notifier doc it would be more than welcome. I try to put comments
in enough places that people should see at least one of them but
maybe i miss a place where i should have put a comments to point
to the doc :)

Cheers,
Jérôme