[PATCH v4 0/9] Emulated coherent graphics memory

[PATCH v4 0/9] Emulated coherent graphics memory

[Thomas Hellström (VMware)]

Planning to merge this through the drm/vmwgfx tree soon, so if there
are any objections, please speak up.

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%.

Changes v1-v2:
- Addressed a number of typos and formatting issues.
- Added a usage warning for apply_to_pfn_range() and apply_to_page_range()
- Re-evaluated the decision to use apply_to_pfn_range() rather than
  modifying the pagewalk.c. It still looks like generically handling the
  transparent huge page cases requires the mmap_sem to be held at least
  in read mode, so sticking with apply_to_pfn_range() for now.
- The TTM page-fault helper vma copy argument was scratched in favour of
  a pageprot_t argument.
Changes v3:
- Adapted to upstream API changes.
Changes v4:
- Adapted to upstream mmu_notifier changes. (Jerome?)
- Fixed a couple of warnings on 32-bit x86
- Fixed image offset computation on multisample images.
  
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 v4 1/9] mm: Allow the [page|pfn]_mkwrite callbacks to drop the mmap_sem

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

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 ddf20bd0c317..168f546af1ad 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2238,7 +2238,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_NOPAGE | VM_FAULT_RETRY)))
 		return ret;
 	if (unlikely(!(ret & VM_FAULT_LOCKED))) {
 		lock_page(page);
@@ -2515,7 +2515,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_NOPAGE | VM_FAULT_RETRY))
 			return ret;
 		return finish_mkwrite_fault(vmf);
 	}
@@ -2536,7 +2536,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_NOPAGE |
+				       VM_FAULT_RETRY)))) {
 			put_page(vmf->page);
 			return tmp;
 		}
@@ -3601,7 +3602,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_NOPAGE |
+					VM_FAULT_RETRY)))) {
 			put_page(vmf->page);
 			return tmp;
 		}
-- 
2.20.1

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

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

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 than 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>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com> #v1
---
 include/linux/mm.h |  10 ++++
 mm/memory.c        | 135 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 113 insertions(+), 32 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0e8834ac32b7..3d06ce2a64af 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2675,6 +2675,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 168f546af1ad..462aa47f8878 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2032,18 +2032,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;
 
@@ -2054,86 +2053,109 @@ 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 before and after the call to apply_to_pfn_range.
+ *
+ * WARNING: Do not use this function unless you know exactly what you are
+ * doing. It is lacking support for huge pages and transparent huge pages.
+ *
+ * Return: 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;
@@ -2143,16 +2165,65 @@ 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.
+ *
+ * WARNING: Do not use this function unless you know exactly what you are
+ * doing. It is lacking support for huge pages and transparent huge pages.
+ */
+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 v4 3/9] mm: Add write-protect and clean utilities for address space ranges

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

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 "as_dirty_helpers.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> #v1
---
 MAINTAINERS           |   1 +
 include/linux/mm.h    |   9 +-
 mm/Kconfig            |   3 +
 mm/Makefile           |   1 +
 mm/as_dirty_helpers.c | 298 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 311 insertions(+), 1 deletion(-)
 create mode 100644 mm/as_dirty_helpers.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7a2f487ea49a..a55d4ef91b0b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5179,6 +5179,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/as_dirty_helpers.c
 
 DRM DRIVERS
 M:	David Airlie <airlied@linux.ie>
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 3d06ce2a64af..a0bc2a82917e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2685,7 +2685,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 f0c76ba47695..5006d0e6a5c7 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -765,4 +765,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 ac5e5ba78874..f5d412bbc2f7 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -104,3 +104,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) += as_dirty_helpers.o
diff --git a/mm/as_dirty_helpers.c b/mm/as_dirty_helpers.c
new file mode 100644
index 000000000000..ccaad41b70bc
--- /dev/null
+++ b/mm/as_dirty_helpers.c
@@ -0,0 +1,298 @@
+// 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;
+	unsigned long end;
+	unsigned long total;
+	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)) {
+		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
+
+		ptent = pte_wrprotect(old_pte);
+		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
+	pgoff_t 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;
+		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
+
+		ptent = pte_mkclean(old_pte);
+		ptep_modify_prot_commit(aas->vma, addr, pte, old_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, MMU_NOTIFY_PROTECTION_PAGE, 0,
+					vma, 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 bits 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 up 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 v4 3/9] mm: Add write-protect and clean utilities for address space ranges

[Nadav Amit]

> On Jun 11, 2019, at 5:24 AM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
> 
> From: Thomas Hellstrom <thellstrom@vmware.com>
> 

[ snip ]

> +/**
> + * 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)) {
> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
> +
> +		ptent = pte_wrprotect(old_pte);
> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
> +	pgoff_t 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;
> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
> +
> +		ptent = pte_mkclean(old_pte);
> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;

Usually, when a PTE is write-protected, or when a dirty-bit is cleared, the
TLB flush must be done while the page-table lock for that specific table is
taken (i.e., within apply_pt_clean() and apply_pt_wrprotect() in this case).

Otherwise, in the case of apply_pt_clean() for example, another core might
shortly after (before the TLB flush) write to the same page whose PTE was
changed. The dirty-bit in such case might not be set, and the change get
lost.

Does this function regards a certain use-case in which deferring the TLB
flushes is fine? If so, assertions and documentation of the related
assumption would be useful.

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

[Thomas Hellström (VMware)]

Hi, Nadav,

On 6/11/19 7:21 PM, Nadav Amit wrote:
>> On Jun 11, 2019, at 5:24 AM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
>>
>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>
> [ snip ]
>
>> +/**
>> + * 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)) {
>> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
>> +
>> +		ptent = pte_wrprotect(old_pte);
>> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
>> +	pgoff_t 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;
>> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
>> +
>> +		ptent = pte_mkclean(old_pte);
>> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
> Usually, when a PTE is write-protected, or when a dirty-bit is cleared, the
> TLB flush must be done while the page-table lock for that specific table is
> taken (i.e., within apply_pt_clean() and apply_pt_wrprotect() in this case).
>
> Otherwise, in the case of apply_pt_clean() for example, another core might
> shortly after (before the TLB flush) write to the same page whose PTE was
> changed. The dirty-bit in such case might not be set, and the change get
> lost.

Hmm. Let's assume that was the case, we have two possible situations:

A: pt_clean

1. That core's TLB entry is invalid. It will set the PTE dirty bit and 
continue. The dirty bit will probably remain set after the TLB flush.
2. That core's TLB entry is valid. It will just continue. The dirty bit 
will remain clear after the TLB flush.

But I fail to see how having the TLB flush within the page table lock 
would help in this case. Since the writing core will never attempt to 
take it? In any case, if such a race occurs, the corresponding bit in 
the bitmap would have been set and we've recorded that the page is dirty.

B: wrprotect situation, the situation is a bit different:

1. That core's TLB entry is invalid. It will read the PTE, cause a fault 
and block in mkwrite() on an external address space lock which is held 
over this operation. (Is it this situation that is your main concern?)
2. That core's TLB entry is valid. It will just continue regardless of 
any locks.

For both mkwrite() and dirty() if we act on the recorded pages *after* 
the TLB flush, we're OK. The difference is that just after the TLB flush 
there should be no write-enabled PTEs in the write-protect case, but 
there may be dirty PTEs in the pt_clean case. Something that is 
mentioned in the docs already.

>
> Does this function regards a certain use-case in which deferring the TLB
> flushes is fine? If so, assertions and documentation of the related
> assumption would be useful.

If I understand your comment correctly, the page table lock is sometimes 
used as the lock in B1, blocking a possible software fault until the TLB 
flush has happened.  Here we assume an external address space lock taken 
both around the wrprotect operation and in mkwrite(). Would it be OK if 
I add comments about the necessity of an external lock to the doc? Ok 
with a follow-up patch?

Thanks,
Thomas

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

[Nadav Amit]

> On Jun 11, 2019, at 11:26 AM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
> 
> Hi, Nadav,
> 
> On 6/11/19 7:21 PM, Nadav Amit wrote:
>>> On Jun 11, 2019, at 5:24 AM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
>>> 
>>> From: Thomas Hellstrom <thellstrom@vmware.com>
>> [ snip ]
>> 
>>> +/**
>>> + * 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)) {
>>> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
>>> +
>>> +		ptent = pte_wrprotect(old_pte);
>>> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
>>> +	pgoff_t 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;
>>> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
>>> +
>>> +		ptent = pte_mkclean(old_pte);
>>> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
>> Usually, when a PTE is write-protected, or when a dirty-bit is cleared, the
>> TLB flush must be done while the page-table lock for that specific table is
>> taken (i.e., within apply_pt_clean() and apply_pt_wrprotect() in this case).
>> 
>> Otherwise, in the case of apply_pt_clean() for example, another core might
>> shortly after (before the TLB flush) write to the same page whose PTE was
>> changed. The dirty-bit in such case might not be set, and the change get
>> lost.
> 
> Hmm. Let's assume that was the case, we have two possible situations:
> 
> A: pt_clean
> 
> 1. That core's TLB entry is invalid. It will set the PTE dirty bit and continue. The dirty bit will probably remain set after the TLB flush.

I guess you mean the PTE is not cached in the TLB.

> 2. That core's TLB entry is valid. It will just continue. The dirty bit will remain clear after the TLB flush.
> 
> But I fail to see how having the TLB flush within the page table lock would help in this case. Since the writing core will never attempt to take it? In any case, if such a race occurs, the corresponding bit in the bitmap would have been set and we've recorded that the page is dirty.

I don’t understand. What do you mean “recorded that the page is dirty”?
IIUC, the PTE is clear in this case - you mean PG_dirty is set?

To clarify, this code actually may work correctly on Intel CPUs, based on a
recent discussion with Dave Hansen. Apparently, most Intel CPUs set the
dirty bit in memory atomically when a page is first written. 

But this is a generic code and not arch-specific. My concern is that a
certain page might be written to, but would not be marked as dirty in either
the bitmap or the PTE.

The practice of flushing cleaned/write-protected PTEs while hold the
page-table lock related (sorry for my confusion).

> B: wrprotect situation, the situation is a bit different:
> 
> 1. That core's TLB entry is invalid. It will read the PTE, cause a fault and block in mkwrite() on an external address space lock which is held over this operation. (Is it this situation that is your main concern?)
> 2. That core's TLB entry is valid. It will just continue regardless of any locks.
> 
> For both mkwrite() and dirty() if we act on the recorded pages *after* the TLB flush, we're OK. The difference is that just after the TLB flush there should be no write-enabled PTEs in the write-protect case, but there may be dirty PTEs in the pt_clean case. Something that is mentioned in the docs already.

The wrprotect might work correctly, I guess. It does work to mprotect()
(again, sorry for confusing).

>> Does this function regards a certain use-case in which deferring the TLB
>> flushes is fine? If so, assertions and documentation of the related
>> assumption would be useful.
> 
> If I understand your comment correctly, the page table lock is sometimes used as the lock in B1, blocking a possible software fault until the TLB flush has happened.  Here we assume an external address space lock taken both around the wrprotect operation and in mkwrite(). Would it be OK if I add comments about the necessity of an external lock to the doc? Ok with a follow-up patch?

I think the patch should explain itself. I think the comment:

> + * 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. 

... should be more concrete (define address spaces that completely own
memory), and possibly backed by an (debug) assertion to ensure that it is
only used correctly.

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

[Thomas Hellström (VMware)]

On 6/11/19 9:10 PM, Nadav Amit wrote:
>> On Jun 11, 2019, at 11:26 AM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
>>
>> Hi, Nadav,
>>
>> On 6/11/19 7:21 PM, Nadav Amit wrote:
>>>> On Jun 11, 2019, at 5:24 AM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
>>>>
>>>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>> [ snip ]
>>>
>>>> +/**
>>>> + * 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)) {
>>>> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
>>>> +
>>>> +		ptent = pte_wrprotect(old_pte);
>>>> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
>>>> +	pgoff_t 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;
>>>> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
>>>> +
>>>> +		ptent = pte_mkclean(old_pte);
>>>> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
>>> Usually, when a PTE is write-protected, or when a dirty-bit is cleared, the
>>> TLB flush must be done while the page-table lock for that specific table is
>>> taken (i.e., within apply_pt_clean() and apply_pt_wrprotect() in this case).
>>>
>>> Otherwise, in the case of apply_pt_clean() for example, another core might
>>> shortly after (before the TLB flush) write to the same page whose PTE was
>>> changed. The dirty-bit in such case might not be set, and the change get
>>> lost.
>> Hmm. Let's assume that was the case, we have two possible situations:
>>
>> A: pt_clean
>>
>> 1. That core's TLB entry is invalid. It will set the PTE dirty bit and continue. The dirty bit will probably remain set after the TLB flush.
> I guess you mean the PTE is not cached in the TLB.
Yes.
>
>> 2. That core's TLB entry is valid. It will just continue. The dirty bit will remain clear after the TLB flush.
>>
>> But I fail to see how having the TLB flush within the page table lock would help in this case. Since the writing core will never attempt to take it? In any case, if such a race occurs, the corresponding bit in the bitmap would have been set and we've recorded that the page is dirty.
> I don’t understand. What do you mean “recorded that the page is dirty”?
> IIUC, the PTE is clear in this case - you mean PG_dirty is set?

All PTEs we touch and clean are noted in the bitmap.

>
> To clarify, this code actually may work correctly on Intel CPUs, based on a
> recent discussion with Dave Hansen. Apparently, most Intel CPUs set the
> dirty bit in memory atomically when a page is first written.
>
> But this is a generic code and not arch-specific. My concern is that a
> certain page might be written to, but would not be marked as dirty in either
> the bitmap or the PTE.

Regardless of arch, we have four cases:
1. Writes occuring before we scan (and possibly modify) the PTE. Should 
be handled correctly.
2. Writes occurning after the TLB flush. Should be handled correctly, 
unless after a TLB flush the TLB cached entry and the PTE differs on the 
dirty bit. Then we could in theory go on writing without marking the PTE 
dirty. But that would probably be an arch code bug: I mean anything 
using tlb_gather_mmu() would flush TLB outside of the page table lock, 
and if, for example, unmap_mapping_range() left the TLB entries and the 
PTES in an inconsistent state, that wouldn't be good.
3. Writes occuring after the PTE scan, but before the TLB flush without 
us modifying the PTE: That would be the same as a spurious TLB flush. It 
should be harmless. The write will not be picked up in the bitmap, but 
the PTE dirty bit will be set.
4. Writes occuring after us clearing the dirty bit and before the TLB 
flush: We will detect the write, since the bitmap bit is already set. If 
the write continues after the TLB flush, we go to 2.

Note, for archs doing software PTE_DIRTY, that would be very similar to 
softdirty, which is also doing batched TLB flushes...

>
> The practice of flushing cleaned/write-protected PTEs while hold the
> page-table lock related (sorry for my confusion).
>
>> B: wrprotect situation, the situation is a bit different:
>>
>> 1. That core's TLB entry is invalid. It will read the PTE, cause a fault and block in mkwrite() on an external address space lock which is held over this operation. (Is it this situation that is your main concern?)
>> 2. That core's TLB entry is valid. It will just continue regardless of any locks.
>>
>> For both mkwrite() and dirty() if we act on the recorded pages *after* the TLB flush, we're OK. The difference is that just after the TLB flush there should be no write-enabled PTEs in the write-protect case, but there may be dirty PTEs in the pt_clean case. Something that is mentioned in the docs already.
> The wrprotect might work correctly, I guess. It does work to mprotect()
> (again, sorry for confusing).
>
>>> Does this function regards a certain use-case in which deferring the TLB
>>> flushes is fine? If so, assertions and documentation of the related
>>> assumption would be useful.
>> If I understand your comment correctly, the page table lock is sometimes used as the lock in B1, blocking a possible software fault until the TLB flush has happened.  Here we assume an external address space lock taken both around the wrprotect operation and in mkwrite(). Would it be OK if I add comments about the necessity of an external lock to the doc? Ok with a follow-up patch?
> I think the patch should explain itself. I think the comment:
>
>> + * 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.
> ... should be more concrete (define address spaces that completely own
> memory), and possibly backed by an (debug) assertion to ensure that it is
> only used correctly.

Agreed. To clarify we should only run this on VM_IO vmas without 
(trans)huge pages, for which there are already checks. I'll update the doc.

/Thomas

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

[Nadav Amit]

> On Jun 11, 2019, at 2:20 PM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
> 
> On 6/11/19 9:10 PM, Nadav Amit wrote:
>>> On Jun 11, 2019, at 11:26 AM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
>>> 
>>> Hi, Nadav,
>>> 
>>> On 6/11/19 7:21 PM, Nadav Amit wrote:
>>>>> On Jun 11, 2019, at 5:24 AM, Thomas Hellström (VMware) <thellstrom@vmwopensource.org> wrote:
>>>>> 
>>>>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>>> [ snip ]
>>>> 
>>>>> +/**
>>>>> + * 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)) {
>>>>> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
>>>>> +
>>>>> +		ptent = pte_wrprotect(old_pte);
>>>>> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
>>>>> +	pgoff_t 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;
>>>>> +		pte_t old_pte = ptep_modify_prot_start(aas->vma, addr, pte);
>>>>> +
>>>>> +		ptent = pte_mkclean(old_pte);
>>>>> +		ptep_modify_prot_commit(aas->vma, addr, pte, old_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;
>>>> Usually, when a PTE is write-protected, or when a dirty-bit is cleared, the
>>>> TLB flush must be done while the page-table lock for that specific table is
>>>> taken (i.e., within apply_pt_clean() and apply_pt_wrprotect() in this case).
>>>> 
>>>> Otherwise, in the case of apply_pt_clean() for example, another core might
>>>> shortly after (before the TLB flush) write to the same page whose PTE was
>>>> changed. The dirty-bit in such case might not be set, and the change get
>>>> lost.
>>> Hmm. Let's assume that was the case, we have two possible situations:
>>> 
>>> A: pt_clean
>>> 
>>> 1. That core's TLB entry is invalid. It will set the PTE dirty bit and continue. The dirty bit will probably remain set after the TLB flush.
>> I guess you mean the PTE is not cached in the TLB.
> Yes.
>>> 2. That core's TLB entry is valid. It will just continue. The dirty bit will remain clear after the TLB flush.
>>> 
>>> But I fail to see how having the TLB flush within the page table lock would help in this case. Since the writing core will never attempt to take it? In any case, if such a race occurs, the corresponding bit in the bitmap would have been set and we've recorded that the page is dirty.
>> I don’t understand. What do you mean “recorded that the page is dirty”?
>> IIUC, the PTE is clear in this case - you mean PG_dirty is set?
> 
> All PTEs we touch and clean are noted in the bitmap.
> 
>> To clarify, this code actually may work correctly on Intel CPUs, based on a
>> recent discussion with Dave Hansen. Apparently, most Intel CPUs set the
>> dirty bit in memory atomically when a page is first written.
>> 
>> But this is a generic code and not arch-specific. My concern is that a
>> certain page might be written to, but would not be marked as dirty in either
>> the bitmap or the PTE.
> 
> Regardless of arch, we have four cases:
> 1. Writes occuring before we scan (and possibly modify) the PTE. Should be handled correctly.
> 2. Writes occurning after the TLB flush. Should be handled correctly, unless after a TLB flush the TLB cached entry and the PTE differs on the dirty bit. Then we could in theory go on writing without marking the PTE dirty. But that would probably be an arch code bug: I mean anything using tlb_gather_mmu() would flush TLB outside of the page table lock, and if, for example, unmap_mapping_range() left the TLB entries and the PTES in an inconsistent state, that wouldn't be good.
> 3. Writes occuring after the PTE scan, but before the TLB flush without us modifying the PTE: That would be the same as a spurious TLB flush. It should be harmless. The write will not be picked up in the bitmap, but the PTE dirty bit will be set.
> 4. Writes occuring after us clearing the dirty bit and before the TLB flush: We will detect the write, since the bitmap bit is already set. If the write continues after the TLB flush, we go to 2.

Thanks for the detailed explanation. It does sound reasonable.

> Note, for archs doing software PTE_DIRTY, that would be very similar to softdirty, which is also doing batched TLB flushes…

Somewhat similar. But AFAIK, soft-dirty allows you to do only one of two
things:

- Clear the refs ( using /proc/[pid]/clear_refs ); or
- Read the refs (using /proc/[pid]/pagemap )

This interface does not provide any atomicity like the one you try to
obtain.