| Message ID | 20240402124029.47846-2-ioworker0@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/madvise: enhance lazyfreeing with mTHP in madvise_free | expand |
On 02/04/2024 13:40, Lance Yang wrote: > Change the code that clears young and dirty bits from the PTEs to use > ptep_get_and_clear_full() and set_pte_at(), via the new mkold_clean_ptes() > batch helper function. > > Unfortunately, the per-pte get_and_clear/modify/set approach would result > in unfolding/refolding for contpte mappings on arm64. So we need to > override mkold_clean_ptes() for arm64 to avoid it. > > Suggested-by: David Hildenbrand <david@redhat.com> > Suggested-by: Barry Song <21cnbao@gmail.com> > Suggested-by: Ryan Roberts <ryan.roberts@arm.com> > Signed-off-by: Lance Yang <ioworker0@gmail.com> > --- > arch/arm64/include/asm/pgtable.h | 36 ++++++++++++++++++++++++++++++++ > arch/arm64/mm/contpte.c | 10 +++++++++ > include/linux/pgtable.h | 30 ++++++++++++++++++++++++++ > 3 files changed, 76 insertions(+) When I suggested splitting out this patch, this wasn't quite what I meant. Instead I intended for the first patch to make the MADV_FREE change and as part of that introduce mkold_clean_ptes() with a generic implementation. Then a second patch would do the arm64-specific specialization of mkold_clean_ptes() for an arm64-specific performance boost. In general it is often useful to separate core changes from arch changes where practical since they have different target audiences. > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > index 9fd8613b2db2..b032c107090c 100644 > --- a/arch/arm64/include/asm/pgtable.h > +++ b/arch/arm64/include/asm/pgtable.h > @@ -1086,6 +1086,27 @@ static inline bool pud_user_accessible_page(pud_t pud) > } > #endif > > +static inline void __mkold_clean_pte(struct vm_area_struct *vma, > + unsigned long addr, pte_t *ptep) > +{ > + pte_t old_pte, pte; > + > + pte = __ptep_get(ptep); > + do { > + old_pte = pte; > + pte = pte_mkclean(pte_mkold(pte)); > + pte_val(pte) = cmpxchg_relaxed(&pte_val(*ptep), > + pte_val(old_pte), pte_val(pte)); > + } while (pte_val(pte) != pte_val(old_pte)); > +} I'm not sure how value it is to split this out, given we are only exposing a batched version to the core. Perhaps just include the body in mkold_clean_ptes()? > + > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > + unsigned long addr, pte_t *ptep, unsigned int nr, int full) > +{ > + for (; nr-- > 0; ptep++, addr += PAGE_SIZE) > + __mkold_clean_pte(vma, addr, ptep); > +} > + > /* > * Atomic pte/pmd modifications. > */ > @@ -1379,6 +1400,8 @@ extern void contpte_wrprotect_ptes(struct mm_struct *mm, unsigned long addr, > extern int contpte_ptep_set_access_flags(struct vm_area_struct *vma, > unsigned long addr, pte_t *ptep, > pte_t entry, int dirty); > +extern void contpte_ptep_mkold_clean(struct vm_area_struct *vma, > + unsigned long addr, pte_t *ptep); Let's have this follow the same naming pattern; contpte_mkold_clean_ptes() > > static __always_inline void contpte_try_fold(struct mm_struct *mm, > unsigned long addr, pte_t *ptep, pte_t pte) > @@ -1603,6 +1626,18 @@ static inline int ptep_set_access_flags(struct vm_area_struct *vma, > return contpte_ptep_set_access_flags(vma, addr, ptep, entry, dirty); > } > > +#define mkold_clean_ptes mkold_clean_ptes > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > + unsigned long addr, pte_t *ptep, unsigned int nr, int full) > +{ > + pte_t orig_pte = __ptep_get(ptep); > + > + if (likely(!pte_valid_cont(orig_pte))) > + return __mkold_clean_ptes(vma, addr, ptep, nr, full); > + > + return contpte_ptep_mkold_clean(vma, addr, ptep);> +} This function is totally broken as far as I can tell. You are assuming if the first pte is not cont, then the whole range is not cont; you can't assume that. Then if you decide it is cont, you ignore nr and only fixup a single contpte block. Take a look at wrprotect_ptes() or another one of the batched helpers and follow that pattern. > + > #else /* CONFIG_ARM64_CONTPTE */ > > #define ptep_get __ptep_get > @@ -1622,6 +1657,7 @@ static inline int ptep_set_access_flags(struct vm_area_struct *vma, > #define wrprotect_ptes __wrprotect_ptes > #define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS > #define ptep_set_access_flags __ptep_set_access_flags > +#define mkold_clean_ptes __mkold_clean_ptes > > #endif /* CONFIG_ARM64_CONTPTE */ > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c > index 1b64b4c3f8bf..560622cfb2a9 100644 > --- a/arch/arm64/mm/contpte.c > +++ b/arch/arm64/mm/contpte.c > @@ -322,6 +322,16 @@ int contpte_ptep_test_and_clear_young(struct vm_area_struct *vma, > } > EXPORT_SYMBOL_GPL(contpte_ptep_test_and_clear_young); > > +void contpte_ptep_mkold_clean(struct vm_area_struct *vma, unsigned long addr, > + pte_t *ptep) > +{ > + ptep = contpte_align_down(ptep); > + addr = ALIGN_DOWN(addr, CONT_PTE_SIZE); > + > + __mkold_clean_ptes(vma, addr, ptep, CONT_PTES, 0); As above, this is broken as is. > +} > +EXPORT_SYMBOL_GPL(contpte_ptep_mkold_clean); > + > int contpte_ptep_clear_flush_young(struct vm_area_struct *vma, > unsigned long addr, pte_t *ptep) > { > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h > index fa8f92f6e2d7..fd30779fe487 100644 > --- a/include/linux/pgtable.h > +++ b/include/linux/pgtable.h > @@ -391,6 +391,36 @@ static inline void mkold_ptes(struct vm_area_struct *vma, unsigned long addr, > } > #endif > > +#ifndef mkold_clean_ptes > +/** > + * mkold_clean_ptes - Mark PTEs that map consecutive pages of the same folio > + * as old and clean. > + * @vma: VMA the pages are mapped into. > + * @addr: Address the first page is mapped at. > + * @ptep: Page table pointer for the first entry. > + * @nr: Number of entries to mark old and clean. > + * > + * May be overridden by the architecture; otherwise, implemented as a simple > + * loop over ptep_get_and_clear_full(). How about "otherwise, implemented by get_and_clear/modify/set for each pte in the range."? > + * > + * Note that PTE bits in the PTE range besides the PFN can differ. For example, > + * some PTEs might be write-protected. > + * > + * Context: The caller holds the page table lock. The PTEs map consecutive > + * pages that belong to the same folio. The PTEs are all in the same PMD. > + */ > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > + unsigned long addr, pte_t *ptep, unsigned int nr, int full) You've included the "full" param in the function but not in the docs. I don't think the full param is valuable though; it doesn't make sense to call this function when tearing down a process. So drop the parameter and just call ptep_get_and_clear(). > +{ > + pte_t pte; > + > + for (; nr-- > 0; ptep++, addr += PAGE_SIZE) { nit: This is using a different pattern to all the other batch helpers (set_ptes(), wrprotect_ptes() clear_full_ptes()). > + pte = ptep_get_and_clear_full(vma->vm_mm, addr, ptep, full); > + set_pte_at(vma->vm_mm, addr, ptep, pte_mkclean(pte_mkold(pte))); > + } > +} > +#endif > + > #ifndef __HAVE_ARCH_PMDP_TEST_AND_CLEAR_YOUNG > #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_ARCH_HAS_NONLEAF_PMD_YOUNG) > static inline int pmdp_test_and_clear_young(struct vm_area_struct *vma,
Hey Ryan, Thanks for taking time to review! On Thu, Apr 4, 2024 at 12:38 AM Ryan Roberts <ryan.roberts@arm.com> wrote: > > On 02/04/2024 13:40, Lance Yang wrote: > > Change the code that clears young and dirty bits from the PTEs to use > > ptep_get_and_clear_full() and set_pte_at(), via the new mkold_clean_ptes() > > batch helper function. > > > > Unfortunately, the per-pte get_and_clear/modify/set approach would result > > in unfolding/refolding for contpte mappings on arm64. So we need to > > override mkold_clean_ptes() for arm64 to avoid it. > > > > Suggested-by: David Hildenbrand <david@redhat.com> > > Suggested-by: Barry Song <21cnbao@gmail.com> > > Suggested-by: Ryan Roberts <ryan.roberts@arm.com> > > Signed-off-by: Lance Yang <ioworker0@gmail.com> > > --- > > arch/arm64/include/asm/pgtable.h | 36 ++++++++++++++++++++++++++++++++ > > arch/arm64/mm/contpte.c | 10 +++++++++ > > include/linux/pgtable.h | 30 ++++++++++++++++++++++++++ > > 3 files changed, 76 insertions(+) > > When I suggested splitting out this patch, this wasn't quite what I meant. > Instead I intended for the first patch to make the MADV_FREE change and as part > of that introduce mkold_clean_ptes() with a generic implementation. Then a > second patch would do the arm64-specific specialization of mkold_clean_ptes() > for an arm64-specific performance boost. In general it is often useful to > separate core changes from arch changes where practical since they have > different target audiences. Thanks for clearing that up! I‘ll make sure to implement it as discussed. > > > > > > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h > > index 9fd8613b2db2..b032c107090c 100644 > > --- a/arch/arm64/include/asm/pgtable.h > > +++ b/arch/arm64/include/asm/pgtable.h > > @@ -1086,6 +1086,27 @@ static inline bool pud_user_accessible_page(pud_t pud) > > } > > #endif > > > > +static inline void __mkold_clean_pte(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep) > > +{ > > + pte_t old_pte, pte; > > + > > + pte = __ptep_get(ptep); > > + do { > > + old_pte = pte; > > + pte = pte_mkclean(pte_mkold(pte)); > > + pte_val(pte) = cmpxchg_relaxed(&pte_val(*ptep), > > + pte_val(old_pte), pte_val(pte)); > > + } while (pte_val(pte) != pte_val(old_pte)); > > +} > > I'm not sure how value it is to split this out, given we are only exposing a > batched version to the core. Perhaps just include the body in mkold_clean_ptes()? I agree that including the body in mkold_clean_ptes() would be better. > > > + > > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep, unsigned int nr, int full) > > +{ > > + for (; nr-- > 0; ptep++, addr += PAGE_SIZE) > > + __mkold_clean_pte(vma, addr, ptep); > > +} > > + > > /* > > * Atomic pte/pmd modifications. > > */ > > @@ -1379,6 +1400,8 @@ extern void contpte_wrprotect_ptes(struct mm_struct *mm, unsigned long addr, > > extern int contpte_ptep_set_access_flags(struct vm_area_struct *vma, > > unsigned long addr, pte_t *ptep, > > pte_t entry, int dirty); > > +extern void contpte_ptep_mkold_clean(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep); > > Let's have this follow the same naming pattern; contpte_mkold_clean_ptes() Got it. Let’s keep the naming consistent with contpte_mkold_clean_ptes(). > > > > > static __always_inline void contpte_try_fold(struct mm_struct *mm, > > unsigned long addr, pte_t *ptep, pte_t pte) > > @@ -1603,6 +1626,18 @@ static inline int ptep_set_access_flags(struct vm_area_struct *vma, > > return contpte_ptep_set_access_flags(vma, addr, ptep, entry, dirty); > > } > > > > +#define mkold_clean_ptes mkold_clean_ptes > > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep, unsigned int nr, int full) > > +{ > > + pte_t orig_pte = __ptep_get(ptep); > > + > > + if (likely(!pte_valid_cont(orig_pte))) > > + return __mkold_clean_ptes(vma, addr, ptep, nr, full); > > + > > + return contpte_ptep_mkold_clean(vma, addr, ptep);> +} > > This function is totally broken as far as I can tell. You are assuming if the > first pte is not cont, then the whole range is not cont; you can't assume that. > Then if you decide it is cont, you ignore nr and only fixup a single contpte block. > > Take a look at wrprotect_ptes() or another one of the batched helpers and follow > that pattern. Sorry for the mistake. I'll take a closer look at wrprotect_ptes() and make sure to follow that pattern. > > > + > > #else /* CONFIG_ARM64_CONTPTE */ > > > > #define ptep_get __ptep_get > > @@ -1622,6 +1657,7 @@ static inline int ptep_set_access_flags(struct vm_area_struct *vma, > > #define wrprotect_ptes __wrprotect_ptes > > #define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS > > #define ptep_set_access_flags __ptep_set_access_flags > > +#define mkold_clean_ptes __mkold_clean_ptes > > > > #endif /* CONFIG_ARM64_CONTPTE */ > > > > diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c > > index 1b64b4c3f8bf..560622cfb2a9 100644 > > --- a/arch/arm64/mm/contpte.c > > +++ b/arch/arm64/mm/contpte.c > > @@ -322,6 +322,16 @@ int contpte_ptep_test_and_clear_young(struct vm_area_struct *vma, > > } > > EXPORT_SYMBOL_GPL(contpte_ptep_test_and_clear_young); > > > > +void contpte_ptep_mkold_clean(struct vm_area_struct *vma, unsigned long addr, > > + pte_t *ptep) > > +{ > > + ptep = contpte_align_down(ptep); > > + addr = ALIGN_DOWN(addr, CONT_PTE_SIZE); > > + > > + __mkold_clean_ptes(vma, addr, ptep, CONT_PTES, 0); > > As above, this is broken as is. > > > +} > > +EXPORT_SYMBOL_GPL(contpte_ptep_mkold_clean); > > + > > int contpte_ptep_clear_flush_young(struct vm_area_struct *vma, > > unsigned long addr, pte_t *ptep) > > { > > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h > > index fa8f92f6e2d7..fd30779fe487 100644 > > --- a/include/linux/pgtable.h > > +++ b/include/linux/pgtable.h > > @@ -391,6 +391,36 @@ static inline void mkold_ptes(struct vm_area_struct *vma, unsigned long addr, > > } > > #endif > > > > +#ifndef mkold_clean_ptes > > +/** > > + * mkold_clean_ptes - Mark PTEs that map consecutive pages of the same folio > > + * as old and clean. > > + * @vma: VMA the pages are mapped into. > > + * @addr: Address the first page is mapped at. > > + * @ptep: Page table pointer for the first entry. > > + * @nr: Number of entries to mark old and clean. > > + * > > + * May be overridden by the architecture; otherwise, implemented as a simple > > + * loop over ptep_get_and_clear_full(). > > How about "otherwise, implemented by get_and_clear/modify/set for each pte in > the range."? Nice, this is clearer and more accurate than before. > > > + * > > + * Note that PTE bits in the PTE range besides the PFN can differ. For example, > > + * some PTEs might be write-protected. > > + * > > + * Context: The caller holds the page table lock. The PTEs map consecutive > > + * pages that belong to the same folio. The PTEs are all in the same PMD. > > + */ > > +static inline void mkold_clean_ptes(struct vm_area_struct *vma, > > + unsigned long addr, pte_t *ptep, unsigned int nr, int full) > > You've included the "full" param in the function but not in the docs. I don't > think the full param is valuable though; it doesn't make sense to call this > function when tearing down a process. So drop the parameter and just call > ptep_get_and_clear(). Got it. I'll drop the "full" parameter. > > > +{ > > + pte_t pte; > > + > > + for (; nr-- > 0; ptep++, addr += PAGE_SIZE) { > > nit: This is using a different pattern to all the other batch helpers > (set_ptes(), wrprotect_ptes() clear_full_ptes()). Thanks for pointing that out. Thanks, Lance > > > + pte = ptep_get_and_clear_full(vma->vm_mm, addr, ptep, full); > > + set_pte_at(vma->vm_mm, addr, ptep, pte_mkclean(pte_mkold(pte))); > > + } > > +} > > +#endif > > + > > #ifndef __HAVE_ARCH_PMDP_TEST_AND_CLEAR_YOUNG > > #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_ARCH_HAS_NONLEAF_PMD_YOUNG) > > static inline int pmdp_test_and_clear_young(struct vm_area_struct *vma, >
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index 9fd8613b2db2..b032c107090c 100644 --- a/arch/arm64/include/asm/pgtable.h +++ b/arch/arm64/include/asm/pgtable.h @@ -1086,6 +1086,27 @@ static inline bool pud_user_accessible_page(pud_t pud) } #endif +static inline void __mkold_clean_pte(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep) +{ + pte_t old_pte, pte; + + pte = __ptep_get(ptep); + do { + old_pte = pte; + pte = pte_mkclean(pte_mkold(pte)); + pte_val(pte) = cmpxchg_relaxed(&pte_val(*ptep), + pte_val(old_pte), pte_val(pte)); + } while (pte_val(pte) != pte_val(old_pte)); +} + +static inline void mkold_clean_ptes(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep, unsigned int nr, int full) +{ + for (; nr-- > 0; ptep++, addr += PAGE_SIZE) + __mkold_clean_pte(vma, addr, ptep); +} + /* * Atomic pte/pmd modifications. */ @@ -1379,6 +1400,8 @@ extern void contpte_wrprotect_ptes(struct mm_struct *mm, unsigned long addr, extern int contpte_ptep_set_access_flags(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep, pte_t entry, int dirty); +extern void contpte_ptep_mkold_clean(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep); static __always_inline void contpte_try_fold(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_t pte) @@ -1603,6 +1626,18 @@ static inline int ptep_set_access_flags(struct vm_area_struct *vma, return contpte_ptep_set_access_flags(vma, addr, ptep, entry, dirty); } +#define mkold_clean_ptes mkold_clean_ptes +static inline void mkold_clean_ptes(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep, unsigned int nr, int full) +{ + pte_t orig_pte = __ptep_get(ptep); + + if (likely(!pte_valid_cont(orig_pte))) + return __mkold_clean_ptes(vma, addr, ptep, nr, full); + + return contpte_ptep_mkold_clean(vma, addr, ptep); +} + #else /* CONFIG_ARM64_CONTPTE */ #define ptep_get __ptep_get @@ -1622,6 +1657,7 @@ static inline int ptep_set_access_flags(struct vm_area_struct *vma, #define wrprotect_ptes __wrprotect_ptes #define __HAVE_ARCH_PTEP_SET_ACCESS_FLAGS #define ptep_set_access_flags __ptep_set_access_flags +#define mkold_clean_ptes __mkold_clean_ptes #endif /* CONFIG_ARM64_CONTPTE */ diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c index 1b64b4c3f8bf..560622cfb2a9 100644 --- a/arch/arm64/mm/contpte.c +++ b/arch/arm64/mm/contpte.c @@ -322,6 +322,16 @@ int contpte_ptep_test_and_clear_young(struct vm_area_struct *vma, } EXPORT_SYMBOL_GPL(contpte_ptep_test_and_clear_young); +void contpte_ptep_mkold_clean(struct vm_area_struct *vma, unsigned long addr, + pte_t *ptep) +{ + ptep = contpte_align_down(ptep); + addr = ALIGN_DOWN(addr, CONT_PTE_SIZE); + + __mkold_clean_ptes(vma, addr, ptep, CONT_PTES, 0); +} +EXPORT_SYMBOL_GPL(contpte_ptep_mkold_clean); + int contpte_ptep_clear_flush_young(struct vm_area_struct *vma, unsigned long addr, pte_t *ptep) { diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h index fa8f92f6e2d7..fd30779fe487 100644 --- a/include/linux/pgtable.h +++ b/include/linux/pgtable.h @@ -391,6 +391,36 @@ static inline void mkold_ptes(struct vm_area_struct *vma, unsigned long addr, } #endif +#ifndef mkold_clean_ptes +/** + * mkold_clean_ptes - Mark PTEs that map consecutive pages of the same folio + * as old and clean. + * @vma: VMA the pages are mapped into. + * @addr: Address the first page is mapped at. + * @ptep: Page table pointer for the first entry. + * @nr: Number of entries to mark old and clean. + * + * May be overridden by the architecture; otherwise, implemented as a simple + * loop over ptep_get_and_clear_full(). + * + * Note that PTE bits in the PTE range besides the PFN can differ. For example, + * some PTEs might be write-protected. + * + * Context: The caller holds the page table lock. The PTEs map consecutive + * pages that belong to the same folio. The PTEs are all in the same PMD. + */ +static inline void mkold_clean_ptes(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep, unsigned int nr, int full) +{ + pte_t pte; + + for (; nr-- > 0; ptep++, addr += PAGE_SIZE) { + pte = ptep_get_and_clear_full(vma->vm_mm, addr, ptep, full); + set_pte_at(vma->vm_mm, addr, ptep, pte_mkclean(pte_mkold(pte))); + } +} +#endif + #ifndef __HAVE_ARCH_PMDP_TEST_AND_CLEAR_YOUNG #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_ARCH_HAS_NONLEAF_PMD_YOUNG) static inline int pmdp_test_and_clear_young(struct vm_area_struct *vma,
Change the code that clears young and dirty bits from the PTEs to use ptep_get_and_clear_full() and set_pte_at(), via the new mkold_clean_ptes() batch helper function. Unfortunately, the per-pte get_and_clear/modify/set approach would result in unfolding/refolding for contpte mappings on arm64. So we need to override mkold_clean_ptes() for arm64 to avoid it. Suggested-by: David Hildenbrand <david@redhat.com> Suggested-by: Barry Song <21cnbao@gmail.com> Suggested-by: Ryan Roberts <ryan.roberts@arm.com> Signed-off-by: Lance Yang <ioworker0@gmail.com> --- arch/arm64/include/asm/pgtable.h | 36 ++++++++++++++++++++++++++++++++ arch/arm64/mm/contpte.c | 10 +++++++++ include/linux/pgtable.h | 30 ++++++++++++++++++++++++++ 3 files changed, 76 insertions(+)