mm: relaxed TLB flushes and other optimi.
Subsystems (11)
cover mm: relaxed TLB flushes and other optimi.
From: Nadav Amit <[email protected]>
Following the optimizations to avoid unnecessary TLB flushes [1], mprotect() and userfaultfd() did not cause unnecessary TLB flushes when protection was unchanged. This enabled userfaultfd to write-unprotect a page without triggering a TLB flush (and potentially shootdown).
After these changes, David added another feature to mprotect [2], allowing pages that can safely be mapped as writable, to be mapped as such directly from mprotect(), instead of going through the page fault handler. This saves the overhead of a page-fault when write-unprotecting private exclusive pages as writable, for instance.
This change introduced, however, some undesired behaviors, especially if we adopt this new feature for userfaultfd. First, the newly mapped PTE is not set as dirty, which might induce on x86 over 500 cycles of overhead (if the page was not dirty before). Second, once again we can have an expensive TLB shootdown when we write-unprotect a page: when we relax the protection (i.e., give more permission), we would do a TLB flush. If the application is multithreaded, or a userfaultfd monitor uses write-unprotect (which is a common case), a TLB shootdown would be needed.
This patch-set allows userfaultfd to map pages as writeable directly upon write-(un)protect ioctl, while addressing the undesired behaviors that occur when one uses userfaultfd write-unprotect or mprotect to add permissions. It also does some cleanup and micro-optimizations along the way.
The main change that is done in the patch-set - x86 specific, at the moment - is the introduction of "relaxed" TLB flushes when permissions are added. Upon a "relaxed" TLB flush, the mm's TLB generation is advanced and the local TLB is flushed, but no TLB shootdown takes place. If a spurious page-fault occurs and the local generation of the TLB is found to be out-of-sync with the mm generation, a full TLB flush is performed on the faulting core to prevent further spurious page-faults.
To a certain extent "relaxed flushes" are similar to the changes that were proposed some time ago for kernel mappings [3]. However, it does not have any complicated interactions with with NMI handlers.
Experiments on Haswell show the performance improvement. Running, for a single page, a loop of (1) mprotect(READ); (2) mprotect(READ|WRITE) and then (3) access provides the following result (on bare metal this time):
mprotect(PROT_READ) time in cycles:
1 Thread 2 Threads
Before (5.19rc4+) 2499 4655
+patch 2495 4363 (-6%)
mprotect(PROT_READ|PROT_WRITE) in cycles:
1 Thread 2 Threads
Before (5.19rc4+) 2529 4675
+patch 2496 2615 (-44%)
If we ran MADV_FREE or the page was not dirty, we can also shorten the PROT_READ time by skipping the TLB shootdown with this patch-set.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
[3] https://lore.kernel.org/all/[email protected]/
Nadav Amit (14):
userfaultfd: set dirty and young on writeprotect
userfaultfd: try to map write-unprotected pages mm/mprotect: allow exclusive anon pages to be writable mm/mprotect: preserve write with MM_CP_TRY_CHANGE_WRITABLE x86/mm: check exec permissions on fault
mm/rmap: avoid flushing on page_vma_mkclean_one() when possible
mm: do fix spurious page-faults for instruction faults x86/mm: introduce flush_tlb_fix_spurious_fault
mm: introduce relaxed TLB flushes
x86/mm: introduce relaxed TLB flushes
x86/mm: use relaxed TLB flushes when protection is removed x86/tlb: no flush on PTE change from RW->RO when PTE is clean mm/mprotect: do not check flush type if a strict is needed
mm: conditional check of pfn in pte_flush_type
arch/x86/include/asm/pgtable.h | 4 +- arch/x86/include/asm/tlb.h | 3 +- arch/x86/include/asm/tlbflush.h | 90 +++++++++++++++++-------- arch/x86/kernel/alternative.c | 2 +- arch/x86/kernel/ldt.c | 3 +- arch/x86/mm/fault.c | 22 +++++- arch/x86/mm/tlb.c | 21 +++++- include/asm-generic/tlb.h | 116 +++++++++++++++++++------------- include/linux/mm.h | 2 + include/linux/mm_types.h | 6 ++ mm/huge_memory.c | 9 ++- mm/hugetlb.c | 2 +- mm/memory.c | 2 +- mm/mmu_gather.c | 1 + mm/mprotect.c | 31 ++++++--- mm/rmap.c | 16 +++-- mm/userfaultfd.c | 10 ++- 17 files changed, 237 insertions(+), 103 deletions(-)
Signature · 1 line
2.25.1
RFC PATCH 1/14 userfaultfd: set dirty and young on writeprotect
From: Nadav Amit <[email protected]>
When userfaultfd makes a PTE writable, it can now change the PTE directly, in some cases, without going triggering a page-fault first. Yet, doing so might leave the PTE that was write-unprotected as old and clean. At least on x86, this would cause a >500 cycles overhead when the PTE is first accessed.
Use MM_CP_WILL_NEED to set the PTE as young and dirty when userfaultfd gets a hint that the page is likely to be used. Avoid changing the PTE to young and dirty in other cases to avoid excessive writeback and messing with the page reclamation logic.
--- include/linux/mm.h | 2 ++ mm/mprotect.c | 9 ++++++++- mm/userfaultfd.c | 8 ++++++-- 3 files changed, 16 insertions(+), 3 deletions(-)
include/linux/mm.h +2 -0
@@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,/* Whether this change is for write protecting */ #define MM_CP_UFFD_WP (1UL << 2)/* do wp */ #define MM_CP_UFFD_WP_RESOLVE (1UL << 3)/* Resolve wp */ +/* Whether to try to mark entries as dirty as they are to be written */ +#define MM_CP_WILL_NEED (1UL << 4)#define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ MM_CP_UFFD_WP_RESOLVE)
mm/mprotect.c +8 -1
@@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +bool will_need = cp_flags &MM_CP_WILL_NEED ;tlb_change_page_size (tlb,PAGE_SIZE );
@@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, ptent =pte_clear_uffd_wp (ptent); } +if (will_need) + ptent =pte_mkyoung (ptent); +/* * In some writable, shared mappings, we might want * to catch actual write access -- see
@@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,*/ if ((cp_flags &MM_CP_TRY_CHANGE_WRITABLE ) && !pte_write (ptent) && -can_change_pte_writable (vma, addr, ptent)) +can_change_pte_writable (vma, addr, ptent)) { ptent =pte_mkwrite (ptent); +if (will_need) + ptent =pte_mkdirty (ptent); + }ptep_modify_prot_commit (vma, addr, pte, oldpte, ptent);if (pte_needs_flush (oldpte, ptent))
mm/userfaultfd.c +6 -2
@@ -749,6 +749,7 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,bool enable_wp = uffd_flags &UFFD_FLAGS_WP ;struct vm_area_struct *dst_vma;unsigned long page_mask; +unsigned long cp_flags;struct mmu_gather tlb;pgprot_t newprot;int err;
@@ -795,9 +796,12 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,else newprot =vm_get_page_prot (dst_vma->vm_flags); + cp_flags = enable_wp ?MM_CP_UFFD_WP :MM_CP_UFFD_WP_RESOLVE ; +if (uffd_flags & (UFFD_FLAGS_ACCESS_LIKELY |UFFD_FLAGS_WRITE_LIKELY )) + cp_flags |=MM_CP_WILL_NEED ; +tlb_gather_mmu (&tlb, dst_mm); -change_protection (&tlb, dst_vma, start, start + len, newprot, - enable_wp ?MM_CP_UFFD_WP :MM_CP_UFFD_WP_RESOLVE ); +change_protection (&tlb, dst_vma, start, start + len, newprot, cp_flags);tlb_finish_mmu (&tlb); err =0 ;
Signature · 1 line
2.25.1
Peter Xu
reviewerRe: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 57 lines · Nadav Amit
From: Nadav Amit <[email protected]>
When userfaultfd makes a PTE writable, it can now change the PTE directly, in some cases, without going triggering a page-fault first. Yet, doing so might leave the PTE that was write-unprotected as old and clean. At least on x86, this would cause a >500 cycles overhead when the PTE is first accessed.
Use
MM_CP_WILL_NEEDto set the PTE as young and dirty when userfaultfd gets a hint that the page is likely to be used. Avoid changing the PTE to young and dirty in other cases to avoid excessive writeback and messing with the page reclamation logic.Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>--- include/linux/mm.h | 2 ++ mm/mprotect.c | 9 ++++++++- mm/userfaultfd.c | 8 ++++++-- 3 files changed, 16 insertions(+), 3 deletions(-)include/linux/mm.h +2
-0@@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,/* Whether this change is for write protecting */ #define MM_CP_UFFD_WP (1UL << 2)/* do wp */ #define MM_CP_UFFD_WP_RESOLVE (1UL << 3)/* Resolve wp */ +/* Whether to try to mark entries as dirty as they are to be written */ +#define MM_CP_WILL_NEED (1UL << 4)#define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ MM_CP_UFFD_WP_RESOLVE)mm/mprotect.c +1
-0@@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +bool will_need = cp_flags &MM_CP_WILL_NEED ;tlb_change_page_size (tlb,PAGE_SIZE );
mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 177
Quoted 5 lines
mm/mprotect.c +2
-0@@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, ptent =pte_clear_uffd_wp (ptent); } +if (will_need) + ptent =pte_mkyoung (ptent);
For uffd path, UFFD_FLAGS_ACCESS_LIKELY|UFFD_FLAGS_WRITE_LIKELY are new internal flags used with or without the new feature bit set. It means even with !ACCESS_HINT we'll start to set young bit while we used not to? Is that some kind of a light abi change?
I'd suggest we only set will_need if ACCESS_HINT is set.
mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 197
Quoted 13 lines
mm/mprotect.c +4
-1+/* * In some writable, shared mappings, we might want * to catch actual write access -- see @@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,*/ if ((cp_flags &MM_CP_TRY_CHANGE_WRITABLE ) && !pte_write (ptent) && -can_change_pte_writable (vma, addr, ptent)) +can_change_pte_writable (vma, addr, ptent)) { ptent =pte_mkwrite (ptent); +if (will_need) + ptent =pte_mkdirty (ptent);
Can we make this unconditional? IOW to cover both:
(1) When will_need is not set, or
(2) mprotect() too
David's patch is good in that we merged the unprotect and CoW. However that's not complete because the dirty bit ops are missing.
Here IMHO we should have a standalone patch to just add the dirty bit into this logic when we'll grant write bit. IMHO it'll make the write+dirty bits coherent again in all paths.
Quoted 34 lines
mm/mprotect.c +1
-0+ }ptep_modify_prot_commit (vma, addr, pte, oldpte, ptent);if (pte_needs_flush (oldpte, ptent))mm/userfaultfd.c +6
-2@@ -749,6 +749,7 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,bool enable_wp = uffd_flags &UFFD_FLAGS_WP ;struct vm_area_struct *dst_vma;unsigned long page_mask; +unsigned long cp_flags;struct mmu_gather tlb;pgprot_t newprot;int err;@@ -795,9 +796,12 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,else newprot =vm_get_page_prot (dst_vma->vm_flags); + cp_flags = enable_wp ?MM_CP_UFFD_WP :MM_CP_UFFD_WP_RESOLVE ; +if (uffd_flags & (UFFD_FLAGS_ACCESS_LIKELY |UFFD_FLAGS_WRITE_LIKELY )) + cp_flags |=MM_CP_WILL_NEED ; +tlb_gather_mmu (&tlb, dst_mm); -change_protection (&tlb, dst_vma, start, start + len, newprot, - enable_wp ?MM_CP_UFFD_WP :MM_CP_UFFD_WP_RESOLVE ); +change_protection (&tlb, dst_vma, start, start + len, newprot, cp_flags);tlb_finish_mmu (&tlb); err =0 ;Signature · 1 line
2.25.1
Signature · 1 line
Peter Xu
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 90 lines · Peter Xu
Quoted 57 lines · Nadav Amit
From: Nadav Amit <[email protected]>
When userfaultfd makes a PTE writable, it can now change the PTE directly, in some cases, without going triggering a page-fault first. Yet, doing so might leave the PTE that was write-unprotected as old and clean. At least on x86, this would cause a >500 cycles overhead when the PTE is first accessed.
Use
MM_CP_WILL_NEEDto set the PTE as young and dirty when userfaultfd gets a hint that the page is likely to be used. Avoid changing the PTE to young and dirty in other cases to avoid excessive writeback and messing with the page reclamation logic.Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>--- include/linux/mm.h | 2 ++ mm/mprotect.c | 9 ++++++++- mm/userfaultfd.c | 8 ++++++-- 3 files changed, 16 insertions(+), 3 deletions(-)include/linux/mm.h +2
-0@@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,/* Whether this change is for write protecting */ #define MM_CP_UFFD_WP (1UL << 2)/* do wp */ #define MM_CP_UFFD_WP_RESOLVE (1UL << 3)/* Resolve wp */ +/* Whether to try to mark entries as dirty as they are to be written */ +#define MM_CP_WILL_NEED (1UL << 4)#define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ MM_CP_UFFD_WP_RESOLVE)mm/mprotect.c +1
-0@@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +bool will_need = cp_flags &MM_CP_WILL_NEED ;tlb_change_page_size (tlb,PAGE_SIZE );mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 177
Quoted 5 lines
mm/mprotect.c +2
-0@@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, ptent =pte_clear_uffd_wp (ptent); } +if (will_need) + ptent =pte_mkyoung (ptent);For uffd path, UFFD_FLAGS_ACCESS_LIKELY|UFFD_FLAGS_WRITE_LIKELY are new internal flags used with or without the new feature bit set. It means even with !ACCESS_HINT we'll start to set young bit while we used not to? Is that some kind of a light abi change?
I'd suggest we only set
will_needifACCESS_HINTis set.mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 197
Quoted 13 lines
mm/mprotect.c +4
-1+/* * In some writable, shared mappings, we might want * to catch actual write access -- see @@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,*/ if ((cp_flags &MM_CP_TRY_CHANGE_WRITABLE ) && !pte_write (ptent) && -can_change_pte_writable (vma, addr, ptent)) +can_change_pte_writable (vma, addr, ptent)) { ptent =pte_mkwrite (ptent); +if (will_need) + ptent =pte_mkdirty (ptent);Can we make this unconditional? IOW to cover both:
(1) When
will_needis not set, or
(2)mprotect()too
Quoted 6 lines
David's patch is good in that we merged the unprotect and CoW. However that's not complete because the dirty bit ops are missing.
Here IMHO we should have a standalone patch to just add the dirty bit into this logic when we'll grant write bit. IMHO it'll make the write+dirty bits coherent again in all paths.
I'm not sure I follow.
We *surely* don't want to dirty random pages (especially once in the pagecache/swapcache) simply because we change protection.
Just like we don't set all pages write+dirty in a writable VMA on a read fault.
Signature · 3 lines
Thanks, David / dhildenb
Peter Xu
reviewerRe: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 98 lines · David Hildenbrand
Quoted 90 lines · Peter Xu
Quoted 57 lines · Nadav Amit
From: Nadav Amit <[email protected]>
When userfaultfd makes a PTE writable, it can now change the PTE directly, in some cases, without going triggering a page-fault first. Yet, doing so might leave the PTE that was write-unprotected as old and clean. At least on x86, this would cause a >500 cycles overhead when the PTE is first accessed.
Use
MM_CP_WILL_NEEDto set the PTE as young and dirty when userfaultfd gets a hint that the page is likely to be used. Avoid changing the PTE to young and dirty in other cases to avoid excessive writeback and messing with the page reclamation logic.Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>--- include/linux/mm.h | 2 ++ mm/mprotect.c | 9 ++++++++- mm/userfaultfd.c | 8 ++++++-- 3 files changed, 16 insertions(+), 3 deletions(-)include/linux/mm.h +2
-0@@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,/* Whether this change is for write protecting */ #define MM_CP_UFFD_WP (1UL << 2)/* do wp */ #define MM_CP_UFFD_WP_RESOLVE (1UL << 3)/* Resolve wp */ +/* Whether to try to mark entries as dirty as they are to be written */ +#define MM_CP_WILL_NEED (1UL << 4)#define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ MM_CP_UFFD_WP_RESOLVE)mm/mprotect.c +1
-0@@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +bool will_need = cp_flags &MM_CP_WILL_NEED ;tlb_change_page_size (tlb,PAGE_SIZE );mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 177
Quoted 5 lines
mm/mprotect.c +2
-0@@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, ptent =pte_clear_uffd_wp (ptent); } +if (will_need) + ptent =pte_mkyoung (ptent);For uffd path, UFFD_FLAGS_ACCESS_LIKELY|UFFD_FLAGS_WRITE_LIKELY are new internal flags used with or without the new feature bit set. It means even with !ACCESS_HINT we'll start to set young bit while we used not to? Is that some kind of a light abi change?
I'd suggest we only set
will_needifACCESS_HINTis set.mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 197
Quoted 13 lines
mm/mprotect.c +4
-1+/* * In some writable, shared mappings, we might want * to catch actual write access -- see @@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,*/ if ((cp_flags &MM_CP_TRY_CHANGE_WRITABLE ) && !pte_write (ptent) && -can_change_pte_writable (vma, addr, ptent)) +can_change_pte_writable (vma, addr, ptent)) { ptent =pte_mkwrite (ptent); +if (will_need) + ptent =pte_mkdirty (ptent);Can we make this unconditional? IOW to cover both:
(1) When
will_needis not set, or
(2)mprotect()tooQuoted 6 lines
David's patch is good in that we merged the unprotect and CoW. However that's not complete because the dirty bit ops are missing.
Here IMHO we should have a standalone patch to just add the dirty bit into this logic when we'll grant write bit. IMHO it'll make the write+dirty bits coherent again in all paths.
Quoted 7 lines
I'm not sure I follow.
We *surely* don't want to dirty random pages (especially once in the pagecache/swapcache) simply because we change protection.
Just like we don't set all pages write+dirty in a writable VMA on a read fault.
IMO unmprotect (in generic mprotect form or uffd form) has a stronger sign of page being written, unlike read faults, as many of them happen because page being written and message generated.
But yeah you have a point too, maybe we shouldn't assume such a condition. Especially as long as we won't set write=1 without soft-dirty tracking enabled, I think it should be safe.
Signature · 1 line
Peter Xu
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 104 lines · Peter Xu
Quoted 103 lines · David Hildenbrand
Quoted 90 lines · Peter Xu
Quoted 57 lines · Nadav Amit
From: Nadav Amit <[email protected]>
When userfaultfd makes a PTE writable, it can now change the PTE directly, in some cases, without going triggering a page-fault first. Yet, doing so might leave the PTE that was write-unprotected as old and clean. At least on x86, this would cause a >500 cycles overhead when the PTE is first accessed.
Use
MM_CP_WILL_NEEDto set the PTE as young and dirty when userfaultfd gets a hint that the page is likely to be used. Avoid changing the PTE to young and dirty in other cases to avoid excessive writeback and messing with the page reclamation logic.Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>--- include/linux/mm.h | 2 ++ mm/mprotect.c | 9 ++++++++- mm/userfaultfd.c | 8 ++++++-- 3 files changed, 16 insertions(+), 3 deletions(-)include/linux/mm.h +2
-0@@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,/* Whether this change is for write protecting */ #define MM_CP_UFFD_WP (1UL << 2)/* do wp */ #define MM_CP_UFFD_WP_RESOLVE (1UL << 3)/* Resolve wp */ +/* Whether to try to mark entries as dirty as they are to be written */ +#define MM_CP_WILL_NEED (1UL << 4)#define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ MM_CP_UFFD_WP_RESOLVE)mm/mprotect.c +1
-0@@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +bool will_need = cp_flags &MM_CP_WILL_NEED ;tlb_change_page_size (tlb,PAGE_SIZE );mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 177
Quoted 5 lines
mm/mprotect.c +2
-0@@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, ptent =pte_clear_uffd_wp (ptent); } +if (will_need) + ptent =pte_mkyoung (ptent);For uffd path, UFFD_FLAGS_ACCESS_LIKELY|UFFD_FLAGS_WRITE_LIKELY are new internal flags used with or without the new feature bit set. It means even with !ACCESS_HINT we'll start to set young bit while we used not to? Is that some kind of a light abi change?
I'd suggest we only set
will_needifACCESS_HINTis set.mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 197
Quoted 13 lines
mm/mprotect.c +4
-1+/* * In some writable, shared mappings, we might want * to catch actual write access -- see @@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,*/ if ((cp_flags &MM_CP_TRY_CHANGE_WRITABLE ) && !pte_write (ptent) && -can_change_pte_writable (vma, addr, ptent)) +can_change_pte_writable (vma, addr, ptent)) { ptent =pte_mkwrite (ptent); +if (will_need) + ptent =pte_mkdirty (ptent);Can we make this unconditional? IOW to cover both:
(1) When
will_needis not set, or
(2)mprotect()tooQuoted 6 lines
David's patch is good in that we merged the unprotect and CoW. However that's not complete because the dirty bit ops are missing.
Here IMHO we should have a standalone patch to just add the dirty bit into this logic when we'll grant write bit. IMHO it'll make the write+dirty bits coherent again in all paths.
I'm not sure I follow.
We *surely* don't want to dirty random pages (especially once in the pagecache/swapcache) simply because we change protection.
Quoted 6 lines
Quoted 2 lines
Just like we don't set all pages write+dirty in a writable VMA on a read fault.
IMO unmprotect (in generic mprotect form or uffd form) has a stronger sign of page being written, unlike read faults, as many of them happen because page being written and message generated.
I'm sorry, but I am very skeptical about such statements. I don't buy it.
Quoted 4 lines
But yeah you have a point too, maybe we shouldn't assume such a condition. Especially as long as we won't set write=1 without soft-dirty tracking enabled, I think it should be safe.
For pagecache pages it may as well be *plain wrong* to bypass the write fault handler and simply mark pages dirty+map them writable.
Please, let's keep this protection change handler here as simple as possible and *not* try to replicate the whole write fault handler logic in here by relying on statements as above.
If we try optimizing for corner cases by making the implementation here overly complicated, then we are clearly doing something wrong.
Signature · 3 lines
Thanks, David / dhildenb
Peter Xu
reviewerRe: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 2 lines · David Hildenbrand
For pagecache pages it may as well be *plain wrong* to bypass the write fault handler and simply mark pages dirty+map them writable.
Could you elaborate?
Signature · 1 line
Peter Xu
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 5 lines · Peter Xu
Quoted 2 lines · David Hildenbrand
For pagecache pages it may as well be *plain wrong* to bypass the write fault handler and simply mark pages dirty+map them writable.
Could you elaborate?
Write-fault handling for some filesystems (that even require this "slow path") is a bit special.
For example, do_shared_fault() might have to call page_mkwrite().
AFAIK file systems use that for lazy allocation of disk blocks. If you simply go ahead and map a !dirty pagecache page writable and mark it dirty, it will not trigger page_mkwrite() and you might end up corrupting data.
That's why we the old change_pte_range() code never touched anything if the pte wasn't already dirty. Because as long as it's not writable, the FS might have to be informed about the write-unprotect.
And we end up in the case here for VM_SHARED with vma_wants_writenotify(). Where we, for example, check
/* The backer wishes to know when pages are first written to? * if (vm_ops && (vm_ops->page_mkwrite || vm_ops->pfn_mkwrite))$
return 1;
Long story short, we should be really careful with write-fault handler bypasses, especially when deciding to set dirty bits. For pagecache pages, we have to be especially careful.
For exclusive anon pages it's mostly ok, because wp_page_reuse() doesn't really contain that much magic. The only thing to consider for ordinary mprotect() is that there is -- IMHO -- absolutely no guarantee that someone will write to a specific write-unprotected page soon. For uffd-wp-unprotect it might be easier to guess, especially, if we un-protect only a single page.
Signature · 3 lines
Thanks, David / dhildenb
Peter Xu
reviewerRe: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 12 lines · David Hildenbrand
Quoted 5 lines · Peter Xu
Quoted 2 lines · David Hildenbrand
For pagecache pages it may as well be *plain wrong* to bypass the write fault handler and simply mark pages dirty+map them writable.
Could you elaborate?
Write-fault handling for some filesystems (that even require this "slow path") is a bit special.
For example,
do_shared_fault()might have to callpage_mkwrite().
Quoted 7 lines
AFAIK file systems use that for lazy allocation of disk blocks. If you simply go ahead and map a !dirty pagecache page writable and mark it dirty, it will not trigger
page_mkwrite()and you might end up corrupting data.That's why we the old
change_pte_range()code never touched anything if the pte wasn't already dirty.
I don't think that pte_dirty() check was for the pagecache code. For any fs that has page_mkwrite() defined, it'll already have vma_wants_writenotify() return 1, so we'll never try to add write bit, hence we'll never even try to check pte_dirty().
Quoted 6 lines
Because as long as it's not writable,
the FS might have to be informed about the write-unprotect.And we end up in the case here for
VM_SHAREDwithvma_wants_writenotify(). Where we, for example, check
Quoted 8 lines
/* The backer wishes to know when pages are first written to? * if (
vm_ops&& (vm_ops->page_mkwrite|| vm_ops->pfn_mkwrite))$return 1;Long story short, we should be really careful with write-fault handler bypasses, especially when deciding to set dirty bits. For pagecache pages, we have to be especially careful.
Since you mentioned page_mkwrite, IMHO it's really the write bit not dirty bit that matters here (and IMHO that's why it's called page_mkwrite() not page_mkdirty()). Here Nadav's patch added pte_mkdirty() only if pte_mkwrite() happens. So I'm a bit confused on what's your worry, and what you're against doing.
Say, even if with my original proposal to set dirty unconditionally, it'll be still be after the pte_mkwrite(). I never see how that could affect page_mkwrite not to mention it'll not even reach there.
Quoted 6 lines
For exclusive anon pages it's mostly ok, because
wp_page_reuse()doesn't really contain that much magic. The only thing to consider for ordinarymprotect()is that there is -- IMHO -- absolutely no guarantee that someone will write to a specific write-unprotected page soon. For uffd-wp-unprotect it might be easier to guess, especially, if we un-protect only a single page.
Yeh, as mentioned I think that's a valid point - looks good to me to attach the dirty bit only when with a hint.
Thanks,
Signature · 1 line
Peter Xu
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 18 lines · Peter Xu
Quoted 17 lines · David Hildenbrand
Quoted 5 lines · Peter Xu
Quoted 2 lines · David Hildenbrand
For pagecache pages it may as well be *plain wrong* to bypass the write fault handler and simply mark pages dirty+map them writable.
Could you elaborate?
Write-fault handling for some filesystems (that even require this "slow path") is a bit special.
For example,
do_shared_fault()might have to callpage_mkwrite().AFAIK file systems use that for lazy allocation of disk blocks. If you simply go ahead and map a !dirty pagecache page writable and mark it dirty, it will not trigger
page_mkwrite()and you might end up corrupting data.
Quoted 8 lines
Quoted 2 lines
That's why we the old
change_pte_range()code never touched anything if the pte wasn't already dirty.I don't think that
pte_dirty()check was for the pagecache code. For any fs that haspage_mkwrite()defined, it'll already havevma_wants_writenotify()return 1, so we'll never try to add write bit, hence we'll never even try to checkpte_dirty().
I might be too tired, but the whole reason we had this magic before my commit in place was only for the pagecache.
With vma_wants_writenotify()=0 you can directly map the pages writable and don't have to do these advanced checks here. In a writable MAP_SHARED VMA you'll already have pte_write().
We only get !pte_write() in case we have vma_wants_writenotify()=1 ...
try_change_writable = vma_wants_writenotify(vma, vma->vm_page_prot);
and that's the code that checked the dirty bit after all to decide -- amongst other things -- if we can simply map it writable without going via the write fault handler and triggering do_shared_fault() .
See crazy/ugly FOLL_FORCE code in GUP that similarly checks the dirty bit.
But yeah, it's all confusing so I might just be wrong regarding pagecache pages.
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 36 lines · David Hildenbrand
Quoted 18 lines · Peter Xu
Quoted 17 lines · David Hildenbrand
Quoted 5 lines · Peter Xu
Quoted 2 lines · David Hildenbrand
For pagecache pages it may as well be *plain wrong* to bypass the write fault handler and simply mark pages dirty+map them writable.
Could you elaborate?
Write-fault handling for some filesystems (that even require this "slow path") is a bit special.
For example,
do_shared_fault()might have to callpage_mkwrite().AFAIK file systems use that for lazy allocation of disk blocks. If you simply go ahead and map a !dirty pagecache page writable and mark it dirty, it will not trigger
page_mkwrite()and you might end up corrupting data.Quoted 7 lines
Quoted 2 lines
That's why we the old
change_pte_range()code never touched anything if the pte wasn't already dirty.I don't think that
pte_dirty()check was for the pagecache code. For any fs that haspage_mkwrite()defined, it'll already havevma_wants_writenotify()return 1, so we'll never try to add write bit, hence we'll never even try to checkpte_dirty().I might be too tired, but the whole reason we had this magic before my commit in place was only for the pagecache.
With vma_wants_writenotify()=0 you can directly map the pages writable and don't have to do these advanced checks here. In a writable
MAP_SHAREDVMA you'll already havepte_write().We only get !pte_write() in case we have vma_wants_writenotify()=1 ...
Quoted 7 lines
try_change_writable=vma_wants_writenotify(vma, vma->vm_page_prot);and that's the code that checked the dirty bit after all to decide -- amongst other things -- if we can simply map it writable without going via the write fault handler and triggering
do_shared_fault().See crazy/ugly
FOLL_FORCEcode in GUP that similarly checks the dirty bit.
I thought you want to get rid of it at least for anonymous pages. No?
Quoted 3 lines
But yeah, it's all confusing so I might just be wrong regarding pagecache pages.
Just to note: I am not very courageous and I did not intend to change condition for when non-anonymous pages are set as writable. That’s the reason I did not change the dirty for non-writable non-anonymous entries (as Peter said). And that’s the reason that setting the dirty bit (at least as I should have done it) is only performed after we made the decision on the write-bit.
IOW, after you made your decision about the write-bit, then and only then you may be able to set the dirty bit for writable entries. Since the entry is already writeable (i.e., can be written without a fault later directly from userspace), there should be no concern of correctness when you set it.
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 40 lines · Nadav Amit
Quoted 38 lines · David Hildenbrand
Quoted 18 lines · Peter Xu
Quoted 17 lines · David Hildenbrand
Quoted 5 lines · Peter Xu
Quoted 2 lines · David Hildenbrand
For pagecache pages it may as well be *plain wrong* to bypass the write fault handler and simply mark pages dirty+map them writable.
Could you elaborate?
Write-fault handling for some filesystems (that even require this "slow path") is a bit special.
For example,
do_shared_fault()might have to callpage_mkwrite().AFAIK file systems use that for lazy allocation of disk blocks. If you simply go ahead and map a !dirty pagecache page writable and mark it dirty, it will not trigger
page_mkwrite()and you might end up corrupting data.Quoted 7 lines
Quoted 2 lines
That's why we the old
change_pte_range()code never touched anything if the pte wasn't already dirty.I don't think that
pte_dirty()check was for the pagecache code. For any fs that haspage_mkwrite()defined, it'll already havevma_wants_writenotify()return 1, so we'll never try to add write bit, hence we'll never even try to checkpte_dirty().I might be too tired, but the whole reason we had this magic before my commit in place was only for the pagecache.
With vma_wants_writenotify()=0 you can directly map the pages writable and don't have to do these advanced checks here. In a writable
MAP_SHAREDVMA you'll already havepte_write().We only get !pte_write() in case we have vma_wants_writenotify()=1 ...
try_change_writable=vma_wants_writenotify(vma, vma->vm_page_prot);
Quoted 7 lines
Quoted 5 lines
and that's the code that checked the dirty bit after all to decide -- amongst other things -- if we can simply map it writable without going via the write fault handler and triggering
do_shared_fault().See crazy/ugly
FOLL_FORCEcode in GUP that similarly checks the dirty bit.I thought you want to get rid of it at least for anonymous pages. No?
Yes. Especially for any MAP_PRIVATE mappings.
If you want to write to something that's not mapped writable in a MAP_PRIVATE mapping it
a) Has to be an exclusive anonymous page
b) The pte has to be dirty
In any other case, you clearly missed to COW or the modifications might get lost if the PTE is not dirty.
MAP_SHARED is a bit more involved. But whether the pte is dirty might be good enough ... but this needs a lot more care.
Quoted 11 lines
Quoted 4 lines
But yeah, it's all confusing so I might just be wrong regarding pagecache pages.
Just to note: I am not very courageous and I did not intend to change condition for when non-anonymous pages are set as writable. That’s the reason I did not change the dirty for non-writable non-anonymous entries (as Peter said). And that’s the reason that setting the dirty bit (at least as I should have done it) is only performed after we made the decision on the write-bit.
Good. As long as we stick to anonymous pages I roughly know what we we can and cannot do at this point :)
The problem I see is that detection whether we can write requires the dirty bit ... and whether to set the dirty bit requires the information whether we can write.
Again, for anonymous pages we should be able to relax the "dirty" requirement when detecting whether we can write.
Quoted 5 lines
IOW, after you made your decision about the write-bit, then and only then you may be able to set the dirty bit for writable entries. Since the entry is already writeable (i.e., can be written without a fault later directly from userspace), there should be no concern of correctness when you set it.
That makes sense to me. What keeps confusing me are architectures with and without a hw-managed dirty bit ... :)
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 47 lines · David Hildenbrand
Quoted 46 lines · Nadav Amit
Quoted 43 lines · David Hildenbrand
Quoted 18 lines · Peter Xu
Quoted 17 lines · David Hildenbrand
Quoted 5 lines · Peter Xu
Quoted 2 lines · David Hildenbrand
For pagecache pages it may as well be *plain wrong* to bypass the write fault handler and simply mark pages dirty+map them writable.
Could you elaborate?
Write-fault handling for some filesystems (that even require this "slow path") is a bit special.
For example,
do_shared_fault()might have to callpage_mkwrite().AFAIK file systems use that for lazy allocation of disk blocks. If you simply go ahead and map a !dirty pagecache page writable and mark it dirty, it will not trigger
page_mkwrite()and you might end up corrupting data.Quoted 7 lines
Quoted 2 lines
That's why we the old
change_pte_range()code never touched anything if the pte wasn't already dirty.I don't think that
pte_dirty()check was for the pagecache code. For any fs that haspage_mkwrite()defined, it'll already havevma_wants_writenotify()return 1, so we'll never try to add write bit, hence we'll never even try to checkpte_dirty().I might be too tired, but the whole reason we had this magic before my commit in place was only for the pagecache.
With vma_wants_writenotify()=0 you can directly map the pages writable and don't have to do these advanced checks here. In a writable
MAP_SHAREDVMA you'll already havepte_write().We only get !pte_write() in case we have vma_wants_writenotify()=1 ...
try_change_writable=vma_wants_writenotify(vma, vma->vm_page_prot);and that's the code that checked the dirty bit after all to decide -- amongst other things -- if we can simply map it writable without going via the write fault handler and triggering
do_shared_fault().See crazy/ugly
FOLL_FORCEcode in GUP that similarly checks the dirty bit.
Quoted 8 lines
Quoted 1 line
I thought you want to get rid of it at least for anonymous pages. No?
Yes. Especially for any
MAP_PRIVATEmappings.If you want to write to something that's not mapped writable in a
MAP_PRIVATEmapping it
a) Has to be an exclusive anonymous page
b) The pte has to be dirty
Do you need both conditions to be true? I thought (a) is sufficient (if the soft-dirty and related checks succeed).
Quoted 21 lines
In any other case, you clearly missed to COW or the modifications might get lost if the PTE is not dirty.
MAP_SHAREDis a bit more involved. But whether the pte is dirty might be good enough ... but this needs a lot more care.Quoted 9 lines
Quoted 2 lines
But yeah, it's all confusing so I might just be wrong regarding pagecache pages.
Just to note: I am not very courageous and I did not intend to change condition for when non-anonymous pages are set as writable. That’s the reason I did not change the dirty for non-writable non-anonymous entries (as Peter said). And that’s the reason that setting the dirty bit (at least as I should have done it) is only performed after we made the decision on the write-bit.
Good. As long as we stick to anonymous pages I roughly know what we we can and cannot do at this point :)
Quoted 6 lines
The problem I see is that detection whether we can write requires the dirty bit ... and whether to set the dirty bit requires the information whether we can write.
Again, for anonymous pages we should be able to relax the "dirty" requirement when detecting whether we can write.
That’s all I wanted to do there.
Quoted 8 lines
Quoted 5 lines
IOW, after you made your decision about the write-bit, then and only then you may be able to set the dirty bit for writable entries. Since the entry is already writeable (i.e., can be written without a fault later directly from userspace), there should be no concern of correctness when you set it.
That makes sense to me. What keeps confusing me are architectures with and without a hw-managed dirty bit ... :)
I don’t know which arch you have in your mind. But the moment a PTE is writable, then marking it logically/architecturally as dirty should be fine.
But… if the Exclusive check is not good enough for private+anon without the “logical” dirty bit, then there would be a problem.
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 9 lines
Quoted 6 lines
Yes. Especially for any
MAP_PRIVATEmappings.If you want to write to something that's not mapped writable in a
MAP_PRIVATEmapping it
a) Has to be an exclusive anonymous page
b) The pte has to be dirtyDo you need both conditions to be true? I thought (a) is sufficient (if the soft-dirty and related checks succeed).
If we force-write to a page, we need it to be dirty to tell reclaim code that the content stale. We can either mark the pte dirty manually, or just let the write fault handler deal with it to simplify GUP code. This needs some more thought, but that's my understanding.
Quoted 40 lines
Quoted 22 lines
In any other case, you clearly missed to COW or the modifications might get lost if the PTE is not dirty.
MAP_SHAREDis a bit more involved. But whether the pte is dirty might be good enough ... but this needs a lot more care.Quoted 9 lines
Quoted 2 lines
But yeah, it's all confusing so I might just be wrong regarding pagecache pages.
Just to note: I am not very courageous and I did not intend to change condition for when non-anonymous pages are set as writable. That’s the reason I did not change the dirty for non-writable non-anonymous entries (as Peter said). And that’s the reason that setting the dirty bit (at least as I should have done it) is only performed after we made the decision on the write-bit.
Good. As long as we stick to anonymous pages I roughly know what we we can and cannot do at this point :)
Quoted 6 lines
The problem I see is that detection whether we can write requires the dirty bit ... and whether to set the dirty bit requires the information whether we can write.
Again, for anonymous pages we should be able to relax the "dirty" requirement when detecting whether we can write.
That’s all I wanted to do there.
Quoted 8 lines
Quoted 5 lines
IOW, after you made your decision about the write-bit, then and only then you may be able to set the dirty bit for writable entries. Since the entry is already writeable (i.e., can be written without a fault later directly from userspace), there should be no concern of correctness when you set it.
That makes sense to me. What keeps confusing me are architectures with and without a hw-managed dirty bit ... :)
Quoted 6 lines
I don’t know which arch you have in your mind. But the moment a PTE is writable, then marking it logically/architecturally as dirty should be fine.
But… if the Exclusive check is not good enough for private+anon without the “logical” dirty bit, then there would be a problem.
I think we are good for anonymous pages.
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 7 lines · David Hildenbrand
Quoted 7 lines
Quoted 6 lines
Yes. Especially for any
MAP_PRIVATEmappings.If you want to write to something that's not mapped writable in a
MAP_PRIVATEmapping it
a) Has to be an exclusive anonymous page
b) The pte has to be dirty
Quoted 7 lines
Quoted 2 lines
Do you need both conditions to be true? I thought (a) is sufficient (if the soft-dirty and related checks succeed).
If we force-write to a page, we need it to be dirty to tell reclaim code that the content stale. We can either mark the pte dirty manually, or just let the write fault handler deal with it to simplify GUP code. This needs some more thought, but that's my understanding.
Extending on my previous answer after staring at the code
a) I have to dig if the FOLL_FORCE special-retry-handling is required for MAP_SHARED at all.
check_vma_flags() allows FOLL_FORCE only on MAP_PRIVATE VMAs that lack VM_WRITE.
Consequently, I would have assumed that the first write fault should be sufficient on a MAP_SHARED VMA to actually map the PTE writable and not require any of that special retry magic.
b) I wonder if we have to take care of uffd-wp and softdirty (just like in mprotect code here) as well in case we stumble over an exclusive anonymous page. Yes, the VMA is currently not writable, but I'd have expected at least softdirty tracking to apply.
... I'll dig into the details.
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
In reply to Nadav AmitQuoted 67 lines · Nadav Amit
From: Nadav Amit <[email protected]>
When userfaultfd makes a PTE writable, it can now change the PTE directly, in some cases, without going triggering a page-fault first. Yet, doing so might leave the PTE that was write-unprotected as old and clean. At least on x86, this would cause a >500 cycles overhead when the PTE is first accessed.
Use
MM_CP_WILL_NEEDto set the PTE as young and dirty when userfaultfd gets a hint that the page is likely to be used. Avoid changing the PTE to young and dirty in other cases to avoid excessive writeback and messing with the page reclamation logic.Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>--- include/linux/mm.h | 2 ++ mm/mprotect.c | 9 ++++++++- mm/userfaultfd.c | 8 ++++++-- 3 files changed, 16 insertions(+), 3 deletions(-)include/linux/mm.h +2
-0@@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,/* Whether this change is for write protecting */ #define MM_CP_UFFD_WP (1UL << 2)/* do wp */ #define MM_CP_UFFD_WP_RESOLVE (1UL << 3)/* Resolve wp */ +/* Whether to try to mark entries as dirty as they are to be written */ +#define MM_CP_WILL_NEED (1UL << 4)#define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ MM_CP_UFFD_WP_RESOLVE)mm/mprotect.c +4
-0@@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +bool will_need = cp_flags &MM_CP_WILL_NEED ;tlb_change_page_size (tlb,PAGE_SIZE );@@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, ptent =pte_clear_uffd_wp (ptent); } +if (will_need) + ptent =pte_mkyoung (ptent); +/* * In some writable, shared mappings, we might want * to catch actual write access -- see
mm/mprotect.c static unsigned long change_pte_range(struct mmu_gather *tlb, · line 193
Quoted 3 lines
mm/mprotect.c +0
-0@@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,*/ if ((cp_flags & MM_CP_TRY_CHANGE_WRITABLE) && !pte_write(ptent) &&
Why would we want to check if we can set something writable if it already *is* writable? That doesn't make sense to me.
Quoted 6 lines
mm/mprotect.c +4
-1- can_change_pte_writable(vma, addr, ptent)) + can_change_pte_writable(vma, addr, ptent)) { ptent =pte_mkwrite (ptent); +if (will_need) + ptent =pte_mkdirty (ptent); + }
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 63 lines · David Hildenbrand
⚠ External Email
Quoted 60 lines · Nadav Amit
From: Nadav Amit <[email protected]>
When userfaultfd makes a PTE writable, it can now change the PTE directly, in some cases, without going triggering a page-fault first. Yet, doing so might leave the PTE that was write-unprotected as old and clean. At least on x86, this would cause a >500 cycles overhead when the PTE is first accessed.
Use
MM_CP_WILL_NEEDto set the PTE as young and dirty when userfaultfd gets a hint that the page is likely to be used. Avoid changing the PTE to young and dirty in other cases to avoid excessive writeback and messing with the page reclamation logic.Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>---
include/linux/mm.h| 2 ++mm/mprotect.c| 9 ++++++++-mm/userfaultfd.c| 8 ++++++--3 files changed, 16 insertions(+), 3 deletions(-)include/linux/mm.h +2
-0@@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,/* Whether this change is for write protecting */ #define MM_CP_UFFD_WP (1UL << 2)/* do wp */ #define MM_CP_UFFD_WP_RESOLVE (1UL << 3)/* Resolve wp */ +/* Whether to try to mark entries as dirty as they are to be written */ +#define MM_CP_WILL_NEED (1UL << 4)#define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ MM_CP_UFFD_WP_RESOLVE)mm/mprotect.c +1
-0@@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +bool will_need = cp_flags &MM_CP_WILL_NEED ;tlb_change_page_size (tlb,PAGE_SIZE );@@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, ptent =pte_clear_uffd_wp (ptent); }
Quoted 14 lines
Quoted 10 lines
mm/mprotect.c +3
-0+if (will_need) + ptent =pte_mkyoung (ptent); +/* * In some writable, shared mappings, we might want * to catch actual write access -- see @@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,*/ if ((
cp_flags&MM_CP_TRY_CHANGE_WRITABLE) &&
!pte_write(ptent) &&Why would we want to check if we can set something writable if it already *is* writable? That doesn't make sense to me.
We check !pte_write(). What am I missing in your question?
Having said that, I do notice now that pte_mkdirty() should not be done only this condition is fulfilled. Instead we should just have something like:
if (will_need) {
ptent = pte_mkyoung(ptent);
if (pte_write(ptent))
ptent = pte_mkdirty(ptent);
}But I do not think this answers your question, which I did not understand.
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 80 lines · Nadav Amit
Quoted 63 lines · David Hildenbrand
⚠ External Email
Quoted 60 lines · Nadav Amit
From: Nadav Amit <[email protected]>
When userfaultfd makes a PTE writable, it can now change the PTE directly, in some cases, without going triggering a page-fault first. Yet, doing so might leave the PTE that was write-unprotected as old and clean. At least on x86, this would cause a >500 cycles overhead when the PTE is first accessed.
Use
MM_CP_WILL_NEEDto set the PTE as young and dirty when userfaultfd gets a hint that the page is likely to be used. Avoid changing the PTE to young and dirty in other cases to avoid excessive writeback and messing with the page reclamation logic.Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>---
include/linux/mm.h| 2 ++mm/mprotect.c| 9 ++++++++-mm/userfaultfd.c| 8 ++++++--3 files changed, 16 insertions(+), 3 deletions(-)include/linux/mm.h +2
-0@@ -1988,6 +1988,8 @@ extern unsigned long move_page_tables(struct vm_area_struct *vma,/* Whether this change is for write protecting */ #define MM_CP_UFFD_WP (1UL << 2)/* do wp */ #define MM_CP_UFFD_WP_RESOLVE (1UL << 3)/* Resolve wp */ +/* Whether to try to mark entries as dirty as they are to be written */ +#define MM_CP_WILL_NEED (1UL << 4)#define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \ MM_CP_UFFD_WP_RESOLVE)mm/mprotect.c +1
-0@@ -82,6 +82,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +bool will_need = cp_flags &MM_CP_WILL_NEED ;tlb_change_page_size (tlb,PAGE_SIZE );@@ -172,6 +173,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, ptent =pte_clear_uffd_wp (ptent); }Quoted 14 lines
Quoted 10 lines
mm/mprotect.c +3
-0+if (will_need) + ptent =pte_mkyoung (ptent); +/* * In some writable, shared mappings, we might want * to catch actual write access -- see @@ -187,8 +191,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,*/ if ((
cp_flags&MM_CP_TRY_CHANGE_WRITABLE) &&
!pte_write(ptent) &&Why would we want to check if we can set something writable if it already *is* writable? That doesn't make sense to me.
Quoted 1 line
We check !pte_write(). What am I missing in your question?
My patch review skills have seen better days. I thought you'd be removing the pte_write() check ... :( Tired eyes ...
Quoted 10 lines
Having said that, I do notice now that
pte_mkdirty()should not be done only this condition is fulfilled. Instead we should just have something like:if (will_need) { ptent = pte_mkyoung(ptent); if (pte_write(ptent)) ptent = pte_mkdirty(ptent); }
As can_change_pte_writable() will fail if it stumbles over a !pte_dirty page in current code ... so I assume you would have that code before the actual pte_mkwrite() logic, correct?
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 13 lines · David Hildenbrand
My patch review skills have seen better days. I thought you'd be removing the
pte_write()check ... :( Tired eyes ...Quoted 9 lines
Having said that, I do notice now that
pte_mkdirty()should not be done only this condition is fulfilled. Instead we should just have something like:if (
will_need) {
ptent =pte_mkyoung(ptent);
if (pte_write(ptent))
ptent =pte_mkdirty(ptent);
}
Quoted 3 lines
As
can_change_pte_writable()will fail if it stumbles over a !pte_dirty page in current code ... so I assume you would have that code before the actualpte_mkwrite()logic, correct?
No, I thought this should go after for 2 reasons:
1. You want to allow the PTE to be made writable following the can_change_pte_writable().
2. You do not want to set a non-writable PTE as dirty, especially since it might then be used to determine that the PTE can become writable. Doing so would circumvent cases in which set_page_dirty() needs to be called and break things down.
Re: [RFC PATCH 01/14] userfaultfd: set dirty and young on writeprotect
Quoted 21 lines · Nadav Amit
Quoted 13 lines · David Hildenbrand
My patch review skills have seen better days. I thought you'd be removing the
pte_write()check ... :( Tired eyes ...Quoted 9 lines
Having said that, I do notice now that
pte_mkdirty()should not be done only this condition is fulfilled. Instead we should just have something like:if (
will_need) {
ptent =pte_mkyoung(ptent);
if (pte_write(ptent))
ptent =pte_mkdirty(ptent);
}Quoted 3 lines
As
can_change_pte_writable()will fail if it stumbles over a !pte_dirty page in current code ... so I assume you would have that code before the actualpte_mkwrite()logic, correct?No, I thought this should go after for 2 reasons:
Quoted 7 lines
1. You want to allow the PTE to be made writable following the
can_change_pte_writable().2. You do not want to set a non-writable PTE as dirty, especially since it might then be used to determine that the PTE can become writable. Doing so would circumvent cases in which
set_page_dirty()needs to be called and break things down.
The I'm confused how can_change_pte_writable() would ever allow for that. Best to show me the code :)
Signature · 3 lines
Thanks, David / dhildenb
RFC PATCH 2/14 userfaultfd: try to map write-unprotected pages
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
When using userfaultfd write-(un)protect ioctl, try to change the PTE to be writable. This would save a page-fault afterwards.
--- mm/userfaultfd.c | 2 ++ 1 file changed, 2 insertions(+)
mm/userfaultfd.c +2 -0
@@ -799,6 +799,8 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start, cp_flags = enable_wp ?MM_CP_UFFD_WP :MM_CP_UFFD_WP_RESOLVE ;if (uffd_flags & (UFFD_FLAGS_ACCESS_LIKELY |UFFD_FLAGS_WRITE_LIKELY )) cp_flags |=MM_CP_WILL_NEED ; +if (!enable_wp && (uffd_flags &UFFD_FLAGS_WRITE_LIKELY )) + cp_flags |=MM_CP_TRY_CHANGE_WRITABLE ;tlb_gather_mmu (&tlb, dst_mm);change_protection (&tlb, dst_vma, start, start + len, newprot, cp_flags);
Signature · 1 line
2.25.1
Re: [RFC PATCH 02/14] userfaultfd: try to map write-unprotected pages
Quoted 5 lines · Nadav Amit
From: Nadav Amit <[email protected]>
When using userfaultfd write-(un)protect ioctl, try to change the PTE to be writable. This would save a page-fault afterwards.
Quoted 13 lines
Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>Signed-off-by Nadav Amit <[email protected]>
Signature · 1 line
Peter Xu
RFC PATCH 3/14 mm/mprotect: allow exclusive anon pages to be writable
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
Anonymous pages might have the dirty bit clear, but this should not prevent mprotect from making them writable if they are exclusive. Therefore, skip the test whether the page is dirty in this case.
--- mm/mprotect.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
mm/mprotect.c +3 -2
@@ -45,7 +45,7 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,VM_BUG_ON (!(vma->vm_flags &VM_WRITE ) ||pte_write (pte)); -if (pte_protnone (pte) || !pte_dirty (pte)) +if (pte_protnone (pte))return false;/* Do we need write faults for softdirty tracking? */
@@ -66,7 +66,8 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma, page =vm_normal_page (vma, addr, pte);if (!page || !PageAnon (page) || !PageAnonExclusive (page))return false; - } + }else if (!pte_dirty (pte)) +return false;return true; }
Signature · 1 line
2.25.1
Re: [RFC PATCH 03/14] mm/mprotect: allow exclusive anon pages to be writable
Quoted 37 lines · Nadav Amit
From: Nadav Amit <[email protected]>
Anonymous pages might have the dirty bit clear, but this should not prevent mprotect from making them writable if they are exclusive. Therefore, skip the test whether the page is dirty in this case.
Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>Signed-off-by Nadav Amit <[email protected]>--- mm/mprotect.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)mm/mprotect.c +1
-1@@ -45,7 +45,7 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,VM_BUG_ON (!(vma->vm_flags &VM_WRITE ) ||pte_write (pte)); -if (pte_protnone (pte) || !pte_dirty (pte)) +if (pte_protnone (pte))return false;/* Do we need write faults for softdirty tracking? */
mm/mprotect.c static inline bool can_change_pte_writable(struct vm_area_struct *vma, · line 73
Quoted 9 lines
mm/mprotect.c +2
-1@@ -66,7 +66,8 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma, page =vm_normal_page (vma, addr, pte);if (!page || !PageAnon (page) || !PageAnonExclusive (page))return false; - } + }else if (!pte_dirty (pte)) +return false;return true; }
When I wrote that code, I was wondering how often that would actually happen in practice -- and if we care about optimizing that. Do you have a gut feeling in which scenarios this would happen and if we care?
If the page is in the swapcache and was swapped out, you'd be requiring a writeback even though nobody modified the page and possibly isn't going to do so in the near future.
Signature · 3 lines
Thanks, David / dhildenb
Re: [RFC PATCH 03/14] mm/mprotect: allow exclusive anon pages to be writable
Quoted 52 lines · David Hildenbrand
Quoted 37 lines · Nadav Amit
From: Nadav Amit <[email protected]>
Anonymous pages might have the dirty bit clear, but this should not prevent mprotect from making them writable if they are exclusive. Therefore, skip the test whether the page is dirty in this case.
Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>Signed-off-by Nadav Amit <[email protected]>---
mm/mprotect.c| 5 +++--1 file changed, 3 insertions(+), 2 deletions(-)mm/mprotect.c +1
-1@@ -45,7 +45,7 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,VM_BUG_ON (!(vma->vm_flags &VM_WRITE ) ||pte_write (pte)); -if (pte_protnone (pte) || !pte_dirty (pte)) +if (pte_protnone (pte))return false;/* Do we need write faults for softdirty tracking? */ mm/mprotect.c static inline bool can_change_pte_writable(struct vm_area_struct *vma, · line 73
Quoted 9 lines
mm/mprotect.c +2
-1@@ -66,7 +66,8 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma, page =vm_normal_page (vma, addr, pte);if (!page || !PageAnon (page) || !PageAnonExclusive (page))return false; - } + }else if (!pte_dirty (pte)) +return false;return true; }When I wrote that code, I was wondering how often that would actually happen in practice -- and if we care about optimizing that. Do you have a gut feeling in which scenarios this would happen and if we care?
Quoted 3 lines
If the page is in the swapcache and was swapped out, you'd be requiring a writeback even though nobody modified the page and possibly isn't going to do so in the near future.
So here is my due diligence: I did not really encounter a scenario in which it showed up. When I looked at your code, I assumed this was an oversight and not a thoughtful decision. For me the issue is more of the discrepancy between how a certain page is handled before and after it was pages out.
The way that I see it, there is a tradeoff in the way dirty-bit should be handled:
(1) Writable-clean PTEs introduce some non-negligible overhead.
(2) Marking a PTE dirty speculatively would require a write back.
… But this tradeoff should not affect whether a PTE is writable, i.e., mapping the PTE as writable-clean should not cause a writeback. In other words, if you are concerned about unnecessary writebacks, which I think is a fair concern, then do not set the dirty-bit. In a later patch I try to avoid TLB flushes on clean-writable entries that are write-protected.
So I do not think that the writeback you mentioned should be a real issue. Yet if you think that using the fact that the page is not-dirty is a good hueristics to avoid future TLB flushes (for P->NP; as I said there is a solution for RW->RO), or if you are concerned about the cost of vm_normal_page(), perhaps those are valid concerned (although I do not think so).
--
[ Regarding (1): After some discussions with Peter and reading more code, I thought at some point that perhaps avoiding having writable-clean PTE as much as possible makes sense [*], since setting the dirty-bit costs ~550 cycles and a page fault is not a lot more than 1000. But with all the mitigations (and after adding IBRS for retbless) page-fault entry is kind of expensive.
[*] At least on x86 ]
Re: [RFC PATCH 03/14] mm/mprotect: allow exclusive anon pages to be writable
Quoted 74 lines · Nadav Amit
Quoted 52 lines · David Hildenbrand
Quoted 37 lines · Nadav Amit
From: Nadav Amit <[email protected]>
Anonymous pages might have the dirty bit clear, but this should not prevent mprotect from making them writable if they are exclusive. Therefore, skip the test whether the page is dirty in this case.
Cc Andrea Arcangeli <[email protected]>Cc Andrew Cooper <[email protected]>Cc Andrew Morton <[email protected]>Cc Andy Lutomirski <[email protected]>Cc Dave Hansen <[email protected]>Cc David Hildenbrand <[email protected]>Cc Peter Xu <[email protected]>Cc Peter Zijlstra <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Will Deacon <[email protected]>Cc Yu Zhao <[email protected]>Cc Nick Piggin <[email protected]>Signed-off-by Nadav Amit <[email protected]>---
mm/mprotect.c| 5 +++--1 file changed, 3 insertions(+), 2 deletions(-)mm/mprotect.c +1
-1@@ -45,7 +45,7 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma,VM_BUG_ON (!(vma->vm_flags &VM_WRITE ) ||pte_write (pte)); -if (pte_protnone (pte) || !pte_dirty (pte)) +if (pte_protnone (pte))return false;/* Do we need write faults for softdirty tracking? */ mm/mprotect.c static inline bool can_change_pte_writable(struct vm_area_struct *vma, · line 73
Quoted 9 lines
mm/mprotect.c +2
-1@@ -66,7 +66,8 @@ static inline bool can_change_pte_writable(struct vm_area_struct *vma, page =vm_normal_page (vma, addr, pte);if (!page || !PageAnon (page) || !PageAnonExclusive (page))return false; - } + }else if (!pte_dirty (pte)) +return false;return true; }When I wrote that code, I was wondering how often that would actually happen in practice -- and if we care about optimizing that. Do you have a gut feeling in which scenarios this would happen and if we care?
Quoted 3 lines
If the page is in the swapcache and was swapped out, you'd be requiring a writeback even though nobody modified the page and possibly isn't going to do so in the near future.
So here is my due diligence: I did not really encounter a scenario in which it showed up. When I looked at your code, I assumed this was an oversight and not a thoughtful decision. For me the issue is more of the discrepancy between how a certain page is handled before and after it was pages out.
The way that I see it, there is a tradeoff in the way dirty-bit should be handled:
(1) Writable-clean PTEs introduce some non-negligible overhead.
(2) Marking a PTE dirty speculatively would require a write back.… But this tradeoff should not affect whether a PTE is writable, i.e., mapping the PTE as writable-clean should not cause a writeback. In other words, if you are concerned about unnecessary writebacks, which I think is a fair concern, then do not set the dirty-bit. In a later patch I try to avoid TLB flushes on clean-writable entries that are write-protected.
Quoted 6 lines
So I do not think that the writeback you mentioned should be a real issue. Yet if you think that using the fact that the page is not-dirty is a good hueristics to avoid future TLB flushes (for
P->NP; as I said there is a solution forRW->RO), or if you are concerned about the cost ofvm_normal_page(), perhaps those are valid concerned (although I do not think so).
I think I now understand what you mean. I somehow remembered that some architectures set a PTE dirty when marking it writable, but I guess this is not true -- and setting it writable will keep it !dirty until really accessed (either by the HW or by a fault). [I'll do some more digging just to confirm]
With that in mind, your patch makes sense, and I guess you'd want that as patch #1, because otherwise I fail to see how current patch #1 would even succeed in reaching the "pte_mkdirty" call -- if !pte_dirty() protects the code from running.
Quoted 9 lines
--
[ Regarding (1): After some discussions with Peter and reading more code, I thought at some point that perhaps avoiding having writable-clean PTE as much as possible makes sense [*], since setting the dirty-bit costs ~550 cycles and a page fault is not a lot more than 1000. But with all the mitigations (and after adding IBRS for retbless) page-fault entry is kind of expensive.
I understand the reasoning for anonymous memory, but not for page cache pages. And for anonymous memory I think there are still cases where we don't want to do that (swapcache and MADV_FREE comes to mind) -- and IMHO some of the scenarios where we have clean anonymous memory at all, we just don't care about optimizing for setting the dirty bit faster on x86 ;) .
So my gut feeling is to keep it as simple as possible. To me, it translates to setting the dirty bit only if we have clear indication that write access is likely next.
Thanks Nadav for refreshing my memory :) !
Signature · 3 lines
Thanks, David / dhildenb
RFC PATCH 4/14 mm/mprotect: preserve write with MM_CP_TRY_CHANGE_WRITABLE
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
When MM_CP_TRY_CHANGE_WRITABLE is used, change_pte_range() tries to set PTEs as writable.
Yet, writable PTEs might still become read-only, due to various limitations of the logic that determines whether a PTE can become writable (see can_change_pte_writable()). Anyhow, it is much easier to keep the writable bit set when MM_CP_TRY_CHANGE_WRITABLE is used than to first clear it and then figure out whether it can be set again.
Preserve the write-bit when MM_CP_TRY_CHANGE_WRITABLE is used, similarly to the way it is done with NUMA.
--- mm/mprotect.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
mm/mprotect.c +4 -3
@@ -84,6 +84,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ;bool will_need = cp_flags &MM_CP_WILL_NEED ; +bool try_change_writable = cp_flags &MM_CP_TRY_CHANGE_WRITABLE ;tlb_change_page_size (tlb,PAGE_SIZE );
@@ -114,7 +115,8 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, oldpte = *pte;if (pte_present (oldpte)) {pte_t ptent; -bool preserve_write = prot_numa &&pte_write (oldpte); +bool preserve_write = (prot_numa || try_change_writable) && +pte_write (oldpte);/* * Avoid trapping faults against the zero or KSM
@@ -190,8 +192,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,* example, if a PTE is already dirty and no other * COW or special handling is required. */ -if ((cp_flags &MM_CP_TRY_CHANGE_WRITABLE ) && - !pte_write (ptent) && +if (try_change_writable && !pte_write (ptent) &&can_change_pte_writable (vma, addr, ptent)) { ptent =pte_mkwrite (ptent);if (will_need)
Signature · 1 line
2.25.1
RFC PATCH 5/14 x86/mm: check exec permissions on fault
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
access_error() currently does not check for execution permission violation. As a result, spurious page-faults due to execution permission violation cause SIGSEGV.
It appears not to be an issue so far, but the next patches avoid TLB flushes on permission promotion, which can lead to this scenario. nodejs for instance crashes when TLB flush is avoided on permission promotion.
Add a check to prevent access_error() from returning mistakenly that spurious page-faults due to instruction fetch are a reason for an access error.
It is assumed that error code bits of "instruction fetch" and "write" in the hardware error code are mutual exclusive, and the change assumes so. However, to be on the safe side, especially if hypervisors misbehave, assert this is the case and warn otherwise.
--- arch/x86/mm/fault.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)
arch/x86/mm/fault.c +20 -2
@@ -1107,10 +1107,28 @@ access_error(unsigned long error_code, struct vm_area_struct *vma) (error_code & X86_PF_INSTR), foreign))return 1 ; -if (error_code &X86_PF_WRITE ) { +if (error_code & (X86_PF_WRITE |X86_PF_INSTR )) { +/* +* CPUs are not expected to set the two error code bits +* together, but to ensure that hypervisors do not misbehave, +* run an additional sanity check. +*/ +if ((error_code & (X86_PF_WRITE |X86_PF_INSTR )) == + (X86_PF_WRITE |X86_PF_INSTR )) { +WARN_ON_ONCE (1 ); +return 1 ; + } +/* write, present and write, not present: */ -if (unlikely (!(vma->vm_flags &VM_WRITE ))) +if ((error_code &X86_PF_WRITE ) && +unlikely (!(vma->vm_flags &VM_WRITE ))) +return 1 ; + +/* exec, present and exec, not present: */ +if ((error_code &X86_PF_INSTR ) && +unlikely (!(vma->vm_flags &VM_EXEC )))return 1 ; +return 0 ; }
Signature · 1 line
2.25.1
RFC PATCH 6/14 mm/rmap: avoid flushing on page_vma_mkclean_one() when possible
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
x86 is capable to avoid TLB flush on clean writable entries. page_vma_mkclean_one() does not take advantage of this behavior. Adapt it.
--- mm/rmap.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
mm/rmap.c +12 -4
@@ -961,17 +961,25 @@ static int page_vma_mkclean_one(struct page_vma_mapped_walk *pvmw) address = pvmw->address;if (pvmw->pte) { -pte_t entry; +pte_t entry, oldpte;pte_t *pte = pvmw->pte;if (!pte_dirty (*pte) && !pte_write (*pte))continue ;flush_cache_page (vma, address,pte_pfn (*pte)); - entry =ptep_clear_flush (vma, address, pte); - entry =pte_wrprotect (entry); + oldpte =ptep_modify_prot_start (pvmw->vma, address, + pte); + + entry =pte_wrprotect (oldpte); entry =pte_mkclean (entry); -set_pte_at (vma->vm_mm, address, pte, entry); + +if (pte_needs_flush (oldpte, entry) || +mm_tlb_flush_pending (vma->vm_mm)) +flush_tlb_page (vma, address); + +ptep_modify_prot_commit (vma, address, pte, oldpte, + entry); ret =1 ; }else {#ifdef CONFIG_TRANSPARENT_HUGEPAGE
Signature · 1 line
2.25.1
RFC PATCH 7/14 mm: do fix spurious page-faults for instruction faults
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
The next patches might cause spurious instruction faults on x86. To prevent them from occurring too much, call flush_tlb_fix_spurious_fault() for page-faults on code fetching as well. The callee is expected to do a full flush, or whatever is necessary to avoid further TLB flushes.
--- mm/memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memory.c +1 -1
@@ -4924,7 +4924,7 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)* This still avoids useless tlb flushes for .text page faults * with threads. */ -if (vmf->flags &FAULT_FLAG_WRITE ) +if (vmf->flags & (FAULT_FLAG_WRITE |FAULT_FLAG_INSTRUCTION ))flush_tlb_fix_spurious_fault (vmf->vma, vmf->address); } unlock:
Signature · 1 line
2.25.1
RFC PATCH 8/14 x86/mm: introduce flush_tlb_fix_spurious_fault
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
The next patches introduce relaxed TLB flushes for x86, which would require a full TLB flush upon spurious page-fault. If a spurious page-fault occurs on x86, check if the local TLB generation is out of sync and perform a TLB flush if needed.
--- arch/x86/include/asm/pgtable.h | 4 +++- arch/x86/mm/tlb.c | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-)
arch/x86/include/asm/pgtable.h +3 -1
@@ -1079,7 +1079,9 @@ static inline void ptep_set_wrprotect(struct mm_struct *mm,clear_bit (_PAGE_BIT_RW, (unsigned long *)&ptep->pte); } -#define flush_tlb_fix_spurious_fault (vma, address) do { } while (0) +extern void flush_tlb_fix_spurious_fault (struct vm_area_struct *vma, +unsigned long address); +#define flush_tlb_fix_spurious_fault flush_tlb_fix_spurious_fault#define mk_pmd (page, pgprot) pfn_pmd(page_to_pfn(page), (pgprot))
arch/x86/mm/tlb.c +17 -0
@@ -955,6 +955,23 @@ static void put_flush_tlb_info(void)#endif } +void flush_tlb_fix_spurious_fault (struct vm_area_struct *vma, +unsigned long address) +{ +u32 loaded_mm_asid =this_cpu_read (cpu_tlbstate.loaded_mm_asid); +u64 mm_tlb_gen =atomic64_read (&vma->vm_mm->context.tlb_gen); +u64 local_tlb_gen =this_cpu_read (cpu_tlbstate.ctxs[loaded_mm_asid].tlb_gen); +struct flush_tlb_info *info; + +if (local_tlb_gen == mm_tlb_gen) +return ; + +preempt_disable (); + info =get_flush_tlb_info (NULL ,0 ,TLB_FLUSH_ALL ,0 , false,0 ); +flush_tlb_func (info); +preempt_enable (); +} +void flush_tlb_mm_range (struct mm_struct *mm,unsigned long start,unsigned long end,unsigned int stride_shift,bool freed_tables)
Signature · 1 line
2.25.1
RFC PATCH 9/14 mm: introduce relaxed TLB flushes
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
Introduce the concept of strict and relaxed TLB flushes. Relaxed TLB flushes are TLB flushes that can be skipped but might lead to degraded performance. It is down to arch code (in the next patches) to deal with relaxed flushes correctly. One such behavior is flushing the local TLB eagerly and remote TLBs lazily.
Track whether a flush is strict in the mmu_gather struct and introduce the required constants for tracking.
--- arch/x86/include/asm/tlbflush.h | 41 ++++++------ include/asm-generic/tlb.h | 114 ++++++++++++++++++-------------- include/linux/mm_types.h | 6 ++ mm/huge_memory.c | 7 +- mm/hugetlb.c | 2 +- mm/mmu_gather.c | 1 + mm/mprotect.c | 8 ++- mm/rmap.c | 2 +- 8 files changed, 107 insertions(+), 74 deletions(-)
arch/x86/include/asm/tlbflush.h +21 -20
@@ -259,7 +259,7 @@ static inline void arch_tlbbatch_add_mm(struct arch_tlbflush_unmap_batch *batch,extern void arch_tlbbatch_flush (struct arch_tlbflush_unmap_batch *batch); -static inline bool pte_flags_need_flush (unsigned long oldflags, +static inline enum pte_flush_type pte_flags_flush_type (unsigned long oldflags,unsigned long newflags,bool ignore_access) {
@@ -290,71 +290,72 @@ static inline bool pte_flags_need_flush(unsigned long oldflags, diff &= ~_PAGE_ACCESSED;/* -* Did any of the 'flush_on_clear' flags was clleared set from between -* 'oldflags' and 'newflags'? +* Did any of the 'flush_on_clear' flags was cleared between 'oldflags' +* and 'newflags'? */ if (diff & oldflags & flush_on_clear) -return true; +return PTE_FLUSH_STRICT ;/* Flush on modified flags. */ if (diff & flush_on_change) -return true; +return PTE_FLUSH_STRICT ;/* Ensure there are no flags that were left behind */ if (IS_ENABLED (CONFIG_DEBUG_VM ) && (diff & ~(flush_on_clear | software_flags | flush_on_change))) {VM_WARN_ON_ONCE (1 ); -return true; +return PTE_FLUSH_STRICT ; } - return false; + return PTE_FLUSH_NONE; }/* -* pte_needs_flush() checks whether permissions were demoted and require a -* flush. It should only be used for userspace PTEs. +* pte_flush_type() checks whether permissions were demoted or promoted and +* whether a strict or relaxed TLB flush is need. It should only be used on +* userspace PTEs. */ -static inline bool pte_needs_flush (pte_t oldpte,pte_t newpte) +static inline enum pte_flush_type pte_flush_type (pte_t oldpte,pte_t newpte) {/* !PRESENT -> * ; no need for flush */ if (!(pte_flags (oldpte) & _PAGE_PRESENT)) -return false; +return PTE_FLUSH_NONE ;/* PFN changed ; needs flush */ if (pte_pfn (oldpte) !=pte_pfn (newpte)) -return true; +return PTE_FLUSH_STRICT ;/* * check PTE flags; ignore access-bit; see comment in * ptep_clear_flush_young(). */ -return pte_flags_need_flush (pte_flags (oldpte),pte_flags (newpte), +return pte_flags_flush_type (pte_flags (oldpte),pte_flags (newpte), true); } -#define pte_needs_flush pte_needs_flush +#define pte_flush_type pte_flush_type/* -* huge_pmd_needs_flush() checks whether permissions were demoted and require a +* huge_pmd_flush_type() checks whether permissions were demoted and require a * flush. It should only be used for userspace huge PMDs. */ -static inline bool huge_pmd_needs_flush (pmd_t oldpmd,pmd_t newpmd) +static inline enum pte_flush_type huge_pmd_flush_type (pmd_t oldpmd,pmd_t newpmd) {/* !PRESENT -> * ; no need for flush */ if (!(pmd_flags (oldpmd) & _PAGE_PRESENT)) -return false; +return PTE_FLUSH_NONE ;/* PFN changed ; needs flush */ if (pmd_pfn (oldpmd) !=pmd_pfn (newpmd)) -return true; +return PTE_FLUSH_STRICT ;/* * check PMD flags; do not ignore access-bit; see * pmdp_clear_flush_young(). */ -return pte_flags_need_flush (pmd_flags (oldpmd),pmd_flags (newpmd), +return pte_flags_flush_type (pmd_flags (oldpmd),pmd_flags (newpmd), false); } -#define huge_pmd_needs_flush huge_pmd_needs_flush +#define huge_pmd_flush_type huge_pmd_flush_type#endif /* !MODULE */
include/asm-generic/tlb.h +66 -48
199 lines not on this page · raw message
include/linux/mm_types.h +6 -0
@@ -698,6 +698,12 @@ extern void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm);extern void tlb_gather_mmu_fullmm (struct mmu_gather *tlb,struct mm_struct *mm);extern void tlb_finish_mmu (struct mmu_gather *tlb); +enum pte_flush_type { +PTE_FLUSH_NONE =0 ,/* not necessary */ +PTE_FLUSH_STRICT =1 ,/* required */ +PTE_FLUSH_RELAXED =2 ,/* can cause spurious page-faults */ +}; +struct vm_fault ;/**
mm/huge_memory.c +5 -2
@@ -1713,6 +1713,7 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,bool prot_numa = cp_flags &MM_CP_PROT_NUMA ;bool uffd_wp = cp_flags &MM_CP_UFFD_WP ;bool uffd_wp_resolve = cp_flags &MM_CP_UFFD_WP_RESOLVE ; +enum pte_flush_type flush_type;tlb_change_page_size (tlb,HPAGE_PMD_SIZE );
@@ -1815,8 +1816,10 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma, ret =HPAGE_PMD_NR ;set_pmd_at (mm, addr, pmd, entry); -if (huge_pmd_needs_flush (oldpmd, entry)) -tlb_flush_pmd_range (tlb, addr,HPAGE_PMD_SIZE ); + flush_type =huge_pmd_flush_type (oldpmd, entry); +if (flush_type !=PTE_FLUSH_NONE ) +tlb_flush_pmd_range (tlb, addr,HPAGE_PMD_SIZE , + flush_type ==PTE_FLUSH_STRICT );BUG_ON (vma_is_anonymous (vma) && !preserve_write &&pmd_write (entry)); unlock:
mm/hugetlb.c +1 -1
@@ -5022,7 +5022,7 @@ static void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct ptl =huge_pte_lock (h, mm, ptep);if (huge_pmd_unshare (mm, vma, &address, ptep)) {spin_unlock (ptl); -tlb_flush_pmd_range (tlb, address &PUD_MASK ,PUD_SIZE ); +tlb_flush_pmd_range (tlb, address &PUD_MASK ,PUD_SIZE , true); force_flush = true;continue ; }
mm/mmu_gather.c +1 -0
@@ -348,6 +348,7 @@ void tlb_finish_mmu(struct mmu_gather *tlb) tlb->fullmm =1 ;__tlb_reset_range (tlb); tlb->freed_tables =1 ; + tlb->strict =1 ; }tlb_flush_mmu (tlb);
mm/mprotect.c +6 -2
@@ -117,6 +117,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,pte_t ptent;bool preserve_write = (prot_numa || try_change_writable) &&pte_write (oldpte); +enum pte_flush_type flush_type;/* * Avoid trapping faults against the zero or KSM
@@ -200,8 +201,11 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, }ptep_modify_prot_commit (vma, addr, pte, oldpte, ptent); -if (pte_needs_flush (oldpte, ptent)) -tlb_flush_pte_range (tlb, addr,PAGE_SIZE ); + + flush_type =pte_flush_type (oldpte, ptent); +if (flush_type !=PTE_FLUSH_NONE ) +tlb_flush_pte_range (tlb, addr,PAGE_SIZE , + flush_type ==PTE_FLUSH_STRICT ); pages++; }else if (is_swap_pte (oldpte)) {swp_entry_t entry =pte_to_swp_entry (oldpte);
mm/rmap.c +1 -1
@@ -974,7 +974,7 @@ static int page_vma_mkclean_one(struct page_vma_mapped_walk *pvmw) entry =pte_wrprotect (oldpte); entry =pte_mkclean (entry); -if (pte_needs_flush (oldpte, entry) || +if (pte_flush_type (oldpte, entry) !=PTE_FLUSH_NONE ||mm_tlb_flush_pending (vma->vm_mm))flush_tlb_page (vma, address);
Signature · 1 line
2.25.1
RFC PATCH 10/14 x86/mm: introduce relaxed TLB flushes
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
Introduce relaxed TLB flushes in x86. When protection is removed from PTEs (i.e., PTEs become writeable or executable), relaxed TLB flushes would be used. Relaxed TLB flushes do flush the local TLB, but do not flush remote TLBs.
If later a spurious page-fault is encountered, and the local TLB generation is found to be out of sync with the mm's TLB generation, a full TLB flush takes place to prevent further spurious page-faults from occurring.
--- arch/x86/include/asm/tlb.h | 3 ++- arch/x86/include/asm/tlbflush.h | 9 +++++---- arch/x86/kernel/alternative.c | 2 +- arch/x86/kernel/ldt.c | 3 ++- arch/x86/mm/tlb.c | 4 ++-- 5 files changed, 12 insertions(+), 9 deletions(-)
arch/x86/include/asm/tlb.h +2 -1
@@ -20,7 +20,8 @@ static inline void tlb_flush(struct mmu_gather *tlb) end = tlb->end; } -flush_tlb_mm_range (tlb->mm, start, end, stride_shift, tlb->freed_tables); +flush_tlb_mm_range (tlb->mm, start, end, stride_shift, tlb->freed_tables, + tlb->strict); }/*
arch/x86/include/asm/tlbflush.h +5 -4
@@ -220,23 +220,24 @@ void flush_tlb_multi(const struct cpumask *cpumask,#endif #define flush_tlb_mm (mm) \ - flush_tlb_mm_range(mm, 0UL, TLB_FLUSH_ALL, 0UL, true) + flush_tlb_mm_range(mm, 0UL, TLB_FLUSH_ALL, 0UL, true, true)#define flush_tlb_range (vma, start, end) \ flush_tlb_mm_range((vma)->vm_mm, start, end, \ ((vma)->vm_flags & VM_HUGETLB) \ ? huge_page_shift(hstate_vma(vma)) \ - : PAGE_SHIFT, false) + : PAGE_SHIFT, false, true)extern void flush_tlb_all (void );extern void flush_tlb_mm_range (struct mm_struct *mm,unsigned long start,unsigned long end,unsigned int stride_shift, -bool freed_tables); +bool freed_tables,bool strict);extern void flush_tlb_kernel_range (unsigned long start,unsigned long end);static inline void flush_tlb_page (struct vm_area_struct *vma,unsigned long a) { -flush_tlb_mm_range (vma->vm_mm, a, a +PAGE_SIZE ,PAGE_SHIFT , false); +flush_tlb_mm_range (vma->vm_mm, a, a +PAGE_SIZE ,PAGE_SHIFT , false, + true); }static inline u64 inc_mm_tlb_gen (struct mm_struct *mm)
arch/x86/kernel/alternative.c +1 -1
@@ -1099,7 +1099,7 @@ static void *__text_poke(text_poke_f func, void *addr, const void *src, size_t l*/ flush_tlb_mm_range (poking_mm, poking_addr, poking_addr + (cross_page_boundary ?2 :1 ) *PAGE_SIZE , -PAGE_SHIFT , false); +PAGE_SHIFT , false, true);if (func == text_poke_memcpy) {/*
arch/x86/kernel/ldt.c +2 -1
@@ -372,7 +372,8 @@ static void unmap_ldt_struct(struct mm_struct *mm, struct ldt_struct *ldt) } va = (unsigned long )ldt_slot_va (ldt->slot); -flush_tlb_mm_range (mm, va, va + nr_pages *PAGE_SIZE ,PAGE_SHIFT , false); +flush_tlb_mm_range (mm, va, va + nr_pages *PAGE_SIZE ,PAGE_SHIFT , false, + true); }#else /* !CONFIG_PAGE_TABLE_ISOLATION */
arch/x86/mm/tlb.c +2 -2
@@ -974,7 +974,7 @@ void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma,void flush_tlb_mm_range (struct mm_struct *mm,unsigned long start,unsigned long end,unsigned int stride_shift, -bool freed_tables) +bool freed_tables,bool strict) {struct flush_tlb_info *info;u64 new_tlb_gen;
@@ -1000,7 +1000,7 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,* a local TLB flush is needed. Optimize this use-case by calling * flush_tlb_func_local() directly in this case. */ -if (cpumask_any_but (mm_cpumask (mm), cpu) < nr_cpu_ids) { +if (strict &&cpumask_any_but (mm_cpumask (mm), cpu) < nr_cpu_ids) {flush_tlb_multi (mm_cpumask (mm), info); }else if (mm ==this_cpu_read (cpu_tlbstate.loaded_mm)) {lockdep_assert_irqs_enabled ();
Signature · 1 line
2.25.1
RFC PATCH 11/14 x86/mm: use relaxed TLB flushes when protection is removed
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
When checking x86 PTE flags to determine whether a TLB flush is needed, determine whether a relaxed TLB flush is sufficient. If protection is added (NX removed or W added), indicate that a relaxed TLB flush would suffice.
--- arch/x86/include/asm/tlbflush.h | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
arch/x86/include/asm/tlbflush.h +16 -4
@@ -271,18 +271,23 @@ static inline enum pte_flush_type pte_flags_flush_type(unsigned long oldflags,* dirty/access bit if needed without a fault. */ const pteval_t flush_on_clear = _PAGE_DIRTY | _PAGE_PRESENT | - _PAGE_ACCESSED; + _PAGE_ACCESSED | _PAGE_RW; +const pteval_t flush_on_set = _PAGE_NX; +const pteval_t flush_on_set_relaxed = _PAGE_RW; +const pteval_t flush_on_clear_relaxed = _PAGE_NX;const pteval_t software_flags = _PAGE_SOFTW1 | _PAGE_SOFTW2 | _PAGE_SOFTW3 | _PAGE_SOFTW4; -const pteval_t flush_on_change = _PAGE_RW | _PAGE_USER | _PAGE_PWT | +const pteval_t flush_on_change = _PAGE_USER | _PAGE_PWT | _PAGE_PCD | _PAGE_PSE | _PAGE_GLOBAL | _PAGE_PAT | _PAGE_PAT_LARGE | _PAGE_PKEY_BIT0 | _PAGE_PKEY_BIT1 | - _PAGE_PKEY_BIT2 | _PAGE_PKEY_BIT3 | _PAGE_NX; + _PAGE_PKEY_BIT2 | _PAGE_PKEY_BIT3;unsigned long diff = oldflags ^ newflags;BUILD_BUG_ON (flush_on_clear & software_flags);BUILD_BUG_ON (flush_on_clear & flush_on_change);BUILD_BUG_ON (flush_on_change & software_flags); +BUILD_BUG_ON (flush_on_change & flush_on_clear_relaxed); +BUILD_BUG_ON (flush_on_change & flush_on_set_relaxed);/* Ignore software flags */ diff &= ~software_flags;
@@ -301,9 +306,16 @@ static inline enum pte_flush_type pte_flags_flush_type(unsigned long oldflags,if (diff & flush_on_change)return PTE_FLUSH_STRICT ; +if (diff & oldflags & flush_on_clear_relaxed) +return PTE_FLUSH_RELAXED ; + +if (diff & newflags & flush_on_set_relaxed) +return PTE_FLUSH_RELAXED ; +/* Ensure there are no flags that were left behind */ if (IS_ENABLED (CONFIG_DEBUG_VM ) && - (diff & ~(flush_on_clear | software_flags | flush_on_change))) { + (diff & ~(flush_on_clear | flush_on_set | + software_flags | flush_on_change))) {VM_WARN_ON_ONCE (1 );return PTE_FLUSH_STRICT ; }
Signature · 1 line
2.25.1
RFC PATCH 12/14 x86/tlb: no flush on PTE change from RW->RO when PTE is clean
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
On x86 it is possible to skip a TLB flush when a RW entry become RO and the PTE is clean. Add logic to detect this case and skip the flush.
--- arch/x86/include/asm/tlbflush.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
arch/x86/include/asm/tlbflush.h +14 -2
@@ -271,8 +271,9 @@ static inline enum pte_flush_type pte_flags_flush_type(unsigned long oldflags,* dirty/access bit if needed without a fault. */ const pteval_t flush_on_clear = _PAGE_DIRTY | _PAGE_PRESENT | - _PAGE_ACCESSED | _PAGE_RW; + _PAGE_ACCESSED;const pteval_t flush_on_set = _PAGE_NX; +const pteval_t flush_on_special = _PAGE_RW;const pteval_t flush_on_set_relaxed = _PAGE_RW;const pteval_t flush_on_clear_relaxed = _PAGE_NX; const pteval_t software_flags = _PAGE_SOFTW1 | _PAGE_SOFTW2 |
@@ -302,6 +303,17 @@ static inline enum pte_flush_type pte_flags_flush_type(unsigned long oldflags,if (diff & oldflags & flush_on_clear)return PTE_FLUSH_STRICT ; +/* +* Did any of the 'flush_on_set' flags was set between 'oldflags' and +* 'newflags'? +*/ +if (diff & newflags & flush_on_set) +return PTE_FLUSH_STRICT ; + +/* On RW->RO, a flush is needed if the old entry is dirty */ +if ((diff & oldflags & _PAGE_RW) && (oldflags & _PAGE_DIRTY)) +return PTE_FLUSH_STRICT ; +/* Flush on modified flags. */ if (diff & flush_on_change)return PTE_FLUSH_STRICT ;
@@ -314,7 +326,7 @@ static inline enum pte_flush_type pte_flags_flush_type(unsigned long oldflags,/* Ensure there are no flags that were left behind */ if (IS_ENABLED (CONFIG_DEBUG_VM ) && - (diff & ~(flush_on_clear | flush_on_set | + (diff & ~(flush_on_clear | flush_on_set | flush_on_special | software_flags | flush_on_change))) {VM_WARN_ON_ONCE (1 );return PTE_FLUSH_STRICT ;
Signature · 1 line
2.25.1
RFC PATCH 13/14 mm/mprotect: do not check flush type if a strict is needed
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
Once it was determined that a strict TLB flush is needed, it is both likely that other PTEs would need strict TLB flush and little benefit from not extending the range that is flushed.
Skip the check which TLB flush is needed, if it was determined a strict flush is already needed.
--- mm/huge_memory.c | 4 +++- mm/mprotect.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-)
mm/huge_memory.c +3 -1
@@ -1816,7 +1816,9 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma, ret =HPAGE_PMD_NR ;set_pmd_at (mm, addr, pmd, entry); - flush_type =huge_pmd_flush_type (oldpmd, entry); + flush_type =PTE_FLUSH_STRICT ; +if (!tlb->strict) + flush_type =huge_pmd_flush_type (oldpmd, entry);if (flush_type !=PTE_FLUSH_NONE )tlb_flush_pmd_range (tlb, addr,HPAGE_PMD_SIZE , flush_type ==PTE_FLUSH_STRICT );
mm/mprotect.c +3 -1
@@ -202,7 +202,9 @@ static unsigned long change_pte_range(struct mmu_gather *tlb,ptep_modify_prot_commit (vma, addr, pte, oldpte, ptent); - flush_type =pte_flush_type (oldpte, ptent); + flush_type =PTE_FLUSH_STRICT ; +if (!tlb->strict) + flush_type =pte_flush_type (oldpte, ptent);if (flush_type !=PTE_FLUSH_NONE )tlb_flush_pte_range (tlb, addr,PAGE_SIZE , flush_type ==PTE_FLUSH_STRICT );
Signature · 1 line
2.25.1
RFC PATCH 14/14 mm: conditional check of pfn in pte_flush_type
In reply to Nadav AmitFrom: Nadav Amit <[email protected]>
Checking whether PFNs in two PTEs are the same takes surprisingly large number of instructions. Yet in fact, in most cases the caller to pte_flush_type() already knows if the PFN was changed. For instance, mprotect() does not change the PFN, but only modifies the protection flags.
Add argument to pte_flush_type() to indicate whether the PFN should be checked. Keep checking it in mm-debug to see if some caller was wrong to assume the PFN is the same.
--- arch/x86/include/asm/tlbflush.h | 14 ++++++++++---- include/asm-generic/tlb.h | 6 ++++-- mm/huge_memory.c | 2 +- mm/mprotect.c | 2 +- mm/rmap.c | 2 +- 5 files changed, 17 insertions(+), 9 deletions(-)
arch/x86/include/asm/tlbflush.h +10 -4
@@ -340,14 +340,17 @@ static inline enum pte_flush_type pte_flags_flush_type(unsigned long oldflags,* whether a strict or relaxed TLB flush is need. It should only be used on * userspace PTEs. */ -static inline enum pte_flush_type pte_flush_type (pte_t oldpte,pte_t newpte) +static inline enum pte_flush_type pte_flush_type (pte_t oldpte,pte_t newpte, +bool check_pfn) {/* !PRESENT -> * ; no need for flush */ if (!(pte_flags (oldpte) & _PAGE_PRESENT))return PTE_FLUSH_NONE ;/* PFN changed ; needs flush */ -if (pte_pfn (oldpte) !=pte_pfn (newpte)) +if (!check_pfn) +VM_BUG_ON (pte_pfn (oldpte) !=pte_pfn (newpte)); +else if (pte_pfn (oldpte) !=pte_pfn (newpte))return PTE_FLUSH_STRICT ;/*
@@ -363,14 +366,17 @@ static inline enum pte_flush_type pte_flush_type(pte_t oldpte, pte_t newpte)* huge_pmd_flush_type() checks whether permissions were demoted and require a * flush. It should only be used for userspace huge PMDs. */ -static inline enum pte_flush_type huge_pmd_flush_type (pmd_t oldpmd,pmd_t newpmd) +static inline enum pte_flush_type huge_pmd_flush_type (pmd_t oldpmd,pmd_t newpmd, +bool check_pfn) {/* !PRESENT -> * ; no need for flush */ if (!(pmd_flags (oldpmd) & _PAGE_PRESENT))return PTE_FLUSH_NONE ;/* PFN changed ; needs flush */ -if (pmd_pfn (oldpmd) !=pmd_pfn (newpmd)) +if (!check_pfn) +VM_BUG_ON (pmd_pfn (oldpmd) !=pmd_pfn (newpmd)); +else if (pmd_pfn (oldpmd) !=pmd_pfn (newpmd))return PTE_FLUSH_STRICT ;/*
include/asm-generic/tlb.h +4 -2
@@ -677,14 +677,16 @@ static inline void tlb_flush_p4d_range(struct mmu_gather *tlb,#endif #ifndef pte_flush_type -static inline struct pte_flush_type pte_flush_type (pte_t oldpte,pte_t newpte) +static inline struct pte_flush_type pte_flush_type (pte_t oldpte,pte_t newpte, +bool check_pfn) {return PTE_FLUSH_STRICT ; }#endif #ifndef huge_pmd_flush_type -static inline bool huge_pmd_flush_type (pmd_t oldpmd,pmd_t newpmd) +static inline bool huge_pmd_flush_type (pmd_t oldpmd,pmd_t newpmd, +bool check_pfn) {return PTE_FLUSH_STRICT ; }
mm/huge_memory.c +1 -1
@@ -1818,7 +1818,7 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma, flush_type =PTE_FLUSH_STRICT ;if (!tlb->strict) - flush_type =huge_pmd_flush_type (oldpmd, entry); + flush_type =huge_pmd_flush_type (oldpmd, entry, false);if (flush_type !=PTE_FLUSH_NONE )tlb_flush_pmd_range (tlb, addr,HPAGE_PMD_SIZE , flush_type ==PTE_FLUSH_STRICT );
mm/mprotect.c +1 -1
@@ -204,7 +204,7 @@ static unsigned long change_pte_range(struct mmu_gather *tlb, flush_type =PTE_FLUSH_STRICT ;if (!tlb->strict) - flush_type =pte_flush_type (oldpte, ptent); + flush_type =pte_flush_type (oldpte, ptent, false);if (flush_type !=PTE_FLUSH_NONE )tlb_flush_pte_range (tlb, addr,PAGE_SIZE , flush_type ==PTE_FLUSH_STRICT );
mm/rmap.c +1 -1
@@ -974,7 +974,7 @@ static int page_vma_mkclean_one(struct page_vma_mapped_walk *pvmw) entry =pte_wrprotect (oldpte); entry =pte_mkclean (entry); -if (pte_flush_type (oldpte, entry) !=PTE_FLUSH_NONE || +if (pte_flush_type (oldpte, entry, false) !=PTE_FLUSH_NONE ||mm_tlb_flush_pending (vma->vm_mm))flush_tlb_page (vma, address);
Signature · 1 line
2.25.1