| Message ID | 20240501042700.83974-3-ioworker0@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Reclaim lazyfree THP without splitting | expand |
On 2024/5/1 12:26, Lance Yang wrote: > In preparation for supporting try_to_unmap_one() to unmap PMD-mapped > folios, start the pagewalk first, then call split_huge_pmd_address() > to split the folio. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Lance Yang <ioworker0@gmail.com> > --- > include/linux/huge_mm.h | 20 ++++++++++++++++++++ > mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- > mm/rmap.c | 24 +++++++++++++++++------ > 3 files changed, 60 insertions(+), 26 deletions(-) > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index c8d3ec116e29..38c4b5537715 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) > return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); > } > > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > + pmd_t *pmd, bool freeze, struct folio *folio); > + > +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > + unsigned long *start, > + unsigned long *end) > +{ > + *start = ALIGN(*start, HPAGE_PMD_SIZE); > + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); > + > + VM_WARN_ON_ONCE(vma->vm_start > *start); > + VM_WARN_ON_ONCE(vma->vm_end < *end); > +} > + > #else /* CONFIG_TRANSPARENT_HUGEPAGE */ > > static inline bool folio_test_pmd_mappable(struct folio *folio) > @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > unsigned long address, bool freeze, struct folio *folio) {} > static inline void split_huge_pmd_address(struct vm_area_struct *vma, > unsigned long address, bool freeze, struct folio *folio) {} > +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, > + unsigned long address, pmd_t *pmd, > + bool freeze, struct folio *folio) {} > +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > + unsigned long *start, > + unsigned long *end) {} > > #define split_huge_pud(__vma, __pmd, __address) \ > do { } while (0) > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 8261b5669397..145505a1dd05 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, > pmd_populate(mm, pmd, pgtable); > } > > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > + pmd_t *pmd, bool freeze, struct folio *folio) > +{ > + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); > + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); > + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > + VM_BUG_ON(freeze && !folio); > + > + /* > + * When the caller requests to set up a migration entry, we > + * require a folio to check the PMD against. Otherwise, there > + * is a risk of replacing the wrong folio. > + */ > + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > + is_pmd_migration_entry(*pmd)) { > + if (folio && folio != pmd_folio(*pmd)) > + return; > + __split_huge_pmd_locked(vma, pmd, address, freeze); > + } > +} > + > void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > unsigned long address, bool freeze, struct folio *folio) > { > @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); > mmu_notifier_invalidate_range_start(&range); > ptl = pmd_lock(vma->vm_mm, pmd); > - > - /* > - * If caller asks to setup a migration entry, we need a folio to check > - * pmd against. Otherwise we can end up replacing wrong folio. > - */ > - VM_BUG_ON(freeze && !folio); > - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > - > - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > - is_pmd_migration_entry(*pmd)) { > - /* > - * It's safe to call pmd_page when folio is set because it's > - * guaranteed that pmd is present. > - */ > - if (folio && folio != pmd_folio(*pmd)) > - goto out; > - __split_huge_pmd_locked(vma, pmd, range.start, freeze); > - } > - > -out: > + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); > spin_unlock(ptl); > mmu_notifier_invalidate_range_end(&range); > } > diff --git a/mm/rmap.c b/mm/rmap.c > index 7e2575d669a9..432601154583 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > if (flags & TTU_SYNC) > pvmw.flags = PVMW_SYNC; > > - if (flags & TTU_SPLIT_HUGE_PMD) > - split_huge_pmd_address(vma, address, false, folio); > - > /* > * For THP, we have to assume the worse case ie pmd for invalidation. > * For hugetlb, it could be much worse if we need to do pud > @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > range.end = vma_address_end(&pvmw); > mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, > address, range.end); > + if (flags & TTU_SPLIT_HUGE_PMD) > + align_huge_pmd_range(vma, &range.start, &range.end); I am not sure why need this alignment? (1) For a partially mapped THP, 'range.start' and 'range.end' can beyond the VMA limits. For a PMD mapped THP, I think the address is already THP size alignment returned from vma_address(&folio->page, vma). (2) The range.end is not used. > if (folio_test_hugetlb(folio)) { > /* > * If sharing is possible, start and end will be adjusted > @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > mmu_notifier_invalidate_range_start(&range); > > while (page_vma_mapped_walk(&pvmw)) { > - /* Unexpected PMD-mapped THP? */ > - VM_BUG_ON_FOLIO(!pvmw.pte, folio); > - > /* > * If the folio is in an mlock()d vma, we must not swap it out. > */ > @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > goto walk_done_err; > } > > + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { > + /* > + * We temporarily have to drop the PTL and start once > + * again from that now-PTE-mapped page table. > + */ > + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, > + folio); > + pvmw.pmd = NULL; > + spin_unlock(pvmw.ptl); IMO, you should also make the 'pvmw.ptl = NULL;' after unlocking as page_vma_mapped_walk() did, in case some corner case met. > + flags &= ~TTU_SPLIT_HUGE_PMD; > + continue; > + } > + > + /* Unexpected PMD-mapped THP? */ > + VM_BUG_ON_FOLIO(!pvmw.pte, folio); > + > pfn = pte_pfn(ptep_get(pvmw.pte)); > subpage = folio_page(folio, pfn - folio_pfn(folio)); > address = pvmw.address;
Hey Baolin, Thanks for taking time to review! On Tue, May 7, 2024 at 11:40 AM Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > > > > On 2024/5/1 12:26, Lance Yang wrote: > > In preparation for supporting try_to_unmap_one() to unmap PMD-mapped > > folios, start the pagewalk first, then call split_huge_pmd_address() > > to split the folio. > > > > Suggested-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Lance Yang <ioworker0@gmail.com> > > --- > > include/linux/huge_mm.h | 20 ++++++++++++++++++++ > > mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- > > mm/rmap.c | 24 +++++++++++++++++------ > > 3 files changed, 60 insertions(+), 26 deletions(-) > > > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > > index c8d3ec116e29..38c4b5537715 100644 > > --- a/include/linux/huge_mm.h > > +++ b/include/linux/huge_mm.h > > @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) > > return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); > > } > > > > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > > + pmd_t *pmd, bool freeze, struct folio *folio); > > + > > +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > > + unsigned long *start, > > + unsigned long *end) > > +{ > > + *start = ALIGN(*start, HPAGE_PMD_SIZE); > > + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); > > + > > + VM_WARN_ON_ONCE(vma->vm_start > *start); > > + VM_WARN_ON_ONCE(vma->vm_end < *end); > > +} > > + > > #else /* CONFIG_TRANSPARENT_HUGEPAGE */ > > > > static inline bool folio_test_pmd_mappable(struct folio *folio) > > @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > > unsigned long address, bool freeze, struct folio *folio) {} > > static inline void split_huge_pmd_address(struct vm_area_struct *vma, > > unsigned long address, bool freeze, struct folio *folio) {} > > +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, > > + unsigned long address, pmd_t *pmd, > > + bool freeze, struct folio *folio) {} > > +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > > + unsigned long *start, > > + unsigned long *end) {} > > > > #define split_huge_pud(__vma, __pmd, __address) \ > > do { } while (0) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index 8261b5669397..145505a1dd05 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, > > pmd_populate(mm, pmd, pgtable); > > } > > > > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > > + pmd_t *pmd, bool freeze, struct folio *folio) > > +{ > > + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); > > + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); > > + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > > + VM_BUG_ON(freeze && !folio); > > + > > + /* > > + * When the caller requests to set up a migration entry, we > > + * require a folio to check the PMD against. Otherwise, there > > + * is a risk of replacing the wrong folio. > > + */ > > + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > > + is_pmd_migration_entry(*pmd)) { > > + if (folio && folio != pmd_folio(*pmd)) > > + return; > > + __split_huge_pmd_locked(vma, pmd, address, freeze); > > + } > > +} > > + > > void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > > unsigned long address, bool freeze, struct folio *folio) > > { > > @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > > (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); > > mmu_notifier_invalidate_range_start(&range); > > ptl = pmd_lock(vma->vm_mm, pmd); > > - > > - /* > > - * If caller asks to setup a migration entry, we need a folio to check > > - * pmd against. Otherwise we can end up replacing wrong folio. > > - */ > > - VM_BUG_ON(freeze && !folio); > > - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > > - > > - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > > - is_pmd_migration_entry(*pmd)) { > > - /* > > - * It's safe to call pmd_page when folio is set because it's > > - * guaranteed that pmd is present. > > - */ > > - if (folio && folio != pmd_folio(*pmd)) > > - goto out; > > - __split_huge_pmd_locked(vma, pmd, range.start, freeze); > > - } > > - > > -out: > > + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); > > spin_unlock(ptl); > > mmu_notifier_invalidate_range_end(&range); > > } > > diff --git a/mm/rmap.c b/mm/rmap.c > > index 7e2575d669a9..432601154583 100644 > > --- a/mm/rmap.c > > +++ b/mm/rmap.c > > @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > > if (flags & TTU_SYNC) > > pvmw.flags = PVMW_SYNC; > > > > - if (flags & TTU_SPLIT_HUGE_PMD) > > - split_huge_pmd_address(vma, address, false, folio); > > - > > /* > > * For THP, we have to assume the worse case ie pmd for invalidation. > > * For hugetlb, it could be much worse if we need to do pud > > @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > > range.end = vma_address_end(&pvmw); > > mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, > > address, range.end); > > + if (flags & TTU_SPLIT_HUGE_PMD) > > + align_huge_pmd_range(vma, &range.start, &range.end); > > I am not sure why need this alignment? > (1) For a partially mapped THP, 'range.start' and 'range.end' can beyond > the VMA limits. For a PMD mapped THP, I think the address is already THP > size alignment returned from vma_address(&folio->page, vma). > (2) The range.end is not used. Thanks for pointing that out! I agree that this alignment is indeed redundant, and we should remove it. Especially for a partially mapped THP, it could cause 'range.start' and 'range.end' beyond the VMA limits, which could lead us into trouble. > > > if (folio_test_hugetlb(folio)) { > > /* > > * If sharing is possible, start and end will be adjusted > > @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > > mmu_notifier_invalidate_range_start(&range); > > > > while (page_vma_mapped_walk(&pvmw)) { > > - /* Unexpected PMD-mapped THP? */ > > - VM_BUG_ON_FOLIO(!pvmw.pte, folio); > > - > > /* > > * If the folio is in an mlock()d vma, we must not swap it out. > > */ > > @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > > goto walk_done_err; > > } > > > > + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { > > + /* > > + * We temporarily have to drop the PTL and start once > > + * again from that now-PTE-mapped page table. > > + */ > > + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, > > + folio); > > + pvmw.pmd = NULL; > > + spin_unlock(pvmw.ptl); > > IMO, you should also make the 'pvmw.ptl = NULL;' after unlocking as > page_vma_mapped_walk() did, in case some corner case met. Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise. Thanks again for the review! Lance > > > + flags &= ~TTU_SPLIT_HUGE_PMD; > > + continue; > > + } > > + > > + /* Unexpected PMD-mapped THP? */ > > + VM_BUG_ON_FOLIO(!pvmw.pte, folio); > > + > > pfn = pte_pfn(ptep_get(pvmw.pte)); > > subpage = folio_page(folio, pfn - folio_pfn(folio)); > > address = pvmw.address;
>>> >>> + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { >>> + /* >>> + * We temporarily have to drop the PTL and start once >>> + * again from that now-PTE-mapped page table. >>> + */ >>> + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, >>> + folio); >>> + pvmw.pmd = NULL; >>> + spin_unlock(pvmw.ptl); >> >> IMO, you should also make the 'pvmw.ptl = NULL;' after unlocking as >> page_vma_mapped_walk() did, in case some corner case met. > > Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise. > This series already resides in mm-stable. I asked Andrew to remove it for now. If that doesn't work, we'll need fixup patches to address any review feedback.
Hey David, Thanks for reaching out! On Tue, May 7, 2024 at 4:17 PM David Hildenbrand <david@redhat.com> wrote: > > >>> > >>> + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { > >>> + /* > >>> + * We temporarily have to drop the PTL and start once > >>> + * again from that now-PTE-mapped page table. > >>> + */ > >>> + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, > >>> + folio); > >>> + pvmw.pmd = NULL; > >>> + spin_unlock(pvmw.ptl); > >> > >> IMO, you should also make the 'pvmw.ptl = NULL;' after unlocking as > >> page_vma_mapped_walk() did, in case some corner case met. > > > > Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise. > > > > This series already resides in mm-stable. I asked Andrew to remove it > for now. If that doesn't work, we'll need fixup patches to address any > review feedback. I'll patiently wait Andrew's response, and then submit the next version or fixup patches accordingly. Thanks, Lance > > -- > Cheers, > > David / dhildenb >
On 1 May 2024, at 0:26, Lance Yang wrote: > In preparation for supporting try_to_unmap_one() to unmap PMD-mapped > folios, start the pagewalk first, then call split_huge_pmd_address() > to split the folio. > > Suggested-by: David Hildenbrand <david@redhat.com> > Signed-off-by: Lance Yang <ioworker0@gmail.com> > --- > include/linux/huge_mm.h | 20 ++++++++++++++++++++ > mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- > mm/rmap.c | 24 +++++++++++++++++------ > 3 files changed, 60 insertions(+), 26 deletions(-) > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > index c8d3ec116e29..38c4b5537715 100644 > --- a/include/linux/huge_mm.h > +++ b/include/linux/huge_mm.h > @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) > return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); > } > > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > + pmd_t *pmd, bool freeze, struct folio *folio); > + > +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > + unsigned long *start, > + unsigned long *end) > +{ > + *start = ALIGN(*start, HPAGE_PMD_SIZE); > + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); > + > + VM_WARN_ON_ONCE(vma->vm_start > *start); > + VM_WARN_ON_ONCE(vma->vm_end < *end); > +} > + > #else /* CONFIG_TRANSPARENT_HUGEPAGE */ > > static inline bool folio_test_pmd_mappable(struct folio *folio) > @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > unsigned long address, bool freeze, struct folio *folio) {} > static inline void split_huge_pmd_address(struct vm_area_struct *vma, > unsigned long address, bool freeze, struct folio *folio) {} > +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, > + unsigned long address, pmd_t *pmd, > + bool freeze, struct folio *folio) {} > +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > + unsigned long *start, > + unsigned long *end) {} > > #define split_huge_pud(__vma, __pmd, __address) \ > do { } while (0) > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 8261b5669397..145505a1dd05 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, > pmd_populate(mm, pmd, pgtable); > } > > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > + pmd_t *pmd, bool freeze, struct folio *folio) > +{ > + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); > + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); > + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > + VM_BUG_ON(freeze && !folio); > + > + /* > + * When the caller requests to set up a migration entry, we > + * require a folio to check the PMD against. Otherwise, there > + * is a risk of replacing the wrong folio. > + */ > + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > + is_pmd_migration_entry(*pmd)) { > + if (folio && folio != pmd_folio(*pmd)) > + return; > + __split_huge_pmd_locked(vma, pmd, address, freeze); > + } > +} > + > void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > unsigned long address, bool freeze, struct folio *folio) > { > @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); > mmu_notifier_invalidate_range_start(&range); > ptl = pmd_lock(vma->vm_mm, pmd); > - > - /* > - * If caller asks to setup a migration entry, we need a folio to check > - * pmd against. Otherwise we can end up replacing wrong folio. > - */ > - VM_BUG_ON(freeze && !folio); > - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > - > - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > - is_pmd_migration_entry(*pmd)) { > - /* > - * It's safe to call pmd_page when folio is set because it's > - * guaranteed that pmd is present. > - */ > - if (folio && folio != pmd_folio(*pmd)) > - goto out; > - __split_huge_pmd_locked(vma, pmd, range.start, freeze); > - } > - > -out: > + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); > spin_unlock(ptl); > mmu_notifier_invalidate_range_end(&range); > } > diff --git a/mm/rmap.c b/mm/rmap.c > index 7e2575d669a9..432601154583 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > if (flags & TTU_SYNC) > pvmw.flags = PVMW_SYNC; > > - if (flags & TTU_SPLIT_HUGE_PMD) > - split_huge_pmd_address(vma, address, false, folio); > - > /* > * For THP, we have to assume the worse case ie pmd for invalidation. > * For hugetlb, it could be much worse if we need to do pud > @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > range.end = vma_address_end(&pvmw); > mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, > address, range.end); > + if (flags & TTU_SPLIT_HUGE_PMD) > + align_huge_pmd_range(vma, &range.start, &range.end); > if (folio_test_hugetlb(folio)) { > /* > * If sharing is possible, start and end will be adjusted > @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > mmu_notifier_invalidate_range_start(&range); > > while (page_vma_mapped_walk(&pvmw)) { > - /* Unexpected PMD-mapped THP? */ > - VM_BUG_ON_FOLIO(!pvmw.pte, folio); > - > /* > * If the folio is in an mlock()d vma, we must not swap it out. > */ > @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > goto walk_done_err; > } > > + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { > + /* > + * We temporarily have to drop the PTL and start once > + * again from that now-PTE-mapped page table. > + */ > + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, > + folio); Just in case you might miss here, since you will no longer align range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE in split_huge_pmd_locked(), you will need to align the input address now. > + pvmw.pmd = NULL; > + spin_unlock(pvmw.ptl); > + flags &= ~TTU_SPLIT_HUGE_PMD; > + continue; > + } > + > + /* Unexpected PMD-mapped THP? */ > + VM_BUG_ON_FOLIO(!pvmw.pte, folio); > + > pfn = pte_pfn(ptep_get(pvmw.pte)); > subpage = folio_page(folio, pfn - folio_pfn(folio)); > address = pvmw.address; > -- > 2.33.1 -- Best Regards, Yan, Zi
On Tue, 7 May 2024 16:38:07 +0800 Lance Yang <ioworker0@gmail.com> wrote: > > > Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise. > > > > > > > This series already resides in mm-stable. I asked Andrew to remove it > > for now. If that doesn't work, we'll need fixup patches to address any > > review feedback. > > I'll patiently wait Andrew's response, and then submit the next version or > fixup patches accordingly. Well, which series are we talking about? "mm/madvise: enhance lazyfreeing with mTHP in madvise_free v10" or ""Reclaim lazyfree THP without splitting v4" or both? And how significant are the needed fixup patches? And what is our confidence level after those fixups are in place?
On 07.05.24 19:22, Andrew Morton wrote: > On Tue, 7 May 2024 16:38:07 +0800 Lance Yang <ioworker0@gmail.com> wrote: > >>>> Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise. >>>> >>> >>> This series already resides in mm-stable. I asked Andrew to remove it >>> for now. If that doesn't work, we'll need fixup patches to address any >>> review feedback. >> >> I'll patiently wait Andrew's response, and then submit the next version or >> fixup patches accordingly. > > Well, which series are we talking about? "mm/madvise: enhance > lazyfreeing with mTHP in madvise_free v10" or ""Reclaim lazyfree THP > without splitting v4" or both? See my other mail, "mm/madvise: enhance lazyfreeing with mTHP in madvise_free v10" is all acked/reviewed and good to go. > > And how significant are the needed fixup patches? > > And what is our confidence level after those fixups are in place? I'm afraid I won't have time to review this series this/next week, so I cannot tell. I already assumed this would not be 6.10 material.
On Tue, 7 May 2024 19:33:05 +0200 David Hildenbrand <david@redhat.com> wrote: > > Well, which series are we talking about? "mm/madvise: enhance > > lazyfreeing with mTHP in madvise_free v10" or ""Reclaim lazyfree THP > > without splitting v4" or both? > > See my other mail, "mm/madvise: enhance lazyfreeing with mTHP in > madvise_free v10" is all acked/reviewed and good to go. > > > > > And how significant are the needed fixup patches? > > > > And what is our confidence level after those fixups are in place? > > I'm afraid I won't have time to review this series this/next week, so I > cannot tell. I already assumed this would not be 6.10 material. OK, I've dropped the series "Reclaim lazyfree THP without splitting", v4. Let's revisit in the next cycle.
On 07.05.24 19:38, Andrew Morton wrote: > On Tue, 7 May 2024 19:33:05 +0200 David Hildenbrand <david@redhat.com> wrote: > >>> Well, which series are we talking about? "mm/madvise: enhance >>> lazyfreeing with mTHP in madvise_free v10" or ""Reclaim lazyfree THP >>> without splitting v4" or both? >> >> See my other mail, "mm/madvise: enhance lazyfreeing with mTHP in >> madvise_free v10" is all acked/reviewed and good to go. >> >>> >>> And how significant are the needed fixup patches? >>> >>> And what is our confidence level after those fixups are in place? >> >> I'm afraid I won't have time to review this series this/next week, so I >> cannot tell. I already assumed this would not be 6.10 material. > > OK, I've dropped the series "Reclaim lazyfree THP without splitting", > v4. Let's revisit in the next cycle. Thanks, should be more than ready by then :)
On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote: > > On 1 May 2024, at 0:26, Lance Yang wrote: > > > In preparation for supporting try_to_unmap_one() to unmap PMD-mapped > > folios, start the pagewalk first, then call split_huge_pmd_address() > > to split the folio. > > > > Suggested-by: David Hildenbrand <david@redhat.com> > > Signed-off-by: Lance Yang <ioworker0@gmail.com> > > --- > > include/linux/huge_mm.h | 20 ++++++++++++++++++++ > > mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- > > mm/rmap.c | 24 +++++++++++++++++------ > > 3 files changed, 60 insertions(+), 26 deletions(-) > > > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > > index c8d3ec116e29..38c4b5537715 100644 > > --- a/include/linux/huge_mm.h > > +++ b/include/linux/huge_mm.h > > @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) > > return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); > > } > > > > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > > + pmd_t *pmd, bool freeze, struct folio *folio); > > + > > +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > > + unsigned long *start, > > + unsigned long *end) > > +{ > > + *start = ALIGN(*start, HPAGE_PMD_SIZE); > > + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); > > + > > + VM_WARN_ON_ONCE(vma->vm_start > *start); > > + VM_WARN_ON_ONCE(vma->vm_end < *end); > > +} > > + > > #else /* CONFIG_TRANSPARENT_HUGEPAGE */ > > > > static inline bool folio_test_pmd_mappable(struct folio *folio) > > @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > > unsigned long address, bool freeze, struct folio *folio) {} > > static inline void split_huge_pmd_address(struct vm_area_struct *vma, > > unsigned long address, bool freeze, struct folio *folio) {} > > +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, > > + unsigned long address, pmd_t *pmd, > > + bool freeze, struct folio *folio) {} > > +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > > + unsigned long *start, > > + unsigned long *end) {} > > > > #define split_huge_pud(__vma, __pmd, __address) \ > > do { } while (0) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index 8261b5669397..145505a1dd05 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, > > pmd_populate(mm, pmd, pgtable); > > } > > > > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > > + pmd_t *pmd, bool freeze, struct folio *folio) > > +{ > > + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); > > + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); > > + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > > + VM_BUG_ON(freeze && !folio); > > + > > + /* > > + * When the caller requests to set up a migration entry, we > > + * require a folio to check the PMD against. Otherwise, there > > + * is a risk of replacing the wrong folio. > > + */ > > + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > > + is_pmd_migration_entry(*pmd)) { > > + if (folio && folio != pmd_folio(*pmd)) > > + return; > > + __split_huge_pmd_locked(vma, pmd, address, freeze); > > + } > > +} > > + > > void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > > unsigned long address, bool freeze, struct folio *folio) > > { > > @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > > (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); > > mmu_notifier_invalidate_range_start(&range); > > ptl = pmd_lock(vma->vm_mm, pmd); > > - > > - /* > > - * If caller asks to setup a migration entry, we need a folio to check > > - * pmd against. Otherwise we can end up replacing wrong folio. > > - */ > > - VM_BUG_ON(freeze && !folio); > > - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > > - > > - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > > - is_pmd_migration_entry(*pmd)) { > > - /* > > - * It's safe to call pmd_page when folio is set because it's > > - * guaranteed that pmd is present. > > - */ > > - if (folio && folio != pmd_folio(*pmd)) > > - goto out; > > - __split_huge_pmd_locked(vma, pmd, range.start, freeze); > > - } > > - > > -out: > > + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); > > spin_unlock(ptl); > > mmu_notifier_invalidate_range_end(&range); > > } > > diff --git a/mm/rmap.c b/mm/rmap.c > > index 7e2575d669a9..432601154583 100644 > > --- a/mm/rmap.c > > +++ b/mm/rmap.c > > @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > > if (flags & TTU_SYNC) > > pvmw.flags = PVMW_SYNC; > > > > - if (flags & TTU_SPLIT_HUGE_PMD) > > - split_huge_pmd_address(vma, address, false, folio); > > - > > /* > > * For THP, we have to assume the worse case ie pmd for invalidation. > > * For hugetlb, it could be much worse if we need to do pud > > @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > > range.end = vma_address_end(&pvmw); > > mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, > > address, range.end); > > + if (flags & TTU_SPLIT_HUGE_PMD) > > + align_huge_pmd_range(vma, &range.start, &range.end); > > if (folio_test_hugetlb(folio)) { > > /* > > * If sharing is possible, start and end will be adjusted > > @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > > mmu_notifier_invalidate_range_start(&range); > > > > while (page_vma_mapped_walk(&pvmw)) { > > - /* Unexpected PMD-mapped THP? */ > > - VM_BUG_ON_FOLIO(!pvmw.pte, folio); > > - > > /* > > * If the folio is in an mlock()d vma, we must not swap it out. > > */ > > @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > > goto walk_done_err; > > } > > > > + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { > > + /* > > + * We temporarily have to drop the PTL and start once > > + * again from that now-PTE-mapped page table. > > + */ > > + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, > > + folio); > > Just in case you might miss here, since you will no longer align > range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE > in split_huge_pmd_locked(), you will need to align the input address now. Thanks for bringing that up! I do miss the alignment here when I decide to no longer align range.start in another email - thanks! Zi, could I move the alignment here? IIUC, we will not encounter a partially mapped THP here, and range.start and range.end should also not beyond the VMA limits. align_huge_pmd_range(vma, &range.start, &range.end); split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, folio); Thanks, Lance > > > + pvmw.pmd = NULL; > > + spin_unlock(pvmw.ptl); > > + flags &= ~TTU_SPLIT_HUGE_PMD; > > + continue; > > + } > > + > > + /* Unexpected PMD-mapped THP? */ > > + VM_BUG_ON_FOLIO(!pvmw.pte, folio); > > + > > pfn = pte_pfn(ptep_get(pvmw.pte)); > > subpage = folio_page(folio, pfn - folio_pfn(folio)); > > address = pvmw.address; > > -- > > 2.33.1 > > > -- > Best Regards, > Yan, Zi
On 8 May 2024, at 1:43, Lance Yang wrote: > On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote: >> >> On 1 May 2024, at 0:26, Lance Yang wrote: >> >>> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped >>> folios, start the pagewalk first, then call split_huge_pmd_address() >>> to split the folio. >>> >>> Suggested-by: David Hildenbrand <david@redhat.com> >>> Signed-off-by: Lance Yang <ioworker0@gmail.com> >>> --- >>> include/linux/huge_mm.h | 20 ++++++++++++++++++++ >>> mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- >>> mm/rmap.c | 24 +++++++++++++++++------ >>> 3 files changed, 60 insertions(+), 26 deletions(-) >>> >>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >>> index c8d3ec116e29..38c4b5537715 100644 >>> --- a/include/linux/huge_mm.h >>> +++ b/include/linux/huge_mm.h >>> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) >>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); >>> } >>> >>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, >>> + pmd_t *pmd, bool freeze, struct folio *folio); >>> + >>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma, >>> + unsigned long *start, >>> + unsigned long *end) >>> +{ >>> + *start = ALIGN(*start, HPAGE_PMD_SIZE); >>> + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); >>> + >>> + VM_WARN_ON_ONCE(vma->vm_start > *start); >>> + VM_WARN_ON_ONCE(vma->vm_end < *end); >>> +} >>> + >>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */ >>> >>> static inline bool folio_test_pmd_mappable(struct folio *folio) >>> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>> unsigned long address, bool freeze, struct folio *folio) {} >>> static inline void split_huge_pmd_address(struct vm_area_struct *vma, >>> unsigned long address, bool freeze, struct folio *folio) {} >>> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, >>> + unsigned long address, pmd_t *pmd, >>> + bool freeze, struct folio *folio) {} >>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma, >>> + unsigned long *start, >>> + unsigned long *end) {} >>> >>> #define split_huge_pud(__vma, __pmd, __address) \ >>> do { } while (0) >>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>> index 8261b5669397..145505a1dd05 100644 >>> --- a/mm/huge_memory.c >>> +++ b/mm/huge_memory.c >>> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, >>> pmd_populate(mm, pmd, pgtable); >>> } >>> >>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, >>> + pmd_t *pmd, bool freeze, struct folio *folio) >>> +{ >>> + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); >>> + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); >>> + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); >>> + VM_BUG_ON(freeze && !folio); >>> + >>> + /* >>> + * When the caller requests to set up a migration entry, we >>> + * require a folio to check the PMD against. Otherwise, there >>> + * is a risk of replacing the wrong folio. >>> + */ >>> + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || >>> + is_pmd_migration_entry(*pmd)) { >>> + if (folio && folio != pmd_folio(*pmd)) >>> + return; >>> + __split_huge_pmd_locked(vma, pmd, address, freeze); >>> + } >>> +} >>> + >>> void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>> unsigned long address, bool freeze, struct folio *folio) >>> { >>> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>> (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); >>> mmu_notifier_invalidate_range_start(&range); >>> ptl = pmd_lock(vma->vm_mm, pmd); >>> - >>> - /* >>> - * If caller asks to setup a migration entry, we need a folio to check >>> - * pmd against. Otherwise we can end up replacing wrong folio. >>> - */ >>> - VM_BUG_ON(freeze && !folio); >>> - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); >>> - >>> - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || >>> - is_pmd_migration_entry(*pmd)) { >>> - /* >>> - * It's safe to call pmd_page when folio is set because it's >>> - * guaranteed that pmd is present. >>> - */ >>> - if (folio && folio != pmd_folio(*pmd)) >>> - goto out; >>> - __split_huge_pmd_locked(vma, pmd, range.start, freeze); >>> - } >>> - >>> -out: >>> + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); >>> spin_unlock(ptl); >>> mmu_notifier_invalidate_range_end(&range); >>> } >>> diff --git a/mm/rmap.c b/mm/rmap.c >>> index 7e2575d669a9..432601154583 100644 >>> --- a/mm/rmap.c >>> +++ b/mm/rmap.c >>> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>> if (flags & TTU_SYNC) >>> pvmw.flags = PVMW_SYNC; >>> >>> - if (flags & TTU_SPLIT_HUGE_PMD) >>> - split_huge_pmd_address(vma, address, false, folio); >>> - >>> /* >>> * For THP, we have to assume the worse case ie pmd for invalidation. >>> * For hugetlb, it could be much worse if we need to do pud >>> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>> range.end = vma_address_end(&pvmw); >>> mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, >>> address, range.end); >>> + if (flags & TTU_SPLIT_HUGE_PMD) >>> + align_huge_pmd_range(vma, &range.start, &range.end); >>> if (folio_test_hugetlb(folio)) { >>> /* >>> * If sharing is possible, start and end will be adjusted >>> @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>> mmu_notifier_invalidate_range_start(&range); >>> >>> while (page_vma_mapped_walk(&pvmw)) { >>> - /* Unexpected PMD-mapped THP? */ >>> - VM_BUG_ON_FOLIO(!pvmw.pte, folio); >>> - >>> /* >>> * If the folio is in an mlock()d vma, we must not swap it out. >>> */ >>> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>> goto walk_done_err; >>> } >>> >>> + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { >>> + /* >>> + * We temporarily have to drop the PTL and start once >>> + * again from that now-PTE-mapped page table. >>> + */ >>> + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, >>> + folio); >> >> Just in case you might miss here, since you will no longer align >> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE >> in split_huge_pmd_locked(), you will need to align the input address now. > > Thanks for bringing that up! > > I do miss the alignment here when I decide to no longer align range.start > in another email - thanks! > No problem. > Zi, could I move the alignment here? > IIUC, we will not encounter a partially mapped THP here, and range.start > and range.end should also not beyond the VMA limits. > > align_huge_pmd_range(vma, &range.start, &range.end); > split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, > folio); I think you can just do split_huge_pmd_locked(vma, ALIGN(range.start, HPAGE_PMD_SIZE), pvmw.pmd, false, folio); since range will later be used by mmu_notifier_invalidate_range_end() and changing it might cause secondary TLB invalidation issues. > > Thanks, > Lance > >> >>> + pvmw.pmd = NULL; >>> + spin_unlock(pvmw.ptl); >>> + flags &= ~TTU_SPLIT_HUGE_PMD; >>> + continue; >>> + } >>> + >>> + /* Unexpected PMD-mapped THP? */ >>> + VM_BUG_ON_FOLIO(!pvmw.pte, folio); >>> + >>> pfn = pte_pfn(ptep_get(pvmw.pte)); >>> subpage = folio_page(folio, pfn - folio_pfn(folio)); >>> address = pvmw.address; >>> -- >>> 2.33.1 >> >> >> -- >> Best Regards, >> Yan, Zi -- Best Regards, Yan, Zi
On Wed, May 8, 2024 at 10:07 PM Zi Yan <ziy@nvidia.com> wrote: > > On 8 May 2024, at 1:43, Lance Yang wrote: > > > On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote: > >> > >> On 1 May 2024, at 0:26, Lance Yang wrote: > >> > >>> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped > >>> folios, start the pagewalk first, then call split_huge_pmd_address() > >>> to split the folio. > >>> > >>> Suggested-by: David Hildenbrand <david@redhat.com> > >>> Signed-off-by: Lance Yang <ioworker0@gmail.com> > >>> --- > >>> include/linux/huge_mm.h | 20 ++++++++++++++++++++ > >>> mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- > >>> mm/rmap.c | 24 +++++++++++++++++------ > >>> 3 files changed, 60 insertions(+), 26 deletions(-) > >>> > >>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h > >>> index c8d3ec116e29..38c4b5537715 100644 > >>> --- a/include/linux/huge_mm.h > >>> +++ b/include/linux/huge_mm.h > >>> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) > >>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); > >>> } > >>> > >>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > >>> + pmd_t *pmd, bool freeze, struct folio *folio); > >>> + > >>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > >>> + unsigned long *start, > >>> + unsigned long *end) > >>> +{ > >>> + *start = ALIGN(*start, HPAGE_PMD_SIZE); > >>> + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); > >>> + > >>> + VM_WARN_ON_ONCE(vma->vm_start > *start); > >>> + VM_WARN_ON_ONCE(vma->vm_end < *end); > >>> +} > >>> + > >>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */ > >>> > >>> static inline bool folio_test_pmd_mappable(struct folio *folio) > >>> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > >>> unsigned long address, bool freeze, struct folio *folio) {} > >>> static inline void split_huge_pmd_address(struct vm_area_struct *vma, > >>> unsigned long address, bool freeze, struct folio *folio) {} > >>> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, > >>> + unsigned long address, pmd_t *pmd, > >>> + bool freeze, struct folio *folio) {} > >>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma, > >>> + unsigned long *start, > >>> + unsigned long *end) {} > >>> > >>> #define split_huge_pud(__vma, __pmd, __address) \ > >>> do { } while (0) > >>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >>> index 8261b5669397..145505a1dd05 100644 > >>> --- a/mm/huge_memory.c > >>> +++ b/mm/huge_memory.c > >>> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, > >>> pmd_populate(mm, pmd, pgtable); > >>> } > >>> > >>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, > >>> + pmd_t *pmd, bool freeze, struct folio *folio) > >>> +{ > >>> + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); > >>> + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); > >>> + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > >>> + VM_BUG_ON(freeze && !folio); > >>> + > >>> + /* > >>> + * When the caller requests to set up a migration entry, we > >>> + * require a folio to check the PMD against. Otherwise, there > >>> + * is a risk of replacing the wrong folio. > >>> + */ > >>> + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > >>> + is_pmd_migration_entry(*pmd)) { > >>> + if (folio && folio != pmd_folio(*pmd)) > >>> + return; > >>> + __split_huge_pmd_locked(vma, pmd, address, freeze); > >>> + } > >>> +} > >>> + > >>> void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > >>> unsigned long address, bool freeze, struct folio *folio) > >>> { > >>> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, > >>> (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); > >>> mmu_notifier_invalidate_range_start(&range); > >>> ptl = pmd_lock(vma->vm_mm, pmd); > >>> - > >>> - /* > >>> - * If caller asks to setup a migration entry, we need a folio to check > >>> - * pmd against. Otherwise we can end up replacing wrong folio. > >>> - */ > >>> - VM_BUG_ON(freeze && !folio); > >>> - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); > >>> - > >>> - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || > >>> - is_pmd_migration_entry(*pmd)) { > >>> - /* > >>> - * It's safe to call pmd_page when folio is set because it's > >>> - * guaranteed that pmd is present. > >>> - */ > >>> - if (folio && folio != pmd_folio(*pmd)) > >>> - goto out; > >>> - __split_huge_pmd_locked(vma, pmd, range.start, freeze); > >>> - } > >>> - > >>> -out: > >>> + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); > >>> spin_unlock(ptl); > >>> mmu_notifier_invalidate_range_end(&range); > >>> } > >>> diff --git a/mm/rmap.c b/mm/rmap.c > >>> index 7e2575d669a9..432601154583 100644 > >>> --- a/mm/rmap.c > >>> +++ b/mm/rmap.c > >>> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > >>> if (flags & TTU_SYNC) > >>> pvmw.flags = PVMW_SYNC; > >>> > >>> - if (flags & TTU_SPLIT_HUGE_PMD) > >>> - split_huge_pmd_address(vma, address, false, folio); > >>> - > >>> /* > >>> * For THP, we have to assume the worse case ie pmd for invalidation. > >>> * For hugetlb, it could be much worse if we need to do pud > >>> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > >>> range.end = vma_address_end(&pvmw); > >>> mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, > >>> address, range.end); > >>> + if (flags & TTU_SPLIT_HUGE_PMD) > >>> + align_huge_pmd_range(vma, &range.start, &range.end); > >>> if (folio_test_hugetlb(folio)) { > >>> /* > >>> * If sharing is possible, start and end will be adjusted > >>> @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > >>> mmu_notifier_invalidate_range_start(&range); > >>> > >>> while (page_vma_mapped_walk(&pvmw)) { > >>> - /* Unexpected PMD-mapped THP? */ > >>> - VM_BUG_ON_FOLIO(!pvmw.pte, folio); > >>> - > >>> /* > >>> * If the folio is in an mlock()d vma, we must not swap it out. > >>> */ > >>> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, > >>> goto walk_done_err; > >>> } > >>> > >>> + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { > >>> + /* > >>> + * We temporarily have to drop the PTL and start once > >>> + * again from that now-PTE-mapped page table. > >>> + */ > >>> + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, > >>> + folio); > >> > >> Just in case you might miss here, since you will no longer align > >> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE > >> in split_huge_pmd_locked(), you will need to align the input address now. > > > > Thanks for bringing that up! > > > > I do miss the alignment here when I decide to no longer align range.start > > in another email - thanks! > > > No problem. > > > Zi, could I move the alignment here? > > IIUC, we will not encounter a partially mapped THP here, and range.start > > and range.end should also not beyond the VMA limits. > > > > align_huge_pmd_range(vma, &range.start, &range.end); > > split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, > > folio); > > I think you can just do > > split_huge_pmd_locked(vma, ALIGN(range.start, HPAGE_PMD_SIZE), pvmw.pmd, false, folio); > > since range will later be used by mmu_notifier_invalidate_range_end() and changing > it might cause secondary TLB invalidation issues. Ok, makes sense to me - thanks! But we probably cannot use the HPAGE_PMD_SIZE here; it will cause broken compilation as seen in v3[1]. Perhaps we still need to add a new alignment function for the huge PMD? [1] https://lore.kernel.org/linux-mm/20240429202040.187453-1-sj@kernel.org/ Thanks again for the review! Best, Lance > > > > > Thanks, > > Lance > > > >> > >>> + pvmw.pmd = NULL; > >>> + spin_unlock(pvmw.ptl); > >>> + flags &= ~TTU_SPLIT_HUGE_PMD; > >>> + continue; > >>> + } > >>> + > >>> + /* Unexpected PMD-mapped THP? */ > >>> + VM_BUG_ON_FOLIO(!pvmw.pte, folio); > >>> + > >>> pfn = pte_pfn(ptep_get(pvmw.pte)); > >>> subpage = folio_page(folio, pfn - folio_pfn(folio)); > >>> address = pvmw.address; > >>> -- > >>> 2.33.1 > >> > >> > >> -- > >> Best Regards, > >> Yan, Zi > > > -- > Best Regards, > Yan, Zi
On 8 May 2024, at 10:35, Lance Yang wrote: > On Wed, May 8, 2024 at 10:07 PM Zi Yan <ziy@nvidia.com> wrote: >> >> On 8 May 2024, at 1:43, Lance Yang wrote: >> >>> On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote: >>>> >>>> On 1 May 2024, at 0:26, Lance Yang wrote: >>>> >>>>> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped >>>>> folios, start the pagewalk first, then call split_huge_pmd_address() >>>>> to split the folio. >>>>> >>>>> Suggested-by: David Hildenbrand <david@redhat.com> >>>>> Signed-off-by: Lance Yang <ioworker0@gmail.com> >>>>> --- >>>>> include/linux/huge_mm.h | 20 ++++++++++++++++++++ >>>>> mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- >>>>> mm/rmap.c | 24 +++++++++++++++++------ >>>>> 3 files changed, 60 insertions(+), 26 deletions(-) >>>>> >>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >>>>> index c8d3ec116e29..38c4b5537715 100644 >>>>> --- a/include/linux/huge_mm.h >>>>> +++ b/include/linux/huge_mm.h >>>>> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) >>>>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); >>>>> } >>>>> >>>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, >>>>> + pmd_t *pmd, bool freeze, struct folio *folio); >>>>> + >>>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma, >>>>> + unsigned long *start, >>>>> + unsigned long *end) >>>>> +{ >>>>> + *start = ALIGN(*start, HPAGE_PMD_SIZE); >>>>> + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); >>>>> + >>>>> + VM_WARN_ON_ONCE(vma->vm_start > *start); >>>>> + VM_WARN_ON_ONCE(vma->vm_end < *end); >>>>> +} >>>>> + >>>>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */ >>>>> >>>>> static inline bool folio_test_pmd_mappable(struct folio *folio) >>>>> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>>>> unsigned long address, bool freeze, struct folio *folio) {} >>>>> static inline void split_huge_pmd_address(struct vm_area_struct *vma, >>>>> unsigned long address, bool freeze, struct folio *folio) {} >>>>> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, >>>>> + unsigned long address, pmd_t *pmd, >>>>> + bool freeze, struct folio *folio) {} >>>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma, >>>>> + unsigned long *start, >>>>> + unsigned long *end) {} >>>>> >>>>> #define split_huge_pud(__vma, __pmd, __address) \ >>>>> do { } while (0) >>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>>> index 8261b5669397..145505a1dd05 100644 >>>>> --- a/mm/huge_memory.c >>>>> +++ b/mm/huge_memory.c >>>>> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, >>>>> pmd_populate(mm, pmd, pgtable); >>>>> } >>>>> >>>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, >>>>> + pmd_t *pmd, bool freeze, struct folio *folio) >>>>> +{ >>>>> + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); >>>>> + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); >>>>> + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); >>>>> + VM_BUG_ON(freeze && !folio); >>>>> + >>>>> + /* >>>>> + * When the caller requests to set up a migration entry, we >>>>> + * require a folio to check the PMD against. Otherwise, there >>>>> + * is a risk of replacing the wrong folio. >>>>> + */ >>>>> + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || >>>>> + is_pmd_migration_entry(*pmd)) { >>>>> + if (folio && folio != pmd_folio(*pmd)) >>>>> + return; >>>>> + __split_huge_pmd_locked(vma, pmd, address, freeze); >>>>> + } >>>>> +} >>>>> + >>>>> void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>>>> unsigned long address, bool freeze, struct folio *folio) >>>>> { >>>>> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>>>> (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); >>>>> mmu_notifier_invalidate_range_start(&range); >>>>> ptl = pmd_lock(vma->vm_mm, pmd); >>>>> - >>>>> - /* >>>>> - * If caller asks to setup a migration entry, we need a folio to check >>>>> - * pmd against. Otherwise we can end up replacing wrong folio. >>>>> - */ >>>>> - VM_BUG_ON(freeze && !folio); >>>>> - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); >>>>> - >>>>> - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || >>>>> - is_pmd_migration_entry(*pmd)) { >>>>> - /* >>>>> - * It's safe to call pmd_page when folio is set because it's >>>>> - * guaranteed that pmd is present. >>>>> - */ >>>>> - if (folio && folio != pmd_folio(*pmd)) >>>>> - goto out; >>>>> - __split_huge_pmd_locked(vma, pmd, range.start, freeze); >>>>> - } >>>>> - >>>>> -out: >>>>> + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); >>>>> spin_unlock(ptl); >>>>> mmu_notifier_invalidate_range_end(&range); >>>>> } >>>>> diff --git a/mm/rmap.c b/mm/rmap.c >>>>> index 7e2575d669a9..432601154583 100644 >>>>> --- a/mm/rmap.c >>>>> +++ b/mm/rmap.c >>>>> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>>>> if (flags & TTU_SYNC) >>>>> pvmw.flags = PVMW_SYNC; >>>>> >>>>> - if (flags & TTU_SPLIT_HUGE_PMD) >>>>> - split_huge_pmd_address(vma, address, false, folio); >>>>> - >>>>> /* >>>>> * For THP, we have to assume the worse case ie pmd for invalidation. >>>>> * For hugetlb, it could be much worse if we need to do pud >>>>> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>>>> range.end = vma_address_end(&pvmw); >>>>> mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, >>>>> address, range.end); >>>>> + if (flags & TTU_SPLIT_HUGE_PMD) >>>>> + align_huge_pmd_range(vma, &range.start, &range.end); >>>>> if (folio_test_hugetlb(folio)) { >>>>> /* >>>>> * If sharing is possible, start and end will be adjusted >>>>> @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>>>> mmu_notifier_invalidate_range_start(&range); >>>>> >>>>> while (page_vma_mapped_walk(&pvmw)) { >>>>> - /* Unexpected PMD-mapped THP? */ >>>>> - VM_BUG_ON_FOLIO(!pvmw.pte, folio); >>>>> - >>>>> /* >>>>> * If the folio is in an mlock()d vma, we must not swap it out. >>>>> */ >>>>> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>>>> goto walk_done_err; >>>>> } >>>>> >>>>> + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { >>>>> + /* >>>>> + * We temporarily have to drop the PTL and start once >>>>> + * again from that now-PTE-mapped page table. >>>>> + */ >>>>> + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, >>>>> + folio); >>>> >>>> Just in case you might miss here, since you will no longer align >>>> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE >>>> in split_huge_pmd_locked(), you will need to align the input address now. >>> >>> Thanks for bringing that up! >>> >>> I do miss the alignment here when I decide to no longer align range.start >>> in another email - thanks! >>> >> No problem. >> >>> Zi, could I move the alignment here? >>> IIUC, we will not encounter a partially mapped THP here, and range.start >>> and range.end should also not beyond the VMA limits. >>> >>> align_huge_pmd_range(vma, &range.start, &range.end); >>> split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, >>> folio); >> >> I think you can just do >> >> split_huge_pmd_locked(vma, ALIGN(range.start, HPAGE_PMD_SIZE), pvmw.pmd, false, folio); >> >> since range will later be used by mmu_notifier_invalidate_range_end() and changing >> it might cause secondary TLB invalidation issues. > > Ok, makes sense to me - thanks! > > But we probably cannot use the HPAGE_PMD_SIZE here; it will cause > broken compilation as seen in v3[1]. > > Perhaps we still need to add a new alignment function for the huge PMD? > > [1] https://lore.kernel.org/linux-mm/20240429202040.187453-1-sj@kernel.org/ > > Thanks again for the review! > Or you can adjust the alignment inside split_huge_pmd_locked(), since it can be called other than __split_huge_pmd(). Hmm, I notice that split_huge_pmd_address() has mmu_notifier ops but your split_huge_pmd_locked() does not include them, I wonder if that could cause issues with mmu_notifier issues. Adding mmu_notifier people to confirm. -- Best Regards, Yan, Zi
On 8 May 2024, at 10:48, Zi Yan wrote: > On 8 May 2024, at 10:35, Lance Yang wrote: > >> On Wed, May 8, 2024 at 10:07 PM Zi Yan <ziy@nvidia.com> wrote: >>> >>> On 8 May 2024, at 1:43, Lance Yang wrote: >>> >>>> On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote: >>>>> >>>>> On 1 May 2024, at 0:26, Lance Yang wrote: >>>>> >>>>>> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped >>>>>> folios, start the pagewalk first, then call split_huge_pmd_address() >>>>>> to split the folio. >>>>>> >>>>>> Suggested-by: David Hildenbrand <david@redhat.com> >>>>>> Signed-off-by: Lance Yang <ioworker0@gmail.com> >>>>>> --- >>>>>> include/linux/huge_mm.h | 20 ++++++++++++++++++++ >>>>>> mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- >>>>>> mm/rmap.c | 24 +++++++++++++++++------ >>>>>> 3 files changed, 60 insertions(+), 26 deletions(-) >>>>>> >>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >>>>>> index c8d3ec116e29..38c4b5537715 100644 >>>>>> --- a/include/linux/huge_mm.h >>>>>> +++ b/include/linux/huge_mm.h >>>>>> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) >>>>>> return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); >>>>>> } >>>>>> >>>>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, >>>>>> + pmd_t *pmd, bool freeze, struct folio *folio); >>>>>> + >>>>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma, >>>>>> + unsigned long *start, >>>>>> + unsigned long *end) >>>>>> +{ >>>>>> + *start = ALIGN(*start, HPAGE_PMD_SIZE); >>>>>> + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); >>>>>> + >>>>>> + VM_WARN_ON_ONCE(vma->vm_start > *start); >>>>>> + VM_WARN_ON_ONCE(vma->vm_end < *end); >>>>>> +} >>>>>> + >>>>>> #else /* CONFIG_TRANSPARENT_HUGEPAGE */ >>>>>> >>>>>> static inline bool folio_test_pmd_mappable(struct folio *folio) >>>>>> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>>>>> unsigned long address, bool freeze, struct folio *folio) {} >>>>>> static inline void split_huge_pmd_address(struct vm_area_struct *vma, >>>>>> unsigned long address, bool freeze, struct folio *folio) {} >>>>>> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, >>>>>> + unsigned long address, pmd_t *pmd, >>>>>> + bool freeze, struct folio *folio) {} >>>>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma, >>>>>> + unsigned long *start, >>>>>> + unsigned long *end) {} >>>>>> >>>>>> #define split_huge_pud(__vma, __pmd, __address) \ >>>>>> do { } while (0) >>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>>>> index 8261b5669397..145505a1dd05 100644 >>>>>> --- a/mm/huge_memory.c >>>>>> +++ b/mm/huge_memory.c >>>>>> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, >>>>>> pmd_populate(mm, pmd, pgtable); >>>>>> } >>>>>> >>>>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, >>>>>> + pmd_t *pmd, bool freeze, struct folio *folio) >>>>>> +{ >>>>>> + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); >>>>>> + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); >>>>>> + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); >>>>>> + VM_BUG_ON(freeze && !folio); >>>>>> + >>>>>> + /* >>>>>> + * When the caller requests to set up a migration entry, we >>>>>> + * require a folio to check the PMD against. Otherwise, there >>>>>> + * is a risk of replacing the wrong folio. >>>>>> + */ >>>>>> + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || >>>>>> + is_pmd_migration_entry(*pmd)) { >>>>>> + if (folio && folio != pmd_folio(*pmd)) >>>>>> + return; >>>>>> + __split_huge_pmd_locked(vma, pmd, address, freeze); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>>>>> unsigned long address, bool freeze, struct folio *folio) >>>>>> { >>>>>> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, >>>>>> (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); >>>>>> mmu_notifier_invalidate_range_start(&range); >>>>>> ptl = pmd_lock(vma->vm_mm, pmd); >>>>>> - >>>>>> - /* >>>>>> - * If caller asks to setup a migration entry, we need a folio to check >>>>>> - * pmd against. Otherwise we can end up replacing wrong folio. >>>>>> - */ >>>>>> - VM_BUG_ON(freeze && !folio); >>>>>> - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); >>>>>> - >>>>>> - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || >>>>>> - is_pmd_migration_entry(*pmd)) { >>>>>> - /* >>>>>> - * It's safe to call pmd_page when folio is set because it's >>>>>> - * guaranteed that pmd is present. >>>>>> - */ >>>>>> - if (folio && folio != pmd_folio(*pmd)) >>>>>> - goto out; >>>>>> - __split_huge_pmd_locked(vma, pmd, range.start, freeze); >>>>>> - } >>>>>> - >>>>>> -out: >>>>>> + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); >>>>>> spin_unlock(ptl); >>>>>> mmu_notifier_invalidate_range_end(&range); >>>>>> } >>>>>> diff --git a/mm/rmap.c b/mm/rmap.c >>>>>> index 7e2575d669a9..432601154583 100644 >>>>>> --- a/mm/rmap.c >>>>>> +++ b/mm/rmap.c >>>>>> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>>>>> if (flags & TTU_SYNC) >>>>>> pvmw.flags = PVMW_SYNC; >>>>>> >>>>>> - if (flags & TTU_SPLIT_HUGE_PMD) >>>>>> - split_huge_pmd_address(vma, address, false, folio); >>>>>> - >>>>>> /* >>>>>> * For THP, we have to assume the worse case ie pmd for invalidation. >>>>>> * For hugetlb, it could be much worse if we need to do pud >>>>>> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>>>>> range.end = vma_address_end(&pvmw); >>>>>> mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, >>>>>> address, range.end); >>>>>> + if (flags & TTU_SPLIT_HUGE_PMD) >>>>>> + align_huge_pmd_range(vma, &range.start, &range.end); >>>>>> if (folio_test_hugetlb(folio)) { >>>>>> /* >>>>>> * If sharing is possible, start and end will be adjusted >>>>>> @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>>>>> mmu_notifier_invalidate_range_start(&range); >>>>>> >>>>>> while (page_vma_mapped_walk(&pvmw)) { >>>>>> - /* Unexpected PMD-mapped THP? */ >>>>>> - VM_BUG_ON_FOLIO(!pvmw.pte, folio); >>>>>> - >>>>>> /* >>>>>> * If the folio is in an mlock()d vma, we must not swap it out. >>>>>> */ >>>>>> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, >>>>>> goto walk_done_err; >>>>>> } >>>>>> >>>>>> + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { >>>>>> + /* >>>>>> + * We temporarily have to drop the PTL and start once >>>>>> + * again from that now-PTE-mapped page table. >>>>>> + */ >>>>>> + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, >>>>>> + folio); >>>>> >>>>> Just in case you might miss here, since you will no longer align >>>>> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE >>>>> in split_huge_pmd_locked(), you will need to align the input address now. >>>> >>>> Thanks for bringing that up! >>>> >>>> I do miss the alignment here when I decide to no longer align range.start >>>> in another email - thanks! >>>> >>> No problem. >>> >>>> Zi, could I move the alignment here? >>>> IIUC, we will not encounter a partially mapped THP here, and range.start >>>> and range.end should also not beyond the VMA limits. >>>> >>>> align_huge_pmd_range(vma, &range.start, &range.end); >>>> split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, >>>> folio); >>> >>> I think you can just do >>> >>> split_huge_pmd_locked(vma, ALIGN(range.start, HPAGE_PMD_SIZE), pvmw.pmd, false, folio); >>> >>> since range will later be used by mmu_notifier_invalidate_range_end() and changing >>> it might cause secondary TLB invalidation issues. >> >> Ok, makes sense to me - thanks! >> >> But we probably cannot use the HPAGE_PMD_SIZE here; it will cause >> broken compilation as seen in v3[1]. >> >> Perhaps we still need to add a new alignment function for the huge PMD? >> >> [1] https://lore.kernel.org/linux-mm/20240429202040.187453-1-sj@kernel.org/ >> >> Thanks again for the review! >> > Or you can adjust the alignment inside split_huge_pmd_locked(), since it can > be called other than __split_huge_pmd(). > > Hmm, I notice that split_huge_pmd_address() has mmu_notifier ops but your > split_huge_pmd_locked() does not include them, I wonder if that could cause > issues with mmu_notifier issues. Adding mmu_notifier people to confirm. Hi Alistair and Jason, Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole, so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw)) and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked() and does not include the mmu notifier ops inside split_huge_pmd_address(). I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start() before the while loop only has range of the original address and split huge pmd can affect the entire PMD address range and these two ranges might not be the same. -- Best Regards, Yan, Zi
On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote: > Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole, > so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw)) > and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked() > and does not include the mmu notifier ops inside split_huge_pmd_address(). > I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start() > before the while loop only has range of the original address and > split huge pmd can affect the entire PMD address range and these two ranges > might not be the same. That does not sound entirely good.. I suppose it depends on what split does, if the MM page table has the same translation before and after split then perhaps no invalidation is even necessary. Jason
On 8 May 2024, at 11:52, Jason Gunthorpe wrote: > On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote: > >> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole, >> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw)) >> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked() >> and does not include the mmu notifier ops inside split_huge_pmd_address(). >> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start() >> before the while loop only has range of the original address and >> split huge pmd can affect the entire PMD address range and these two ranges >> might not be the same. > > That does not sound entirely good.. > > I suppose it depends on what split does, if the MM page table has the > same translation before and after split then perhaps no invalidation > is even necessary. Before split, it is a PMD mapping to a PMD THP (order-9). After split, they are 512 PTEs mapping to the same THP. Unless the secondary TLB does not support PMD mapping and use 512 PTEs instead, it seems to be an issue from my understanding. In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1] and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping a subpage in the middle of a PMD THP, the former notifies about the PMD range change due to one PMD split into 512 PTEs and the latter only needs to notify about the invalidation of the unmapped PTE. I do not think the latter can replace the former, although a potential optimization can be that the latter can be removed as it is included in the range of the former. Regarding Lance's current code change, is it OK to change mmu_notifier range after mmu_notifier_invalidate_range_start()? Since in Lance's code, first mmu_notifier is gone and second, whose range is only about the single PTE, starts mmu_notifier_invalidate_range_start(), then the code finds that a PMD needs to be split into 512 PTEs. Would changing the range from PTE to PMD suffice? Or the code should call mmu_notifier_invalidate_range_start() with the new PMD range? I am not even sure two start with one end is legit. [1] https://elixir.bootlin.com/linux/v6.9-rc7/source/mm/huge_memory.c#L2658 [2] https://elixir.bootlin.com/linux/v6.9-rc7/source/mm/rmap.c#L1650 -- Best Regards, Yan, Zi
On Wed, May 08, 2024 at 12:22:08PM -0400, Zi Yan wrote: > On 8 May 2024, at 11:52, Jason Gunthorpe wrote: > > > On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote: > > > >> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole, > >> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw)) > >> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked() > >> and does not include the mmu notifier ops inside split_huge_pmd_address(). > >> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start() > >> before the while loop only has range of the original address and > >> split huge pmd can affect the entire PMD address range and these two ranges > >> might not be the same. > > > > That does not sound entirely good.. > > > > I suppose it depends on what split does, if the MM page table has the > > same translation before and after split then perhaps no invalidation > > is even necessary. > > Before split, it is a PMD mapping to a PMD THP (order-9). After split, > they are 512 PTEs mapping to the same THP. Unless the secondary TLB > does not support PMD mapping and use 512 PTEs instead, it seems to > be an issue from my understanding. I may not recall fully, but I don't think any secondaries are so sensitive to the PMD/PTE distinction.. At least the ones using hmm_range_fault() are not. When the PTE eventually comes up for invalidation then the secondary should wipe out any granual they may have captured. Though, perhaps KVM should be checked carefully. > In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1] > and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping > a subpage in the middle of a PMD THP, the former notifies about the PMD range > change due to one PMD split into 512 PTEs and the latter only needs to notify > about the invalidation of the unmapped PTE. I do not think the latter can > replace the former, although a potential optimization can be that the latter > can be removed as it is included in the range of the former. I think we probably don't need both, either size might be fine, but the larger size is definately fine.. > Regarding Lance's current code change, is it OK to change mmu_notifier range > after mmu_notifier_invalidate_range_start()? No, it cannot be changed during a start/stop transaction. Jason
Hey Zi and Jason, Thanks a lot for reaching out! On Thu, May 9, 2024 at 12:35 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, May 08, 2024 at 12:22:08PM -0400, Zi Yan wrote: > > On 8 May 2024, at 11:52, Jason Gunthorpe wrote: > > > > > On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote: > > > > > >> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole, > > >> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw)) > > >> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked() > > >> and does not include the mmu notifier ops inside split_huge_pmd_address(). IMO, It might be reasonable to exclude the mmu notifier ops in split_huge_pmd_locked(). IIUC, before acquiring the PTL, callers need to tear down the secondary mappings via mmu_notifier_invalidate_range_start() with the range aligned to HPAGE_PMD_SIZE. > > >> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start() > > >> before the while loop only has range of the original address and > > >> split huge pmd can affect the entire PMD address range and these two ranges > > >> might not be the same. As Baolin mentioned [1] before: "For a PMD mapped THP, I think the address is already THP size alignment returned from vma_address(&folio->page, vma)." Given this, perhaps we don't need to re-align the input address after starting the pagewalk? IMO, if any corner cases arise, we should catch them by using VM_WARN_ON_ONCE() in split_huge_pmd_locked(). Zi, what do you think? [1] https://lore.kernel.org/linux-mm/cc9fd23f-7d87-48a7-a737-acbea8e95fb7@linux.alibaba.com/ > > > > > > That does not sound entirely good.. > > > > > > I suppose it depends on what split does, if the MM page table has the > > > same translation before and after split then perhaps no invalidation > > > is even necessary. > > > > Before split, it is a PMD mapping to a PMD THP (order-9). After split, > > they are 512 PTEs mapping to the same THP. Unless the secondary TLB > > does not support PMD mapping and use 512 PTEs instead, it seems to > > be an issue from my understanding. > > I may not recall fully, but I don't think any secondaries are > so sensitive to the PMD/PTE distinction.. At least the ones using > hmm_range_fault() are not. > > When the PTE eventually comes up for invalidation then the secondary > should wipe out any granual they may have captured. > > Though, perhaps KVM should be checked carefully. > > > In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1] > > and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping > > a subpage in the middle of a PMD THP, the former notifies about the PMD range > > change due to one PMD split into 512 PTEs and the latter only needs to notify > > about the invalidation of the unmapped PTE. I do not think the latter can > > replace the former, although a potential optimization can be that the latter > > can be removed as it is included in the range of the former. > > I think we probably don't need both, either size might be fine, but > the larger size is definately fine.. > > > Regarding Lance's current code change, is it OK to change mmu_notifier range > > after mmu_notifier_invalidate_range_start()? > > No, it cannot be changed during a start/stop transaction. I understood and will keep that in mind - thanks! Thanks again for clarifying! Lance > > Jason > >
Hey Jason, Thanks a lot for clarifying! On Thu, May 9, 2024 at 12:35 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, May 08, 2024 at 12:22:08PM -0400, Zi Yan wrote: > > On 8 May 2024, at 11:52, Jason Gunthorpe wrote: > > > > > On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote: > > > > > >> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole, > > >> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw)) > > >> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked() > > >> and does not include the mmu notifier ops inside split_huge_pmd_address(). > > >> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start() > > >> before the while loop only has range of the original address and > > >> split huge pmd can affect the entire PMD address range and these two ranges > > >> might not be the same. > > > > > > That does not sound entirely good.. > > > > > > I suppose it depends on what split does, if the MM page table has the > > > same translation before and after split then perhaps no invalidation > > > is even necessary. > > > > Before split, it is a PMD mapping to a PMD THP (order-9). After split, > > they are 512 PTEs mapping to the same THP. Unless the secondary TLB > > does not support PMD mapping and use 512 PTEs instead, it seems to > > be an issue from my understanding. > > I may not recall fully, but I don't think any secondaries are > so sensitive to the PMD/PTE distinction.. At least the ones using > hmm_range_fault() are not. > > When the PTE eventually comes up for invalidation then the secondary > should wipe out any granual they may have captured. > > Though, perhaps KVM should be checked carefully. Agreed. IIUC, the secondary mappings are teardown in mmu_notifier_invalidate_range_start() and allowed to be established again only mmu_notifier_invalidate_range_end(), then all modifications will be picked up by the secondary MMU. I just image that the secondary MMU like a TLB, where you only invalidate, and then it will be refilled later (after mmu_notifier_invalidate_range_end()). Thanks again for the lesson! Lance > > > In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1] > > and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping > > a subpage in the middle of a PMD THP, the former notifies about the PMD range > > change due to one PMD split into 512 PTEs and the latter only needs to notify > > about the invalidation of the unmapped PTE. I do not think the latter can > > replace the former, although a potential optimization can be that the latter > > can be removed as it is included in the range of the former. > > I think we probably don't need both, either size might be fine, but > the larger size is definately fine.. > > > Regarding Lance's current code change, is it OK to change mmu_notifier range > > after mmu_notifier_invalidate_range_start()? > > No, it cannot be changed during a start/stop transaction. > > Jason > >
On 9 May 2024, at 4:21, Lance Yang wrote: > Hey Zi and Jason, > > Thanks a lot for reaching out! > > On Thu, May 9, 2024 at 12:35 AM Jason Gunthorpe <jgg@nvidia.com> wrote: >> >> On Wed, May 08, 2024 at 12:22:08PM -0400, Zi Yan wrote: >>> On 8 May 2024, at 11:52, Jason Gunthorpe wrote: >>> >>>> On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote: >>>> >>>>> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole, >>>>> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw)) >>>>> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked() >>>>> and does not include the mmu notifier ops inside split_huge_pmd_address(). > > IMO, It might be reasonable to exclude the mmu notifier ops in > split_huge_pmd_locked(). IIUC, before acquiring the PTL, callers need to tear > down the secondary mappings via mmu_notifier_invalidate_range_start() with > the range aligned to HPAGE_PMD_SIZE. > >>>>> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start() >>>>> before the while loop only has range of the original address and >>>>> split huge pmd can affect the entire PMD address range and these two ranges >>>>> might not be the same. > > As Baolin mentioned [1] before: > "For a PMD mapped THP, I think the address is already THP size alignment > returned from vma_address(&folio->page, vma)." > > Given this, perhaps we don't need to re-align the input address after > starting the pagewalk? IMO, if any corner cases arise, we should catch them > by using VM_WARN_ON_ONCE() in split_huge_pmd_locked(). > > Zi, what do you think? Yes, I agree. Thanks for sorting this out. > > [1] https://lore.kernel.org/linux-mm/cc9fd23f-7d87-48a7-a737-acbea8e95fb7@linux.alibaba.com/ > >>>> >>>> That does not sound entirely good.. >>>> >>>> I suppose it depends on what split does, if the MM page table has the >>>> same translation before and after split then perhaps no invalidation >>>> is even necessary. >>> >>> Before split, it is a PMD mapping to a PMD THP (order-9). After split, >>> they are 512 PTEs mapping to the same THP. Unless the secondary TLB >>> does not support PMD mapping and use 512 PTEs instead, it seems to >>> be an issue from my understanding. >> >> I may not recall fully, but I don't think any secondaries are >> so sensitive to the PMD/PTE distinction.. At least the ones using >> hmm_range_fault() are not. >> >> When the PTE eventually comes up for invalidation then the secondary >> should wipe out any granual they may have captured. >> >> Though, perhaps KVM should be checked carefully. >> >>> In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1] >>> and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping >>> a subpage in the middle of a PMD THP, the former notifies about the PMD range >>> change due to one PMD split into 512 PTEs and the latter only needs to notify >>> about the invalidation of the unmapped PTE. I do not think the latter can >>> replace the former, although a potential optimization can be that the latter >>> can be removed as it is included in the range of the former. >> >> I think we probably don't need both, either size might be fine, but >> the larger size is definately fine.. >> >>> Regarding Lance's current code change, is it OK to change mmu_notifier range >>> after mmu_notifier_invalidate_range_start()? >> >> No, it cannot be changed during a start/stop transaction. > > I understood and will keep that in mind - thanks! > > Thanks again for clarifying! > Lance > >> >> Jason >> >> -- Best Regards, Yan, Zi
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h index c8d3ec116e29..38c4b5537715 100644 --- a/include/linux/huge_mm.h +++ b/include/linux/huge_mm.h @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void) return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION); } +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, + pmd_t *pmd, bool freeze, struct folio *folio); + +static inline void align_huge_pmd_range(struct vm_area_struct *vma, + unsigned long *start, + unsigned long *end) +{ + *start = ALIGN(*start, HPAGE_PMD_SIZE); + *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE); + + VM_WARN_ON_ONCE(vma->vm_start > *start); + VM_WARN_ON_ONCE(vma->vm_end < *end); +} + #else /* CONFIG_TRANSPARENT_HUGEPAGE */ static inline bool folio_test_pmd_mappable(struct folio *folio) @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, unsigned long address, bool freeze, struct folio *folio) {} static inline void split_huge_pmd_address(struct vm_area_struct *vma, unsigned long address, bool freeze, struct folio *folio) {} +static inline void split_huge_pmd_locked(struct vm_area_struct *vma, + unsigned long address, pmd_t *pmd, + bool freeze, struct folio *folio) {} +static inline void align_huge_pmd_range(struct vm_area_struct *vma, + unsigned long *start, + unsigned long *end) {} #define split_huge_pud(__vma, __pmd, __address) \ do { } while (0) diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 8261b5669397..145505a1dd05 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd, pmd_populate(mm, pmd, pgtable); } +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address, + pmd_t *pmd, bool freeze, struct folio *folio) +{ + VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio)); + VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE)); + VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); + VM_BUG_ON(freeze && !folio); + + /* + * When the caller requests to set up a migration entry, we + * require a folio to check the PMD against. Otherwise, there + * is a risk of replacing the wrong folio. + */ + if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || + is_pmd_migration_entry(*pmd)) { + if (folio && folio != pmd_folio(*pmd)) + return; + __split_huge_pmd_locked(vma, pmd, address, freeze); + } +} + void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, unsigned long address, bool freeze, struct folio *folio) { @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd, (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE); mmu_notifier_invalidate_range_start(&range); ptl = pmd_lock(vma->vm_mm, pmd); - - /* - * If caller asks to setup a migration entry, we need a folio to check - * pmd against. Otherwise we can end up replacing wrong folio. - */ - VM_BUG_ON(freeze && !folio); - VM_WARN_ON_ONCE(folio && !folio_test_locked(folio)); - - if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || - is_pmd_migration_entry(*pmd)) { - /* - * It's safe to call pmd_page when folio is set because it's - * guaranteed that pmd is present. - */ - if (folio && folio != pmd_folio(*pmd)) - goto out; - __split_huge_pmd_locked(vma, pmd, range.start, freeze); - } - -out: + split_huge_pmd_locked(vma, range.start, pmd, freeze, folio); spin_unlock(ptl); mmu_notifier_invalidate_range_end(&range); } diff --git a/mm/rmap.c b/mm/rmap.c index 7e2575d669a9..432601154583 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, if (flags & TTU_SYNC) pvmw.flags = PVMW_SYNC; - if (flags & TTU_SPLIT_HUGE_PMD) - split_huge_pmd_address(vma, address, false, folio); - /* * For THP, we have to assume the worse case ie pmd for invalidation. * For hugetlb, it could be much worse if we need to do pud @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, range.end = vma_address_end(&pvmw); mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm, address, range.end); + if (flags & TTU_SPLIT_HUGE_PMD) + align_huge_pmd_range(vma, &range.start, &range.end); if (folio_test_hugetlb(folio)) { /* * If sharing is possible, start and end will be adjusted @@ -1664,9 +1663,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, mmu_notifier_invalidate_range_start(&range); while (page_vma_mapped_walk(&pvmw)) { - /* Unexpected PMD-mapped THP? */ - VM_BUG_ON_FOLIO(!pvmw.pte, folio); - /* * If the folio is in an mlock()d vma, we must not swap it out. */ @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, goto walk_done_err; } + if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) { + /* + * We temporarily have to drop the PTL and start once + * again from that now-PTE-mapped page table. + */ + split_huge_pmd_locked(vma, range.start, pvmw.pmd, false, + folio); + pvmw.pmd = NULL; + spin_unlock(pvmw.ptl); + flags &= ~TTU_SPLIT_HUGE_PMD; + continue; + } + + /* Unexpected PMD-mapped THP? */ + VM_BUG_ON_FOLIO(!pvmw.pte, folio); + pfn = pte_pfn(ptep_get(pvmw.pte)); subpage = folio_page(folio, pfn - folio_pfn(folio)); address = pvmw.address;
In preparation for supporting try_to_unmap_one() to unmap PMD-mapped folios, start the pagewalk first, then call split_huge_pmd_address() to split the folio. Suggested-by: David Hildenbrand <david@redhat.com> Signed-off-by: Lance Yang <ioworker0@gmail.com> --- include/linux/huge_mm.h | 20 ++++++++++++++++++++ mm/huge_memory.c | 42 +++++++++++++++++++++-------------------- mm/rmap.c | 24 +++++++++++++++++------ 3 files changed, 60 insertions(+), 26 deletions(-)