Re: [PATCH v5 4/8] mm: Add write-protect and clean utilities for address space ranges
On 10/10/19 4:17 PM, Peter Zijlstra wrote:
On Thu, Oct 10, 2019 at 03:24:47PM +0200, Thomas Hellström (VMware) wrote:
On 10/10/19 3:05 PM, Peter Zijlstra wrote:
On Thu, Oct 10, 2019 at 02:43:10PM +0200, Thomas Hellström (VMware) wrote:
+/**
+ * wp_shared_mapping_range - 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
+ *
+ * Note: This function currently skips transhuge page-table entries, since
+ * it's intended for dirty-tracking on the PTE level. It will warn on
+ * encountering transhuge write-enabled entries, though, and can easily be
+ * extended to handle them as well.
+ *
+ * Return: The number of ptes actually write-protected. Note that
+ * already write-protected ptes are not counted.
+ */
+unsigned long wp_shared_mapping_range(struct address_space *mapping,
+ pgoff_t first_index, pgoff_t nr)
+{
+ struct wp_walk wpwalk = { .total = 0 };
+
+ i_mmap_lock_read(mapping);
+ WARN_ON(walk_page_mapping(mapping, first_index, nr, _walk_ops,
+ ));
+ i_mmap_unlock_read(mapping);
+
+ return wpwalk.total;
+}
That's a read lock, this means there's concurrency to self. What happens
if someone does two concurrent wp_shared_mapping_range() on the same
mapping?
The thing is, because of pte_wrprotect() the iteration that starts last
will see a smaller pte_write range, if it completes first and does
flush_tlb_range(), it will only flush a partial range.
This is exactly what {inc,dec}_tlb_flush_pending() is for, but you're
not using mm_tlb_flush_nested() to detect the situation and do a bigger
flush.
Or if you're not needing that, then I'm missing why.
Good catch. Thanks,
Yes the read lock is not intended to protect against concurrent users but to
protect the vmas from disappearing under us. Since it fundamentally makes no
sense having two concurrent threads picking up dirty ptes on the same
address_space range we have an external range-based lock to protect against
that.
Nothing mandates/verifies the function you expose is used exclusively.
Therefore you cannot make assumptions on that range lock your user has.
However, that external lock doesn't protect other code from concurrently
modifying ptes and having the mm's tlb_flush_pending increased, so I guess
we unconditionally need to test for that and do a full range flush if
necessary?
Yes, something like:
if (mm_tlb_flush_nested(mm))
flush_tlb_range(walk->vma, walk->vma->vm_start,
walk->vma->vm_end);
else if (wpwalk->tlbflush_end > wpwalk->tlbflush_start)
flush_tlb_range(walk->vma, wpwalk->tlbflush_start,
wpwalk->tlbflush_end);
Hi, Peter,
I've updated the patch to incorporate something similar to the above.
Since you've looked at the patch, any chance of an R-B?
Thanks,
Thomas
Re: [PATCH v5 4/8] mm: Add write-protect and clean utilities for address space ranges
On Thu, Oct 10, 2019 at 03:24:47PM +0200, Thomas Hellström (VMware) wrote:
> On 10/10/19 3:05 PM, Peter Zijlstra wrote:
> > On Thu, Oct 10, 2019 at 02:43:10PM +0200, Thomas Hellström (VMware) wrote:
> > > +/**
> > > + * wp_shared_mapping_range - 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
> > > + *
> > > + * Note: This function currently skips transhuge page-table entries,
> > > since
> > > + * it's intended for dirty-tracking on the PTE level. It will warn on
> > > + * encountering transhuge write-enabled entries, though, and can easily
> > > be
> > > + * extended to handle them as well.
> > > + *
> > > + * Return: The number of ptes actually write-protected. Note that
> > > + * already write-protected ptes are not counted.
> > > + */
> > > +unsigned long wp_shared_mapping_range(struct address_space *mapping,
> > > + pgoff_t first_index, pgoff_t nr)
> > > +{
> > > + struct wp_walk wpwalk = { .total = 0 };
> > > +
> > > + i_mmap_lock_read(mapping);
> > > + WARN_ON(walk_page_mapping(mapping, first_index, nr, _walk_ops,
> > > + ));
> > > + i_mmap_unlock_read(mapping);
> > > +
> > > + return wpwalk.total;
> > > +}
> > That's a read lock, this means there's concurrency to self. What happens
> > if someone does two concurrent wp_shared_mapping_range() on the same
> > mapping?
> >
> > The thing is, because of pte_wrprotect() the iteration that starts last
> > will see a smaller pte_write range, if it completes first and does
> > flush_tlb_range(), it will only flush a partial range.
> >
> > This is exactly what {inc,dec}_tlb_flush_pending() is for, but you're
> > not using mm_tlb_flush_nested() to detect the situation and do a bigger
> > flush.
> >
> > Or if you're not needing that, then I'm missing why.
>
> Good catch. Thanks,
>
> Yes the read lock is not intended to protect against concurrent users but to
> protect the vmas from disappearing under us. Since it fundamentally makes no
> sense having two concurrent threads picking up dirty ptes on the same
> address_space range we have an external range-based lock to protect against
> that.
Nothing mandates/verifies the function you expose is used exclusively.
Therefore you cannot make assumptions on that range lock your user has.
> However, that external lock doesn't protect other code from concurrently
> modifying ptes and having the mm's tlb_flush_pending increased, so I guess
> we unconditionally need to test for that and do a full range flush if
> necessary?
Yes, something like:
if (mm_tlb_flush_nested(mm))
flush_tlb_range(walk->vma, walk->vma->vm_start,
walk->vma->vm_end);
else if (wpwalk->tlbflush_end > wpwalk->tlbflush_start)
flush_tlb_range(walk->vma, wpwalk->tlbflush_start,
wpwalk->tlbflush_end);
Re: [PATCH v5 4/8] mm: Add write-protect and clean utilities for address space ranges
On 10/10/19 3:05 PM, Peter Zijlstra wrote:
On Thu, Oct 10, 2019 at 02:43:10PM +0200, Thomas Hellström (VMware) wrote:
+/**
+ * struct wp_walk - Private struct for pagetable walk callbacks
+ * @range: Range for mmu notifiers
+ * @tlbflush_start: Address of first modified pte
+ * @tlbflush_end: Address of last modified pte + 1
+ * @total: Total number of modified ptes
+ */
+struct wp_walk {
+ struct mmu_notifier_range range;
+ unsigned long tlbflush_start;
+ unsigned long tlbflush_end;
+ unsigned long total;
+};
+
+/**
+ * wp_pte - Write-protect a pte
+ * @pte: Pointer to the pte
+ * @addr: The virtual page address
+ * @walk: pagetable walk callback argument
+ *
+ * The function write-protects a pte and records the range in
+ * virtual address space of touched ptes for efficient range TLB flushes.
+ */
+static int wp_pte(pte_t *pte, unsigned long addr, unsigned long end,
+ struct mm_walk *walk)
+{
+ struct wp_walk *wpwalk = walk->private;
+ pte_t ptent = *pte;
+
+ if (pte_write(ptent)) {
+ pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);
+
+ ptent = pte_wrprotect(old_pte);
+ ptep_modify_prot_commit(walk->vma, addr, pte, old_pte, ptent);
+ wpwalk->total++;
+ wpwalk->tlbflush_start = min(wpwalk->tlbflush_start, addr);
+ wpwalk->tlbflush_end = max(wpwalk->tlbflush_end,
+ addr + PAGE_SIZE);
+ }
+
+ return 0;
+}
+/*
+ * wp_clean_pre_vma - The pagewalk pre_vma callback.
+ *
+ * The pre_vma callback performs the cache flush, stages the tlb flush
+ * and calls the necessary mmu notifiers.
+ */
+static int wp_clean_pre_vma(unsigned long start, unsigned long end,
+ struct mm_walk *walk)
+{
+ struct wp_walk *wpwalk = walk->private;
+
+ wpwalk->tlbflush_start = end;
+ wpwalk->tlbflush_end = start;
+
+ mmu_notifier_range_init(>range, MMU_NOTIFY_PROTECTION_PAGE, 0,
+ walk->vma, walk->mm, start, end);
+ mmu_notifier_invalidate_range_start(>range);
+ flush_cache_range(walk->vma, start, end);
+
+ /*
+* We're not using tlb_gather_mmu() since typically
+* only a small subrange of PTEs are affected, whereas
+* tlb_gather_mmu() records the full range.
+*/
+ inc_tlb_flush_pending(walk->mm);
+
+ return 0;
+}
+
+/*
+ * wp_clean_post_vma - The pagewalk post_vma callback.
+ *
+ * The post_vma callback performs the tlb flush and calls necessary mmu
+ * notifiers.
+ */
+static void wp_clean_post_vma(struct mm_walk *walk)
+{
+ struct wp_walk *wpwalk = walk->private;
+
+ if (wpwalk->tlbflush_end > wpwalk->tlbflush_start)
+ flush_tlb_range(walk->vma, wpwalk->tlbflush_start,
+ wpwalk->tlbflush_end);
+
+ mmu_notifier_invalidate_range_end(>range);
+ dec_tlb_flush_pending(walk->mm);
+}
+/**
+ * wp_shared_mapping_range - 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
+ *
+ * Note: This function currently skips transhuge page-table entries, since
+ * it's intended for dirty-tracking on the PTE level. It will warn on
+ * encountering transhuge write-enabled entries, though, and can easily be
+ * extended to handle them as well.
+ *
+ * Return: The number of ptes actually write-protected. Note that
+ * already write-protected ptes are not counted.
+ */
+unsigned long wp_shared_mapping_range(struct address_space *mapping,
+ pgoff_t first_index, pgoff_t nr)
+{
+ struct wp_walk wpwalk = { .total = 0 };
+
+ i_mmap_lock_read(mapping);
+ WARN_ON(walk_page_mapping(mapping, first_index, nr, _walk_ops,
+ ));
+ i_mmap_unlock_read(mapping);
+
+ return wpwalk.total;
+}
That's a read lock, this means there's concurrency to self. What happens
if someone does two concurrent wp_shared_mapping_range() on the same
mapping?
The thing is, because of pte_wrprotect() the iteration that starts last
will see a smaller pte_write range, if it completes first and does
flush_tlb_range(), it will only flush a partial range.
This is exactly what {inc,dec}_tlb_flush_pending() is for, but you're
not using mm_tlb_flush_nested() to detect the situation and do a bigger
flush.
Or if you're not needing that, then I'm missing why.
Good catch. Thanks,
Yes the read lock is not intended to protect against concurrent users
but to protect the vmas from disappearing under us. Since it
fundamentally makes no sense having two concurrent threads picking up
dirty ptes on the same address_space range we have an external
range-based lock to protect against that.
However, that external lock doesn't protect
Re: [PATCH v5 4/8] mm: Add write-protect and clean utilities for address space ranges
On Thu, Oct 10, 2019 at 02:43:10PM +0200, Thomas Hellström (VMware) wrote:
> +/**
> + * struct wp_walk - Private struct for pagetable walk callbacks
> + * @range: Range for mmu notifiers
> + * @tlbflush_start: Address of first modified pte
> + * @tlbflush_end: Address of last modified pte + 1
> + * @total: Total number of modified ptes
> + */
> +struct wp_walk {
> + struct mmu_notifier_range range;
> + unsigned long tlbflush_start;
> + unsigned long tlbflush_end;
> + unsigned long total;
> +};
> +
> +/**
> + * wp_pte - Write-protect a pte
> + * @pte: Pointer to the pte
> + * @addr: The virtual page address
> + * @walk: pagetable walk callback argument
> + *
> + * The function write-protects a pte and records the range in
> + * virtual address space of touched ptes for efficient range TLB flushes.
> + */
> +static int wp_pte(pte_t *pte, unsigned long addr, unsigned long end,
> + struct mm_walk *walk)
> +{
> + struct wp_walk *wpwalk = walk->private;
> + pte_t ptent = *pte;
> +
> + if (pte_write(ptent)) {
> + pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);
> +
> + ptent = pte_wrprotect(old_pte);
> + ptep_modify_prot_commit(walk->vma, addr, pte, old_pte, ptent);
> + wpwalk->total++;
> + wpwalk->tlbflush_start = min(wpwalk->tlbflush_start, addr);
> + wpwalk->tlbflush_end = max(wpwalk->tlbflush_end,
> +addr + PAGE_SIZE);
> + }
> +
> + return 0;
> +}
> +/*
> + * wp_clean_pre_vma - The pagewalk pre_vma callback.
> + *
> + * The pre_vma callback performs the cache flush, stages the tlb flush
> + * and calls the necessary mmu notifiers.
> + */
> +static int wp_clean_pre_vma(unsigned long start, unsigned long end,
> + struct mm_walk *walk)
> +{
> + struct wp_walk *wpwalk = walk->private;
> +
> + wpwalk->tlbflush_start = end;
> + wpwalk->tlbflush_end = start;
> +
> + mmu_notifier_range_init(>range, MMU_NOTIFY_PROTECTION_PAGE, 0,
> + walk->vma, walk->mm, start, end);
> + mmu_notifier_invalidate_range_start(>range);
> + flush_cache_range(walk->vma, start, end);
> +
> + /*
> + * We're not using tlb_gather_mmu() since typically
> + * only a small subrange of PTEs are affected, whereas
> + * tlb_gather_mmu() records the full range.
> + */
> + inc_tlb_flush_pending(walk->mm);
> +
> + return 0;
> +}
> +
> +/*
> + * wp_clean_post_vma - The pagewalk post_vma callback.
> + *
> + * The post_vma callback performs the tlb flush and calls necessary mmu
> + * notifiers.
> + */
> +static void wp_clean_post_vma(struct mm_walk *walk)
> +{
> + struct wp_walk *wpwalk = walk->private;
> +
> + if (wpwalk->tlbflush_end > wpwalk->tlbflush_start)
> + flush_tlb_range(walk->vma, wpwalk->tlbflush_start,
> + wpwalk->tlbflush_end);
> +
> + mmu_notifier_invalidate_range_end(>range);
> + dec_tlb_flush_pending(walk->mm);
> +}
> +/**
> + * wp_shared_mapping_range - 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
> + *
> + * Note: This function currently skips transhuge page-table entries, since
> + * it's intended for dirty-tracking on the PTE level. It will warn on
> + * encountering transhuge write-enabled entries, though, and can easily be
> + * extended to handle them as well.
> + *
> + * Return: The number of ptes actually write-protected. Note that
> + * already write-protected ptes are not counted.
> + */
> +unsigned long wp_shared_mapping_range(struct address_space *mapping,
> + pgoff_t first_index, pgoff_t nr)
> +{
> + struct wp_walk wpwalk = { .total = 0 };
> +
> + i_mmap_lock_read(mapping);
> + WARN_ON(walk_page_mapping(mapping, first_index, nr, _walk_ops,
> + ));
> + i_mmap_unlock_read(mapping);
> +
> + return wpwalk.total;
> +}
That's a read lock, this means there's concurrency to self. What happens
if someone does two concurrent wp_shared_mapping_range() on the same
mapping?
The thing is, because of pte_wrprotect() the iteration that starts last
will see a smaller pte_write range, if it completes first and does
flush_tlb_range(), it will only flush a partial range.
This is exactly what {inc,dec}_tlb_flush_pending() is for, but you're
not using mm_tlb_flush_nested() to detect the situation and do a bigger
flush.
Or if you're not needing that, then I'm missing why.
[PATCH v5 4/8] mm: Add write-protect and clean utilities for address space ranges
From: Thomas Hellstrom
Add two utilities to 1) write-protect and 2) 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
Cc: Matthew Wilcox
Cc: Will Deacon
Cc: Peter Zijlstra
Cc: Rik van Riel
Cc: Minchan Kim
Cc: Michal Hocko
Cc: Huang Ying
Cc: Jérôme Glisse
Cc: Kirill A. Shutemov
Signed-off-by: Thomas Hellstrom
---
include/linux/mm.h | 13 +-
mm/Kconfig | 3 +
mm/Makefile| 1 +
mm/mapping_dirty_helpers.c | 312 +
4 files changed, 328 insertions(+), 1 deletion(-)
create mode 100644 mm/mapping_dirty_helpers.c
diff --git a/include/linux/mm.h b/include/linux/mm.h
index cc292273e6ba..4bc93477375e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2637,7 +2637,6 @@ typedef int (*pte_fn_t)(pte_t *pte, unsigned long addr,
void *data);
extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
unsigned long size, pte_fn_t fn, void *data);
-
#ifdef CONFIG_PAGE_POISONING
extern bool page_poisoning_enabled(void);
extern void kernel_poison_pages(struct page *page, int numpages, int enable);
@@ -2878,5 +2877,17 @@ static inline int pages_identical(struct page *page1,
struct page *page2)
return !memcmp_pages(page1, page2);
}
+#ifdef CONFIG_MAPPING_DIRTY_HELPERS
+unsigned long clean_record_shared_mapping_range(struct address_space *mapping,
+ pgoff_t first_index, pgoff_t nr,
+ pgoff_t bitmap_pgoff,
+ unsigned long *bitmap,
+ pgoff_t *start,
+ pgoff_t *end);
+
+unsigned long wp_shared_mapping_range(struct address_space *mapping,
+ pgoff_t first_index, pgoff_t nr);
+#endif
+
#endif /* __KERNEL__ */
#endif /* _LINUX_MM_H */
diff --git a/mm/Kconfig b/mm/Kconfig
index a5dae9a7eb51..550f7aceb679 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -736,4 +736,7 @@ config ARCH_HAS_PTE_SPECIAL
config ARCH_HAS_HUGEPD
bool
+config MAPPING_DIRTY_HELPERS
+bool
+
endmenu
diff --git a/mm/Makefile b/mm/Makefile
index d996846697ef..1937cc251883 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -107,3 +107,4 @@ obj-$(CONFIG_PERCPU_STATS) += percpu-stats.o
obj-$(CONFIG_ZONE_DEVICE) += memremap.o
obj-$(CONFIG_HMM_MIRROR) += hmm.o
obj-$(CONFIG_MEMFD_CREATE) += memfd.o
+obj-$(CONFIG_MAPPING_DIRTY_HELPERS) += mapping_dirty_helpers.o
diff --git a/mm/mapping_dirty_helpers.c b/mm/mapping_dirty_helpers.c
new file mode 100644
index ..799b9154b48f
--- /dev/null
+++ b/mm/mapping_dirty_helpers.c
@@ -0,0 +1,312 @@
+// SPDX-License-Identifier: GPL-2.0
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * struct wp_walk - Private struct for pagetable walk callbacks
+ * @range: Range for mmu notifiers
+ * @tlbflush_start: Address of first modified pte
+ * @tlbflush_end: Address of last modified pte + 1
+ * @total: Total number of modified ptes
+ */
+struct wp_walk {
+ struct mmu_notifier_range range;
+ unsigned long tlbflush_start;
+ unsigned long tlbflush_end;
+ unsigned long total;
+};
+
+/**
+ * wp_pte - Write-protect a pte
+ * @pte: Pointer to the pte
+ * @addr: The virtual page address
+ * @walk: pagetable walk callback argument
+ *
+ * The function write-protects a pte and records the range in
+ * virtual address space of touched ptes for efficient range TLB flushes.
+ */
+static int wp_pte(pte_t *pte, unsigned long addr, unsigned long end,
+ struct mm_walk *walk)
+{
+ struct wp_walk *wpwalk = walk->private;
+ pte_t ptent = *pte;
+
+ if (pte_write(ptent)) {
+ pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);
+
+ ptent = pte_wrprotect(old_pte);
+ ptep_modify_prot_commit(walk->vma, addr, pte, old_pte, ptent);
+ wpwalk->total++;
+ wpwalk->tlbflush_start = min(wpwalk->tlbflush_start, addr);
+ wpwalk->tlbflush_end = max(wpwalk->tlbflush_end,
+ addr + PAGE_SIZE);
+ }
+
+ return 0;
+}
+
+/**
+ * struct clean_walk - Private struct for the clean_record_pte function.
+ * @base: struct wp_walk we derive from
+ * @bitmap_pgoff: Address_space Page
