Re: [PATCH v4 5/9] mm: Add write-protect and clean utilities for address space ranges
On 10/8/19 7:07 PM, Linus Torvalds wrote:
> On Tue, Oct 8, 2019 at 2:15 AM Thomas Hellström (VMware)
> wrote:
>> Add two utilities to 1) write-protect and 2) clean all ptes pointing into
>> a range of an address space.
> The code looks much simpler and easier to understand now, I think, but
> that also makes some thing more obvious..
>
>> +static int clean_record_pte(pte_t *pte, unsigned long addr,
>> + unsigned long end, struct mm_walk *walk)
>> +{
>> + struct wp_walk *wpwalk = walk->private;
>> + struct clean_walk *cwalk = to_clean_walk(wpwalk);
>> + pte_t ptent = *pte;
>> +
>> + if (pte_dirty(ptent)) {
>> + pgoff_t pgoff = ((addr - walk->vma->vm_start) >> PAGE_SHIFT)
>> +
>> + walk->vma->vm_pgoff - cwalk->bitmap_pgoff;
>> + pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);
>> +
>> + ptent = pte_mkclean(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);
>> +
>> + __set_bit(pgoff, cwalk->bitmap);
> The above looks fundamentally racy.
>
> You clean the pte in memory, but it remains dirty and writable in the
> TLB, and you only flush it much later.
>
> So now writes can continue to happen to that page, without it
> necessarily being marked dirty again in the page tables, because all
> the CPU TLB caches say "it's already dirty".
>
> This may be ok - you've moved the diry bit into that bitmap, and you
> will flush the TLB before then taking action on the bitmap. So you
> haven't really _lost_ any dirty bits, but it still may be worth a
> comment.
>
> You do have comments about the _other_ issues (ie the whole "If a
> caller needs to make sure all dirty ptes are picked up and none
> additional are added..") but you don't actually have comments about
> the TLB race basically potentially now causing "missing" dirty stuff.
>
> And this may actually be a big problem on some architectures. Not x86,
> and maybe nothing else, but I have this dim memory of some
> architectures being unhappy due to virtual caches, and writeback when
> the page table entry says it's read-only and clean.
>
> Our normal "clean pages" is very anal about this, and does
>
> flush_cache_page(vma, address, pte_pfn(*pte));
> entry = ptep_clear_flush(vma, address, pte);
> entry = pte_wrprotect(entry);
> entry = pte_mkclean(entry);
> set_pte_at(vma->vm_mm, address, pte, entry);
>
> when it cleans a page, and I note both the cache flush and the
> "clear_flush()" (which invalidates the pte and does a synchronous TLB
> flush) and we have magic semantics for that at least on s390 because
> there are some low-level architecture details wrt flushing TLB entries
> and modifying them.
>
> End result: I think the code is probably ok in practice because you
> don't mind the slightly fuzzy dirty bit state, and it's _probably_ ok
> on all architectures that use drm for the TLB invalidation side. But I
> think it bears a bit of thinking about.
Yes, there's been some considerable thinking behind this. We do have the
cache flush in the pre_vma callback, and as you mention the TLB flush
happens before we use the bitmap: any pte that may be subject to a race
is recorded in the bitmap, and the guarantee we want to provide with the
function is
a) All ptes that are dirtied before the function starts executing will
be recorded in the bitmap.
b) All ptes dirtied after that will either be recorded in the bitmap,
remain dirty or both.
I probably should add that in the docs.
And the actual writeback happens asynchronously a lot later *should* be
OK, as long as caches are synced for the dma operation. Of course if
this somehow fails on a particular architecture I guess we need to
rethink and use ptep_clear_flush() at least on that architecture.
Thanks,
Thomas.
>
> Linus
>
Re: [PATCH v4 5/9] mm: Add write-protect and clean utilities for address space ranges
On Tue, Oct 8, 2019 at 2:15 AM Thomas Hellström (VMware)
wrote:
>
> Add two utilities to 1) write-protect and 2) clean all ptes pointing into
> a range of an address space.
The code looks much simpler and easier to understand now, I think, but
that also makes some thing more obvious..
> +static int clean_record_pte(pte_t *pte, unsigned long addr,
> + unsigned long end, struct mm_walk *walk)
> +{
> + struct wp_walk *wpwalk = walk->private;
> + struct clean_walk *cwalk = to_clean_walk(wpwalk);
> + pte_t ptent = *pte;
> +
> + if (pte_dirty(ptent)) {
> + pgoff_t pgoff = ((addr - walk->vma->vm_start) >> PAGE_SHIFT) +
> + walk->vma->vm_pgoff - cwalk->bitmap_pgoff;
> + pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);
> +
> + ptent = pte_mkclean(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);
> +
> + __set_bit(pgoff, cwalk->bitmap);
The above looks fundamentally racy.
You clean the pte in memory, but it remains dirty and writable in the
TLB, and you only flush it much later.
So now writes can continue to happen to that page, without it
necessarily being marked dirty again in the page tables, because all
the CPU TLB caches say "it's already dirty".
This may be ok - you've moved the diry bit into that bitmap, and you
will flush the TLB before then taking action on the bitmap. So you
haven't really _lost_ any dirty bits, but it still may be worth a
comment.
You do have comments about the _other_ issues (ie the whole "If a
caller needs to make sure all dirty ptes are picked up and none
additional are added..") but you don't actually have comments about
the TLB race basically potentially now causing "missing" dirty stuff.
And this may actually be a big problem on some architectures. Not x86,
and maybe nothing else, but I have this dim memory of some
architectures being unhappy due to virtual caches, and writeback when
the page table entry says it's read-only and clean.
Our normal "clean pages" is very anal about this, and does
flush_cache_page(vma, address, pte_pfn(*pte));
entry = ptep_clear_flush(vma, address, pte);
entry = pte_wrprotect(entry);
entry = pte_mkclean(entry);
set_pte_at(vma->vm_mm, address, pte, entry);
when it cleans a page, and I note both the cache flush and the
"clear_flush()" (which invalidates the pte and does a synchronous TLB
flush) and we have magic semantics for that at least on s390 because
there are some low-level architecture details wrt flushing TLB entries
and modifying them.
End result: I think the code is probably ok in practice because you
don't mind the slightly fuzzy dirty bit state, and it's _probably_ ok
on all architectures that use drm for the TLB invalidation side. But I
think it bears a bit of thinking about.
Linus
[PATCH v4 5/9] 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 | 308 +
4 files changed, 324 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 ..10572e558154
--- /dev/null
+++ b/mm/mapping_dirty_helpers.c
@@ -0,0 +1,308 @@
+// 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
