[RFC PATCH RESEND 0/3] mm modifications / helpers for emulated GPU coherent memory

[RFC PATCH RESEND 0/3] mm modifications / helpers for emulated GPU coherent memory

[Thomas Hellstrom]

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

Resending since last series was sent through a mis-configured SMTP server.

Hi,
This is an early RFC to make sure I don't go too far in the wrong direction.

Non-coherent GPUs that can't directly see contents in CPU-visible memory,
like VMWare's SVGA device, run into trouble when trying to implement
coherent memory requirements of modern graphics APIs. Examples are
Vulkan and OpenGL 4.4's ARB_buffer_storage.

To remedy, we need to emulate coherent memory. Typically when it's detected
that a buffer object is about to be accessed by the GPU, we need to
gather the ranges that have been dirtied by the CPU since the last operation,
apply an operation to make the content visible to the GPU and clear the
the dirty tracking.

Depending on the size of the buffer object and the access pattern there are
two major possibilities:

1) Use page_mkwrite() and pfn_mkwrite(). (GPU buffer objects are backed
either by PCI device memory or by driver-alloced pages).
The dirty-tracking needs to be reset by write-protecting the affected ptes
and flush tlb. This has a complexity of O(num_dirty_pages), but the
write page-fault is of course costly.

2) Use hardware dirty-flags in the ptes. The dirty-tracking needs to be reset
by clearing the dirty bits and flush tlb. This has a complexity of
O(num_buffer_object_pages) and dirty bits need to be scanned in full before
each gpu-access.

So in practice the two methods need to be interleaved for best performance.

So to facilitate this, I propose two new helpers, apply_as_wrprotect() and
apply_as_clean() ("as" stands for address-space) both inspired by
unmap_mapping_range(). Users of these helpers are in the making, but needs
some cleaning-up.

There's also a change to x_mkwrite() to allow dropping the mmap_sem while
waiting.

Any comments or suggestions appreciated.

Thanks,
Thomas

[RFC PATCH RESEND 1/3] 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 a52663c0612d..dcd80313cf10 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;
 		}
@@ -3472,7 +3473,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.19.0.rc1

[RFC PATCH RESEND 2/3] 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

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        | 121 +++++++++++++++++++++++++++++++++------------
 2 files changed, 99 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 dcd80313cf10..0feb7191c2d2 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,103 @@ 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.
+ *
+ * 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 +2065,57 @@ 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;
 }
+EXPORT_SYMBOL_GPL(apply_to_pfn_range);
+
+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.19.0.rc1

Re: [RFC PATCH RESEND 2/3] mm: Add an apply_to_pfn_range interface

[Jerome Glisse]

On Thu, Mar 21, 2019 at 01:22:35PM +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 apply_to_page_range() is dangerous API it does not follow other
mm patterns like mmu notifier. It is suppose to be use in arch code
or vmalloc or similar thing but not in regular driver code. I see
it has crept out of this and is being use by few device driver. I am
not sure we should encourage that.

> 
> 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        | 121 +++++++++++++++++++++++++++++++++------------
>  2 files changed, 99 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 dcd80313cf10..0feb7191c2d2 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,103 @@ 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.
> + *
> + * 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 +2065,57 @@ 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;
>  }
> +EXPORT_SYMBOL_GPL(apply_to_pfn_range);
> +
> +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.19.0.rc1
> 

Re: [RFC PATCH RESEND 2/3] mm: Add an apply_to_pfn_range interface

[Thomas Hellstrom]

On Thu, 2019-03-21 at 09:52 -0400, Jerome Glisse wrote:
> On Thu, Mar 21, 2019 at 01:22:35PM +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 apply_to_page_range() is dangerous API it does not follow other
> mm patterns like mmu notifier. It is suppose to be use in arch code
> or vmalloc or similar thing but not in regular driver code. I see
> it has crept out of this and is being use by few device driver. I am
> not sure we should encourage that.

I can certainly remove the EXPORT of the new apply_to_pfn_range() which
will make sure its use stays within the mm code. I don't expect any
additional usage except for the two address-space utilities.

I'm looking for examples to see how it could be more in line with the
rest of the mm code. The main difference from the pattern in, for
example, page_mkclean() seems to be that it's lacking the
mmu_notifier_invalidate_start() and mmu_notifier_invalidate_end()?
Perhaps the intention is to have the pte leaf functions notify on pte
updates? How does this relate to arch_enter_lazy_mmu() which is called
outside of the page table locks? The documentation appears a bit
scarce...

> 
> > 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        | 121 +++++++++++++++++++++++++++++++++------
> > ------
> >  2 files changed, 99 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 dcd80313cf10..0feb7191c2d2 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,103 @@ 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.
> > + *
> > + * 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 +2065,57 @@ 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;
> >  }
> > +EXPORT_SYMBOL_GPL(apply_to_pfn_range);
> > +
> > +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.19.0.rc1
> > 

Re: [RFC PATCH RESEND 2/3] mm: Add an apply_to_pfn_range interface

[Jerome Glisse]

On Thu, Mar 21, 2019 at 07:59:35PM +0000, Thomas Hellstrom wrote:
> On Thu, 2019-03-21 at 09:52 -0400, Jerome Glisse wrote:
> > On Thu, Mar 21, 2019 at 01:22:35PM +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 apply_to_page_range() is dangerous API it does not follow other
> > mm patterns like mmu notifier. It is suppose to be use in arch code
> > or vmalloc or similar thing but not in regular driver code. I see
> > it has crept out of this and is being use by few device driver. I am
> > not sure we should encourage that.
> 
> I can certainly remove the EXPORT of the new apply_to_pfn_range() which
> will make sure its use stays within the mm code. I don't expect any
> additional usage except for the two address-space utilities.
> 
> I'm looking for examples to see how it could be more in line with the
> rest of the mm code. The main difference from the pattern in, for
> example, page_mkclean() seems to be that it's lacking the
> mmu_notifier_invalidate_start() and mmu_notifier_invalidate_end()?
> Perhaps the intention is to have the pte leaf functions notify on pte
> updates? How does this relate to arch_enter_lazy_mmu() which is called
> outside of the page table locks? The documentation appears a bit
> scarce...

Best is to use something like walk_page_range() and have proper mmu
notifier in the callback. The apply_to_page_range() is broken for
huge page (THP) and other things like that. Thought you should not
have THP within mmap of a device file (at least i do not thing any
driver does that).

Cheers,
Jérôme

[RFC PATCH RESEND 3/3] 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.

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  |   9 +-
 mm/Makefile         |   2 +-
 mm/apply_as_range.c | 257 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 266 insertions(+), 2 deletions(-)
 create mode 100644 mm/apply_as_range.c

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/Makefile b/mm/Makefile
index d210cc9d6f80..a94b78f12692 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
diff --git a/mm/apply_as_range.c b/mm/apply_as_range.c
new file mode 100644
index 000000000000..9f03e272ebd0
--- /dev/null
+++ b/mm/apply_as_range.c
@@ -0,0 +1,257 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/mm.h>
+#include <linux/mm_types.h>
+#include <linux/hugetlb.h>
+#include <linux/bitops.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
+ * @flush_cache: Whether to call a cache flush before modifying a pte
+ * @flush_tlb: Whether to flush the tlb after modifying a pte
+ */
+struct apply_as {
+	struct pfn_range_apply base;
+	unsigned long start, end;
+	unsigned long total;
+	const struct vm_area_struct *vma;
+	u32 flush_cache : 1;
+	u32 flush_tlb : 1;
+};
+
+/**
+ * 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 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);
+
+	if (pte_write(*pte)) {
+		set_pte_at(closure->mm, addr, pte, pte_wrprotect(*pte));
+		aas->total++;
+		if (addr < aas->start)
+			aas->start = addr;
+		if (addr + PAGE_SIZE > aas->end)
+			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
+ * @end: Address_space page offset of last modified pte
+ */
+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);
+
+	if (pte_dirty(*pte)) {
+		pgoff_t pgoff = ((addr - aas->vma->vm_start) >> PAGE_SHIFT) +
+			aas->vma->vm_pgoff - clean->bitmap_pgoff;
+
+		set_pte_at(closure->mm, addr, pte, pte_mkclean(*pte));
+		aas->total++;
+		if (addr < aas->start)
+			aas->start = addr;
+		if (addr + PAGE_SIZE > aas->end)
+			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;
+
+	/* FIXME: Is a read lock sufficient here? */
+	down_write(&mapping->i_mmap_rwsem);
+	vma_interval_tree_foreach(vma, &mapping->i_mmap, first_index,
+		first_index + nr - 1) {
+		aas->base.mm = vma->vm_mm;
+
+		/* 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;
+
+		/*
+		 * TODO: Should caches be flushed individually on demand
+		 * in the leaf-pte callbacks instead? That is, how
+		 * costly are inter-core interrupts in an SMP system?
+		 */
+		if (aas->flush_cache)
+			flush_cache_range(vma, start_addr, end_addr);
+		aas->start = end_addr;
+		aas->end = start_addr;
+		aas->vma = vma;
+
+		/* Should not error since aas->base.alloc == 0 */
+		WARN_ON(apply_to_pfn_range(&aas->base, start_addr,
+					   end_addr - start_addr));
+		if (aas->flush_tlb && aas->end > aas->start)
+			flush_tlb_range(vma, aas->start, aas->end);
+	}
+	up_write(&mapping->i_mmap_rwsem);
+
+	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
+ *
+ * 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,
+		.flush_cache = 1,
+		.flush_tlb = 1
+	};
+
+	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 page offset of the first set bit in @bitmap, or if
+ * none set the value pointed to should be @bitmap_pgoff + @nr. The value
+ * is modified as new bits are set by the function.
+ * @end: Page offset of the last set bit in @bitmap + 1 or @bitmap_pgoff if
+ * 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.
+ *
+ * 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)
+{
+	struct apply_as_clean clean = {
+		.base = {
+			.base = {
+				.alloc = 0,
+				.ptefn = apply_pt_clean,
+			},
+			.total = 0,
+			.flush_cache = 0,
+			.flush_tlb = 1,
+		},
+		.bitmap_pgoff = bitmap_pgoff,
+		.bitmap = bitmap,
+		.start = *start,
+		.end = *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.19.0.rc1

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

[Jerome Glisse]

On Thu, Mar 21, 2019 at 01:22:41PM +0000, Thomas Hellstrom wrote:
> 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.


Again this does not use mmu notifier and there is no scary comment to
explain the very limited use case it should be use for ie mmap of a
device file and only by the device driver.

Using it ouside of this would break softdirty or trigger false COW or
other scary thing.

> 
> 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  |   9 +-
>  mm/Makefile         |   2 +-
>  mm/apply_as_range.c | 257 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 266 insertions(+), 2 deletions(-)
>  create mode 100644 mm/apply_as_range.c
> 
> 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/Makefile b/mm/Makefile
> index d210cc9d6f80..a94b78f12692 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
> diff --git a/mm/apply_as_range.c b/mm/apply_as_range.c
> new file mode 100644
> index 000000000000..9f03e272ebd0
> --- /dev/null
> +++ b/mm/apply_as_range.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/mm.h>
> +#include <linux/mm_types.h>
> +#include <linux/hugetlb.h>
> +#include <linux/bitops.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
> + * @flush_cache: Whether to call a cache flush before modifying a pte
> + * @flush_tlb: Whether to flush the tlb after modifying a pte
> + */
> +struct apply_as {
> +	struct pfn_range_apply base;
> +	unsigned long start, end;
> +	unsigned long total;
> +	const struct vm_area_struct *vma;
> +	u32 flush_cache : 1;
> +	u32 flush_tlb : 1;
> +};
> +
> +/**
> + * 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 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);
> +
> +	if (pte_write(*pte)) {
> +		set_pte_at(closure->mm, addr, pte, pte_wrprotect(*pte));

So there is no flushing here, even for x96 this is wrong. It
should be something like:
    ptep_clear_flush()
    flush_cache_page() // if pte is pointing to a regular page
    set_pte_at()
    update_mmu_cache()


> +		aas->total++;
> +		if (addr < aas->start)
> +			aas->start = addr;
> +		if (addr + PAGE_SIZE > aas->end)
> +			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
> + * @end: Address_space page offset of last modified pte
> + */
> +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);
> +
> +	if (pte_dirty(*pte)) {
> +		pgoff_t pgoff = ((addr - aas->vma->vm_start) >> PAGE_SHIFT) +
> +			aas->vma->vm_pgoff - clean->bitmap_pgoff;
> +
> +		set_pte_at(closure->mm, addr, pte, pte_mkclean(*pte));

Clearing the dirty bit is racy, it should be done with write protect
instead as the dirty bit can be set again just after you clear it.
So i am not sure what is the usage pattern where you want to clear
that bit without write protect.

You also need proper page flushing with flush_cache_page()

> +		aas->total++;
> +		if (addr < aas->start)
> +			aas->start = addr;
> +		if (addr + PAGE_SIZE > aas->end)
> +			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
> + */

This comment need to be _scary_ it should only be use for device driver
vma ie device driver mapping.

> +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;
> +
> +	/* FIXME: Is a read lock sufficient here? */
> +	down_write(&mapping->i_mmap_rwsem);

read would be sufficient and you should use i_mmap_lock_read() not
the down_write/read API.

> +	vma_interval_tree_foreach(vma, &mapping->i_mmap, first_index,
> +		first_index + nr - 1) {
> +		aas->base.mm = vma->vm_mm;
> +
> +		/* 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;
> +
> +		/*
> +		 * TODO: Should caches be flushed individually on demand
> +		 * in the leaf-pte callbacks instead? That is, how
> +		 * costly are inter-core interrupts in an SMP system?
> +		 */
> +		if (aas->flush_cache)
> +			flush_cache_range(vma, start_addr, end_addr);

flush_cache_range() is a noop on most architecture what you really need
is proper per page flushing see above.

> +		aas->start = end_addr;
> +		aas->end = start_addr;
> +		aas->vma = vma;
> +
> +		/* Should not error since aas->base.alloc == 0 */
> +		WARN_ON(apply_to_pfn_range(&aas->base, start_addr,
> +					   end_addr - start_addr));
> +		if (aas->flush_tlb && aas->end > aas->start)
> +			flush_tlb_range(vma, aas->start, aas->end);
> +	}
> +	up_write(&mapping->i_mmap_rwsem);
> +
> +	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
> + *
> + * Return: The number of ptes actually write-protected. Note that
> + * already write-protected ptes are not counted.
> + */

It should be scary and limited to mapping of device file.


> +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,
> +		.flush_cache = 1,
> +		.flush_tlb = 1
> +	};
> +
> +	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 page offset of the first set bit in @bitmap, or if
> + * none set the value pointed to should be @bitmap_pgoff + @nr. The value
> + * is modified as new bits are set by the function.
> + * @end: Page offset of the last set bit in @bitmap + 1 or @bitmap_pgoff if
> + * 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.
> + *
> + * Return: The number of dirty ptes actually cleaned.
> + */

It should be scary and limited to mapping of device file.

Cheers,
Jérôme

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

[Thomas Hellstrom]

On Thu, 2019-03-21 at 10:12 -0400, Jerome Glisse wrote:
> On Thu, Mar 21, 2019 at 01:22:41PM +0000, Thomas Hellstrom wrote:
> > 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.
> 
> Again this does not use mmu notifier and there is no scary comment to
> explain the very limited use case it should be use for ie mmap of a
> device file and only by the device driver.

Scary comment and asserts will be added.

> 
> Using it ouside of this would break softdirty or trigger false COW or
> other scary thing.

This is something that should clearly be avoided if at all possible.
False COWs could be avoided by asserting that VMAs are shared. I need
to look deaper into softdirty, but note that the __mkwrite / dirty /
clean pattern is already used in a very similar way in
drivers/video/fb_defio.c although it operates only on real pages one at
a time.

> 
> > 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  |   9 +-
> >  mm/Makefile         |   2 +-
> >  mm/apply_as_range.c | 257
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 266 insertions(+), 2 deletions(-)
> >  create mode 100644 mm/apply_as_range.c
> > 
> > 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/Makefile b/mm/Makefile
> > index d210cc9d6f80..a94b78f12692 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
> > diff --git a/mm/apply_as_range.c b/mm/apply_as_range.c
> > new file mode 100644
> > index 000000000000..9f03e272ebd0
> > --- /dev/null
> > +++ b/mm/apply_as_range.c
> > @@ -0,0 +1,257 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/mm.h>
> > +#include <linux/mm_types.h>
> > +#include <linux/hugetlb.h>
> > +#include <linux/bitops.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
> > + * @flush_cache: Whether to call a cache flush before modifying a
> > pte
> > + * @flush_tlb: Whether to flush the tlb after modifying a pte
> > + */
> > +struct apply_as {
> > +	struct pfn_range_apply base;
> > +	unsigned long start, end;
> > +	unsigned long total;
> > +	const struct vm_area_struct *vma;
> > +	u32 flush_cache : 1;
> > +	u32 flush_tlb : 1;
> > +};
> > +
> > +/**
> > + * 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 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);
> > +
> > +	if (pte_write(*pte)) {
> > +		set_pte_at(closure->mm, addr, pte,
> > pte_wrprotect(*pte));
> 
> So there is no flushing here, even for x96 this is wrong. It
> should be something like:
>     ptep_clear_flush()
>     flush_cache_page() // if pte is pointing to a regular page
>     set_pte_at()
>     update_mmu_cache()
> 

Here cache flushing is done before any leaf function is called.
According to 1) that should be equivalent, although flushing cache in
the leaf function is probably more efficient for most use cases. Both
these functions are no-ops for both x86 and ARM64 where they most
likely will be used...

For ptep_clear_flush() the TLB flushing is here instead deferred to
after all leaf functions have been called. It looks like if the PTE is
dirty, the TLB has no business touching it until then anyway, it should
be happy with its cached value.

Since flushing a single tlb page involves a broadcast across all cores,
I believe flushing a range is a pretty important optimization.

Also for update_mmu_cache() the impression I got from its docs is that
it should only be used when increasing pte permissions, like in fault
handlers, not the opposite?

> 
> > +		aas->total++;
> > +		if (addr < aas->start)
> > +			aas->start = addr;
> > +		if (addr + PAGE_SIZE > aas->end)
> > +			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
> > + * @end: Address_space page offset of last modified pte
> > + */
> > +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);
> > +
> > +	if (pte_dirty(*pte)) {
> > +		pgoff_t pgoff = ((addr - aas->vma->vm_start) >>
> > PAGE_SHIFT) +
> > +			aas->vma->vm_pgoff - clean->bitmap_pgoff;
> > +
> > +		set_pte_at(closure->mm, addr, pte, pte_mkclean(*pte));
> 
> Clearing the dirty bit is racy, it should be done with write protect
> instead as the dirty bit can be set again just after you clear it.
> So i am not sure what is the usage pattern where you want to clear
> that bit without write protect.

If it's set again, then it will be picked up at the next GPU command
submission referencing this page i. e. the next run of this function.
What we're after here is to get to all pages that were dirtied *before*
this call. The raciness and remedy (if desired) is mentioned in the
comments to the exported function below. Typically users write-protect
before scanning dirty bits only if transitioning to mkwrite-dirtying.
The important thing is that we don't accidently clear dirty bits
without picking them up.

> 
> You also need proper page flushing with flush_cache_page()
> 
> > +		aas->total++;
> > +		if (addr < aas->start)
> > +			aas->start = addr;
> > +		if (addr + PAGE_SIZE > aas->end)
> > +			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
> > + */
> 
> This comment need to be _scary_ it should only be use for device
> driver
> vma ie device driver mapping.
> 
> > +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;
> > +
> > +	/* FIXME: Is a read lock sufficient here? */
> > +	down_write(&mapping->i_mmap_rwsem);
> 
> read would be sufficient and you should use i_mmap_lock_read() not
> the down_write/read API.
> 
> > +	vma_interval_tree_foreach(vma, &mapping->i_mmap, first_index,
> > +		first_index + nr - 1) {
> > +		aas->base.mm = vma->vm_mm;
> > +
> > +		/* 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;
> > +
> > +		/*
> > +		 * TODO: Should caches be flushed individually on
> > demand
> > +		 * in the leaf-pte callbacks instead? That is, how
> > +		 * costly are inter-core interrupts in an SMP system?
> > +		 */
> > +		if (aas->flush_cache)
> > +			flush_cache_range(vma, start_addr, end_addr);
> 
> flush_cache_range() is a noop on most architecture what you really
> need
> is proper per page flushing see above.

From the docs 1) they are interchangeable. But I will change to 
per-page cache flushing anyway.


> 
> > +		aas->start = end_addr;
> > +		aas->end = start_addr;
> > +		aas->vma = vma;
> > +
> > +		/* Should not error since aas->base.alloc == 0 */
> > +		WARN_ON(apply_to_pfn_range(&aas->base, start_addr,
> > +					   end_addr - start_addr));
> > +		if (aas->flush_tlb && aas->end > aas->start)
> > +			flush_tlb_range(vma, aas->start, aas->end);
> > +	}
> > +	up_write(&mapping->i_mmap_rwsem);
> > +
> > +	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
> > + *
> > + * Return: The number of ptes actually write-protected. Note that
> > + * already write-protected ptes are not counted.
> > + */
> 
> It should be scary and limited to mapping of device file.
> 

Agreed.

> 
> > +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,
> > +		.flush_cache = 1,
> > +		.flush_tlb = 1
> > +	};
> > +
> > +	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 page offset of the first set bit in @bitmap,
> > or if
> > + * none set the value pointed to should be @bitmap_pgoff + @nr.
> > The value
> > + * is modified as new bits are set by the function.
> > + * @end: Page offset of the last set bit in @bitmap + 1 or
> > @bitmap_pgoff if
> > + * 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.
> > + *
> > + * Return: The number of dirty ptes actually cleaned.
> > + */
> 
> It should be scary and limited to mapping of device file.
> 
> Cheers,
> Jérôme

1) Documentation/cachetlb.txt

Thanks!

Thomas

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

[Jerome Glisse]

On Thu, Mar 21, 2019 at 08:29:31PM +0000, Thomas Hellstrom wrote:
> On Thu, 2019-03-21 at 10:12 -0400, Jerome Glisse wrote:
> > On Thu, Mar 21, 2019 at 01:22:41PM +0000, Thomas Hellstrom wrote:
> > > 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.
> > 
> > Again this does not use mmu notifier and there is no scary comment to
> > explain the very limited use case it should be use for ie mmap of a
> > device file and only by the device driver.
> 
> Scary comment and asserts will be added.
> 
> > 
> > Using it ouside of this would break softdirty or trigger false COW or
> > other scary thing.
> 
> This is something that should clearly be avoided if at all possible.
> False COWs could be avoided by asserting that VMAs are shared. I need
> to look deaper into softdirty, but note that the __mkwrite / dirty /
> clean pattern is already used in a very similar way in
> drivers/video/fb_defio.c although it operates only on real pages one at
> a time.

It should just be allow only for mapping of device file for which none
of the above apply (softdirty, COW, ...).

> 
> > 
> > > 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  |   9 +-
> > >  mm/Makefile         |   2 +-
> > >  mm/apply_as_range.c | 257
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 266 insertions(+), 2 deletions(-)
> > >  create mode 100644 mm/apply_as_range.c
> > > 
> > > 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/Makefile b/mm/Makefile
> > > index d210cc9d6f80..a94b78f12692 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
> > > diff --git a/mm/apply_as_range.c b/mm/apply_as_range.c
> > > new file mode 100644
> > > index 000000000000..9f03e272ebd0
> > > --- /dev/null
> > > +++ b/mm/apply_as_range.c
> > > @@ -0,0 +1,257 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +#include <linux/mm.h>
> > > +#include <linux/mm_types.h>
> > > +#include <linux/hugetlb.h>
> > > +#include <linux/bitops.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
> > > + * @flush_cache: Whether to call a cache flush before modifying a
> > > pte
> > > + * @flush_tlb: Whether to flush the tlb after modifying a pte
> > > + */
> > > +struct apply_as {
> > > +	struct pfn_range_apply base;
> > > +	unsigned long start, end;
> > > +	unsigned long total;
> > > +	const struct vm_area_struct *vma;
> > > +	u32 flush_cache : 1;
> > > +	u32 flush_tlb : 1;
> > > +};
> > > +
> > > +/**
> > > + * 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 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);
> > > +
> > > +	if (pte_write(*pte)) {
> > > +		set_pte_at(closure->mm, addr, pte,
> > > pte_wrprotect(*pte));
> > 
> > So there is no flushing here, even for x96 this is wrong. It
> > should be something like:
> >     ptep_clear_flush()
> >     flush_cache_page() // if pte is pointing to a regular page
> >     set_pte_at()
> >     update_mmu_cache()
> > 
> 
> Here cache flushing is done before any leaf function is called.
> According to 1) that should be equivalent, although flushing cache in
> the leaf function is probably more efficient for most use cases. Both
> these functions are no-ops for both x86 and ARM64 where they most
> likely will be used...
> 
> For ptep_clear_flush() the TLB flushing is here instead deferred to
> after all leaf functions have been called. It looks like if the PTE is
> dirty, the TLB has no business touching it until then anyway, it should
> be happy with its cached value.
> 
> Since flushing a single tlb page involves a broadcast across all cores,
> I believe flushing a range is a pretty important optimization.

Reading the code i missed the range flush below, it should be ok but
you should be using ptep_modify_prot_start()/ptep_modify_prot_commit()
pattern. I think some arch like to be involve in pte changes and the
2 patterns so far in the kernel (AFAIK) is ptep_clear_flush() or the
ptep_modify_prot_start//ptep_modify_prot_commit so i believe it is
better to stick to one of those instead of introducing a third one.

> 
> Also for update_mmu_cache() the impression I got from its docs is that
> it should only be used when increasing pte permissions, like in fault
> handlers, not the opposite?

I think some arch rely on it for something else but if you use the
range flushing properly you should not need it.

> > 
> > > +		aas->total++;
> > > +		if (addr < aas->start)
> > > +			aas->start = addr;
> > > +		if (addr + PAGE_SIZE > aas->end)
> > > +			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
> > > + * @end: Address_space page offset of last modified pte
> > > + */
> > > +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);
> > > +
> > > +	if (pte_dirty(*pte)) {
> > > +		pgoff_t pgoff = ((addr - aas->vma->vm_start) >>
> > > PAGE_SHIFT) +
> > > +			aas->vma->vm_pgoff - clean->bitmap_pgoff;
> > > +
> > > +		set_pte_at(closure->mm, addr, pte, pte_mkclean(*pte));
> > 
> > Clearing the dirty bit is racy, it should be done with write protect
> > instead as the dirty bit can be set again just after you clear it.
> > So i am not sure what is the usage pattern where you want to clear
> > that bit without write protect.
> 
> If it's set again, then it will be picked up at the next GPU command
> submission referencing this page i. e. the next run of this function.
> What we're after here is to get to all pages that were dirtied *before*
> this call. The raciness and remedy (if desired) is mentioned in the
> comments to the exported function below. Typically users write-protect
> before scanning dirty bits only if transitioning to mkwrite-dirtying.
> The important thing is that we don't accidently clear dirty bits
> without picking them up.

Fair enough.

> > 
> > You also need proper page flushing with flush_cache_page()
> > 
> > > +		aas->total++;
> > > +		if (addr < aas->start)
> > > +			aas->start = addr;
> > > +		if (addr + PAGE_SIZE > aas->end)
> > > +			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
> > > + */
> > 
> > This comment need to be _scary_ it should only be use for device
> > driver
> > vma ie device driver mapping.
> > 
> > > +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;
> > > +
> > > +	/* FIXME: Is a read lock sufficient here? */
> > > +	down_write(&mapping->i_mmap_rwsem);
> > 
> > read would be sufficient and you should use i_mmap_lock_read() not
> > the down_write/read API.
> > 
> > > +	vma_interval_tree_foreach(vma, &mapping->i_mmap, first_index,
> > > +		first_index + nr - 1) {
> > > +		aas->base.mm = vma->vm_mm;
> > > +
> > > +		/* 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;
> > > +
> > > +		/*
> > > +		 * TODO: Should caches be flushed individually on
> > > demand
> > > +		 * in the leaf-pte callbacks instead? That is, how
> > > +		 * costly are inter-core interrupts in an SMP system?
> > > +		 */
> > > +		if (aas->flush_cache)
> > > +			flush_cache_range(vma, start_addr, end_addr);
> > 
> > flush_cache_range() is a noop on most architecture what you really
> > need
> > is proper per page flushing see above.
> 
> From the docs 1) they are interchangeable. But I will change to 
> per-page cache flushing anyway.

Yeah you can do flush_cache_range() it is fine.

Cheers,
Jérôme

Re: [RFC PATCH RESEND 0/3] mm modifications / helpers for emulated GPU coherent memory

[Jerome Glisse]

On Thu, Mar 21, 2019 at 01:22:22PM +0000, Thomas Hellstrom wrote:
> Resending since last series was sent through a mis-configured SMTP server.
> 
> Hi,
> This is an early RFC to make sure I don't go too far in the wrong direction.
> 
> Non-coherent GPUs that can't directly see contents in CPU-visible memory,
> like VMWare's SVGA device, run into trouble when trying to implement
> coherent memory requirements of modern graphics APIs. Examples are
> Vulkan and OpenGL 4.4's ARB_buffer_storage.
> 
> To remedy, we need to emulate coherent memory. Typically when it's detected
> that a buffer object is about to be accessed by the GPU, we need to
> gather the ranges that have been dirtied by the CPU since the last operation,
> apply an operation to make the content visible to the GPU and clear the
> the dirty tracking.
> 
> Depending on the size of the buffer object and the access pattern there are
> two major possibilities:
> 
> 1) Use page_mkwrite() and pfn_mkwrite(). (GPU buffer objects are backed
> either by PCI device memory or by driver-alloced pages).
> The dirty-tracking needs to be reset by write-protecting the affected ptes
> and flush tlb. This has a complexity of O(num_dirty_pages), but the
> write page-fault is of course costly.
> 
> 2) Use hardware dirty-flags in the ptes. The dirty-tracking needs to be reset
> by clearing the dirty bits and flush tlb. This has a complexity of
> O(num_buffer_object_pages) and dirty bits need to be scanned in full before
> each gpu-access.
> 
> So in practice the two methods need to be interleaved for best performance.
> 
> So to facilitate this, I propose two new helpers, apply_as_wrprotect() and
> apply_as_clean() ("as" stands for address-space) both inspired by
> unmap_mapping_range(). Users of these helpers are in the making, but needs
> some cleaning-up.

To be clear this should _only be use_ for mmap of device file ? If so
the API should try to enforce that as much as possible for instance by
mandating the file as argument so that the function can check it is
only use in that case. Also big scary comment to make sure no one just
start using those outside this very limited frame.

> 
> There's also a change to x_mkwrite() to allow dropping the mmap_sem while
> waiting.

This will most likely conflict with userfaultfd write protection. Maybe
building your thing on top of that would be better.

https://lwn.net/Articles/783571/

I will take a cursory look at the patches.

Cheers,
Jérôme

Re: [RFC PATCH RESEND 0/3] mm modifications / helpers for emulated GPU coherent memory

[Thomas Hellstrom]

Hi, Jérôme,

Thanks for commenting. I have a couple of questions / clarifications
below.

On Thu, 2019-03-21 at 09:46 -0400, Jerome Glisse wrote:
> On Thu, Mar 21, 2019 at 01:22:22PM +0000, Thomas Hellstrom wrote:
> > Resending since last series was sent through a mis-configured SMTP
> > server.
> > 
> > Hi,
> > This is an early RFC to make sure I don't go too far in the wrong
> > direction.
> > 
> > Non-coherent GPUs that can't directly see contents in CPU-visible
> > memory,
> > like VMWare's SVGA device, run into trouble when trying to
> > implement
> > coherent memory requirements of modern graphics APIs. Examples are
> > Vulkan and OpenGL 4.4's ARB_buffer_storage.
> > 
> > To remedy, we need to emulate coherent memory. Typically when it's
> > detected
> > that a buffer object is about to be accessed by the GPU, we need to
> > gather the ranges that have been dirtied by the CPU since the last
> > operation,
> > apply an operation to make the content visible to the GPU and clear
> > the
> > the dirty tracking.
> > 
> > Depending on the size of the buffer object and the access pattern
> > there are
> > two major possibilities:
> > 
> > 1) Use page_mkwrite() and pfn_mkwrite(). (GPU buffer objects are
> > backed
> > either by PCI device memory or by driver-alloced pages).
> > The dirty-tracking needs to be reset by write-protecting the
> > affected ptes
> > and flush tlb. This has a complexity of O(num_dirty_pages), but the
> > write page-fault is of course costly.
> > 
> > 2) Use hardware dirty-flags in the ptes. The dirty-tracking needs
> > to be reset
> > by clearing the dirty bits and flush tlb. This has a complexity of
> > O(num_buffer_object_pages) and dirty bits need to be scanned in
> > full before
> > each gpu-access.
> > 
> > So in practice the two methods need to be interleaved for best
> > performance.
> > 
> > So to facilitate this, I propose two new helpers,
> > apply_as_wrprotect() and
> > apply_as_clean() ("as" stands for address-space) both inspired by
> > unmap_mapping_range(). Users of these helpers are in the making,
> > but needs
> > some cleaning-up.
> 
> To be clear this should _only be use_ for mmap of device file ? If so
> the API should try to enforce that as much as possible for instance
> by
> mandating the file as argument so that the function can check it is
> only use in that case. Also big scary comment to make sure no one
> just
> start using those outside this very limited frame.

Fine with me. Perhaps we could BUG() / WARN() on certain VMA flags 
instead of mandating the file as argument. That can make sure we
don't accidently hit pages we shouldn't hit.

> 
> > There's also a change to x_mkwrite() to allow dropping the mmap_sem
> > while
> > waiting.
> 
> This will most likely conflict with userfaultfd write protection. 

Are you referring to the x_mkwrite() usage itself or the mmap_sem
dropping facilitation?

> Maybe
> building your thing on top of that would be better.
> 
> 
...
> 
> I will take a cursory look at the patches.
> 

Some more questions / clarifications on those as well.


> Cheers,
> Jérôme

Re: [RFC PATCH RESEND 0/3] mm modifications / helpers for emulated GPU coherent memory

[Jerome Glisse]

On Thu, Mar 21, 2019 at 07:51:16PM +0000, Thomas Hellstrom wrote:
> Hi, Jérôme,
> 
> Thanks for commenting. I have a couple of questions / clarifications
> below.
> 
> On Thu, 2019-03-21 at 09:46 -0400, Jerome Glisse wrote:
> > On Thu, Mar 21, 2019 at 01:22:22PM +0000, Thomas Hellstrom wrote:
> > > Resending since last series was sent through a mis-configured SMTP
> > > server.
> > > 
> > > Hi,
> > > This is an early RFC to make sure I don't go too far in the wrong
> > > direction.
> > > 
> > > Non-coherent GPUs that can't directly see contents in CPU-visible
> > > memory,
> > > like VMWare's SVGA device, run into trouble when trying to
> > > implement
> > > coherent memory requirements of modern graphics APIs. Examples are
> > > Vulkan and OpenGL 4.4's ARB_buffer_storage.
> > > 
> > > To remedy, we need to emulate coherent memory. Typically when it's
> > > detected
> > > that a buffer object is about to be accessed by the GPU, we need to
> > > gather the ranges that have been dirtied by the CPU since the last
> > > operation,
> > > apply an operation to make the content visible to the GPU and clear
> > > the
> > > the dirty tracking.
> > > 
> > > Depending on the size of the buffer object and the access pattern
> > > there are
> > > two major possibilities:
> > > 
> > > 1) Use page_mkwrite() and pfn_mkwrite(). (GPU buffer objects are
> > > backed
> > > either by PCI device memory or by driver-alloced pages).
> > > The dirty-tracking needs to be reset by write-protecting the
> > > affected ptes
> > > and flush tlb. This has a complexity of O(num_dirty_pages), but the
> > > write page-fault is of course costly.
> > > 
> > > 2) Use hardware dirty-flags in the ptes. The dirty-tracking needs
> > > to be reset
> > > by clearing the dirty bits and flush tlb. This has a complexity of
> > > O(num_buffer_object_pages) and dirty bits need to be scanned in
> > > full before
> > > each gpu-access.
> > > 
> > > So in practice the two methods need to be interleaved for best
> > > performance.
> > > 
> > > So to facilitate this, I propose two new helpers,
> > > apply_as_wrprotect() and
> > > apply_as_clean() ("as" stands for address-space) both inspired by
> > > unmap_mapping_range(). Users of these helpers are in the making,
> > > but needs
> > > some cleaning-up.
> > 
> > To be clear this should _only be use_ for mmap of device file ? If so
> > the API should try to enforce that as much as possible for instance
> > by
> > mandating the file as argument so that the function can check it is
> > only use in that case. Also big scary comment to make sure no one
> > just
> > start using those outside this very limited frame.
> 
> Fine with me. Perhaps we could BUG() / WARN() on certain VMA flags 
> instead of mandating the file as argument. That can make sure we
> don't accidently hit pages we shouldn't hit.

You already provide the mapping as argument it should not be hard to
check it is a mapping to a device file as the vma flags will not be
enough to identify this case.

> 
> > 
> > > There's also a change to x_mkwrite() to allow dropping the mmap_sem
> > > while
> > > waiting.
> > 
> > This will most likely conflict with userfaultfd write protection. 
> 
> Are you referring to the x_mkwrite() usage itself or the mmap_sem
> dropping facilitation?

Both i believe, however i have not try to apply your patches on top of
the userfaultfd patchset

Cheers,
Jérôme