| Message ID | 20230306161548.661740-1-gerald.schaefer@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: add PTE pointer parameter to flush_tlb_fix_spurious_fault() | expand |
On Mon, Mar 06, 2023 at 05:15:48PM +0100, Gerald Schaefer wrote: > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > index b6ba466e2e8a..0bd18de9fd97 100644 > --- a/arch/arm64/include/asm/pgtable.h > +++ b/arch/arm64/include/asm/pgtable.h > @@ -57,7 +57,7 @@ static inline bool arch_thp_swp_supported(void) > * fault on one CPU which has been handled concurrently by another CPU > * does not need to perform additional invalidation. > */ > -#define flush_tlb_fix_spurious_fault(vma, address) do { } while (0) > +#define flush_tlb_fix_spurious_fault(vma, address, ptep) do { } while (0) For arm64: Acked-by: Catalin Marinas <catalin.marinas@arm.com> > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h > index 2c70b4d1263d..c1f6b46ec555 100644 > --- a/arch/s390/include/asm/pgtable.h > +++ b/arch/s390/include/asm/pgtable.h > @@ -1239,7 +1239,8 @@ static inline int pte_allow_rdp(pte_t old, pte_t new) > } > > static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, > - unsigned long address) > + unsigned long address, > + pte_t *ptep) > { > /* > * RDP might not have propagated the PTE protection reset to all CPUs, > @@ -1247,11 +1248,12 @@ static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, > * NOTE: This will also be called when a racing pagetable update on > * another thread already installed the correct PTE. Both cases cannot > * really be distinguished. > - * Therefore, only do the local TLB flush when RDP can be used, to avoid > - * unnecessary overhead. > + * Therefore, only do the local TLB flush when RDP can be used, and the > + * PTE does not have _PAGE_PROTECT set, to avoid unnecessary overhead. > + * A local RDP can be used to do the flush. > */ > - if (MACHINE_HAS_RDP) > - asm volatile("ptlb" : : : "memory"); > + if (MACHINE_HAS_RDP && !(pte_val(*ptep) & _PAGE_PROTECT)) > + __ptep_rdp(address, ptep, 0, 0, 1); I wonder whether passing the actual entry is somewhat quicker as it avoids another memory access (though it might already be in the cache).
On Mon, 6 Mar 2023 17:06:44 +0000 Catalin Marinas <catalin.marinas@arm.com> wrote: > On Mon, Mar 06, 2023 at 05:15:48PM +0100, Gerald Schaefer wrote: > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > > index b6ba466e2e8a..0bd18de9fd97 100644 > > --- a/arch/arm64/include/asm/pgtable.h > > +++ b/arch/arm64/include/asm/pgtable.h > > @@ -57,7 +57,7 @@ static inline bool arch_thp_swp_supported(void) > > * fault on one CPU which has been handled concurrently by another CPU > > * does not need to perform additional invalidation. > > */ > > -#define flush_tlb_fix_spurious_fault(vma, address) do { } while (0) > > +#define flush_tlb_fix_spurious_fault(vma, address, ptep) do { } while (0) > > For arm64: > > Acked-by: Catalin Marinas <catalin.marinas@arm.com> > > > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h > > index 2c70b4d1263d..c1f6b46ec555 100644 > > --- a/arch/s390/include/asm/pgtable.h > > +++ b/arch/s390/include/asm/pgtable.h > > @@ -1239,7 +1239,8 @@ static inline int pte_allow_rdp(pte_t old, pte_t new) > > } > > > > static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, > > - unsigned long address) > > + unsigned long address, > > + pte_t *ptep) > > { > > /* > > * RDP might not have propagated the PTE protection reset to all CPUs, > > @@ -1247,11 +1248,12 @@ static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, > > * NOTE: This will also be called when a racing pagetable update on > > * another thread already installed the correct PTE. Both cases cannot > > * really be distinguished. > > - * Therefore, only do the local TLB flush when RDP can be used, to avoid > > - * unnecessary overhead. > > + * Therefore, only do the local TLB flush when RDP can be used, and the > > + * PTE does not have _PAGE_PROTECT set, to avoid unnecessary overhead. > > + * A local RDP can be used to do the flush. > > */ > > - if (MACHINE_HAS_RDP) > > - asm volatile("ptlb" : : : "memory"); > > + if (MACHINE_HAS_RDP && !(pte_val(*ptep) & _PAGE_PROTECT)) > > + __ptep_rdp(address, ptep, 0, 0, 1); > > I wonder whether passing the actual entry is somewhat quicker as it > avoids another memory access (though it might already be in the cache). The RDP instruction itself only requires the PTE pointer as input, or more precisely a pointer to the pagetable origin. We calculate that from the PTE pointer, by masking out some bits, w/o actual memory access to the PTE entry value. Of course, there is the pte_val(*ptep) & _PAGE_PROTECT check here, with memory access, but this might get removed in the future. TBH, I simply wasn't sure (enough) yet, if we could technically ever end up here with _PAGE_PROTECT set at all. For "real" spurious protection faults, it should never be set, not so sure about racing pagetable updates though. So this might actually be an unnecessary / overly cautious check, that gets removed in the future, and not worth passing along the PTE value in addition to the pointer.
Gerald Schaefer <gerald.schaefer@linux.ibm.com> writes: > s390 can do more fine-grained handling of spurious TLB protection faults, > when there also is the PTE pointer available. > > Therefore, pass on the PTE pointer to flush_tlb_fix_spurious_fault() as > an additional parameter. > > This will add no functional change to other architectures, but those with > private flush_tlb_fix_spurious_fault() implementations need to be made > aware of the new parameter. > > Reviewed-by: Alexander Gordeev <agordeev@linux.ibm.com> > Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> > --- > arch/arm64/include/asm/pgtable.h | 2 +- > arch/mips/include/asm/pgtable.h | 3 ++- > arch/powerpc/include/asm/book3s/64/tlbflush.h | 3 ++- > arch/s390/include/asm/pgtable.h | 12 +++++++----- > arch/x86/include/asm/pgtable.h | 2 +- > include/linux/pgtable.h | 2 +- > mm/memory.c | 3 ++- > mm/pgtable-generic.c | 2 +- > 8 files changed, 17 insertions(+), 12 deletions(-) ... > diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h > index 2bbc0fcce04a..ff7f0ee179e5 100644 > --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h > +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h > @@ -121,7 +121,8 @@ static inline void flush_tlb_page(struct vm_area_struct *vma, > > #define flush_tlb_fix_spurious_fault flush_tlb_fix_spurious_fault > static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, > - unsigned long address) > + unsigned long address, > + pte_t *ptep) > { > /* > * Book3S 64 does not require spurious fault flushes because the PTE Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc) cheers
On 06.03.23 17:15, Gerald Schaefer wrote: > s390 can do more fine-grained handling of spurious TLB protection faults, > when there also is the PTE pointer available. > > Therefore, pass on the PTE pointer to flush_tlb_fix_spurious_fault() as > an additional parameter. > > This will add no functional change to other architectures, but those with > private flush_tlb_fix_spurious_fault() implementations need to be made > aware of the new parameter. > > Reviewed-by: Alexander Gordeev <agordeev@linux.ibm.com> > Signed-off-by: Gerald Schaefer <gerald.schaefer@linux.ibm.com> > --- Acked-by: David Hildenbrand <david@redhat.com>
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index b6ba466e2e8a..0bd18de9fd97 100644 --- a/arch/arm64/include/asm/pgtable.h +++ b/arch/arm64/include/asm/pgtable.h @@ -57,7 +57,7 @@ static inline bool arch_thp_swp_supported(void) * fault on one CPU which has been handled concurrently by another CPU * does not need to perform additional invalidation. */ -#define flush_tlb_fix_spurious_fault(vma, address) do { } while (0) +#define flush_tlb_fix_spurious_fault(vma, address, ptep) do { } while (0) /* * ZERO_PAGE is a global shared page that is always zero: used diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h index 791389bf3c12..574fa14ac8b2 100644 --- a/arch/mips/include/asm/pgtable.h +++ b/arch/mips/include/asm/pgtable.h @@ -469,7 +469,8 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot) } static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, - unsigned long address) + unsigned long address, + pte_t *ptep) { } diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h index 2bbc0fcce04a..ff7f0ee179e5 100644 --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h @@ -121,7 +121,8 @@ static inline void flush_tlb_page(struct vm_area_struct *vma, #define flush_tlb_fix_spurious_fault flush_tlb_fix_spurious_fault static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, - unsigned long address) + unsigned long address, + pte_t *ptep) { /* * Book3S 64 does not require spurious fault flushes because the PTE diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h index 2c70b4d1263d..c1f6b46ec555 100644 --- a/arch/s390/include/asm/pgtable.h +++ b/arch/s390/include/asm/pgtable.h @@ -1239,7 +1239,8 @@ static inline int pte_allow_rdp(pte_t old, pte_t new) } static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, - unsigned long address) + unsigned long address, + pte_t *ptep) { /* * RDP might not have propagated the PTE protection reset to all CPUs, @@ -1247,11 +1248,12 @@ static inline void flush_tlb_fix_spurious_fault(struct vm_area_struct *vma, * NOTE: This will also be called when a racing pagetable update on * another thread already installed the correct PTE. Both cases cannot * really be distinguished. - * Therefore, only do the local TLB flush when RDP can be used, to avoid - * unnecessary overhead. + * Therefore, only do the local TLB flush when RDP can be used, and the + * PTE does not have _PAGE_PROTECT set, to avoid unnecessary overhead. + * A local RDP can be used to do the flush. */ - if (MACHINE_HAS_RDP) - asm volatile("ptlb" : : : "memory"); + if (MACHINE_HAS_RDP && !(pte_val(*ptep) & _PAGE_PROTECT)) + __ptep_rdp(address, ptep, 0, 0, 1); } #define flush_tlb_fix_spurious_fault flush_tlb_fix_spurious_fault diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h index 7425f32e5293..15ae4d6ba476 100644 --- a/arch/x86/include/asm/pgtable.h +++ b/arch/x86/include/asm/pgtable.h @@ -1097,7 +1097,7 @@ 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) +#define flush_tlb_fix_spurious_fault(vma, address, ptep) do { } while (0) #define mk_pmd(page, pgprot) pfn_pmd(page_to_pfn(page), (pgprot)) diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h index 9dc936bc77d1..c5a51481bbb9 100644 --- a/include/linux/pgtable.h +++ b/include/linux/pgtable.h @@ -817,7 +817,7 @@ static inline void arch_swap_restore(swp_entry_t entry, struct folio *folio) #endif #ifndef flush_tlb_fix_spurious_fault -#define flush_tlb_fix_spurious_fault(vma, address) flush_tlb_page(vma, address) +#define flush_tlb_fix_spurious_fault(vma, address, ptep) flush_tlb_page(vma, address) #endif /* diff --git a/mm/memory.c b/mm/memory.c index 0adf23ea5416..7ca7951adcf5 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -4956,7 +4956,8 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf) * with threads. */ if (vmf->flags & FAULT_FLAG_WRITE) - flush_tlb_fix_spurious_fault(vmf->vma, vmf->address); + flush_tlb_fix_spurious_fault(vmf->vma, vmf->address, + vmf->pte); } unlock: pte_unmap_unlock(vmf->pte, vmf->ptl); diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c index 90ab721a12a8..d2fc52bffafc 100644 --- a/mm/pgtable-generic.c +++ b/mm/pgtable-generic.c @@ -69,7 +69,7 @@ int ptep_set_access_flags(struct vm_area_struct *vma, int changed = !pte_same(*ptep, entry); if (changed) { set_pte_at(vma->vm_mm, address, ptep, entry); - flush_tlb_fix_spurious_fault(vma, address); + flush_tlb_fix_spurious_fault(vma, address, ptep); } return changed; }