by Lance Yang

[permalink] [raw]

Subject: Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

On Tue, May 7, 2024 at 5:33 PM Baolin Wang
<[email protected]> wrote:
>
>
>
> On 2024/5/7 16:26, Lance Yang wrote:
> > On Tue, May 7, 2024 at 2:32 PM Lance Yang <[email protected]> wrote:
> >>
> >> Hey Baolin,
> >>
> >> Thanks a lot for taking time to review!
> >>
> >> On Tue, May 7, 2024 at 12:01 PM Baolin Wang
> >> <[email protected]> wrote:
> >>>
> >>>
> >>>
> >>> On 2024/5/1 12:27, Lance Yang wrote:
> >>>> When the user no longer requires the pages, they would use
> >>>> madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
> >>>> typically would not re-write to that memory again.
> >>>>
> >>>> During memory reclaim, if we detect that the large folio and its PMD are
> >>>> both still marked as clean and there are no unexpected references
> >>>> (such as GUP), so we can just discard the memory lazily, improving the
> >>>> efficiency of memory reclamation in this case. On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> >>>> mem_cgroup_force_empty() results in the following runtimes in seconds
> >>>> (shorter is better):
> >>>>
> >>>> --------------------------------------------
> >>>> | Old | New | Change |
> >>>> --------------------------------------------
> >>>> | 0.683426 | 0.049197 | -92.80% |
> >>>> --------------------------------------------
> >>>>
> >>>> Suggested-by: Zi Yan <[email protected]>
> >>>> Suggested-by: David Hildenbrand <[email protected]>
> >>>> Signed-off-by: Lance Yang <[email protected]>
> >>>> ---
> >>>> include/linux/huge_mm.h | 9 +++++
> >>>> mm/huge_memory.c | 73 +++++++++++++++++++++++++++++++++++++++++
> >>>> mm/rmap.c | 3 ++
> >>>> 3 files changed, 85 insertions(+)
> >>>>
> >>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >>>> index 38c4b5537715..017cee864080 100644
> >>>> --- a/include/linux/huge_mm.h
> >>>> +++ b/include/linux/huge_mm.h
> >>>> @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
> >>>>
> >>>> void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >>>> pmd_t *pmd, bool freeze, struct folio *folio);
> >>>> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> >>>> + pmd_t *pmdp, struct folio *folio);
> >>>>
> >>>> static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >>>> unsigned long *start,
> >>>> @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >>>> unsigned long *start,
> >>>> unsigned long *end) {}
> >>>>
> >>>> +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
> >>>> + unsigned long addr, pmd_t *pmdp,
> >>>> + struct folio *folio)
> >>>> +{
> >>>> + return false;
> >>>> +}
> >>>> +
> >>>> #define split_huge_pud(__vma, __pmd, __address) \
> >>>> do { } while (0)
> >>>>
> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>>> index 145505a1dd05..90fdef847a88 100644
> >>>> --- a/mm/huge_memory.c
> >>>> +++ b/mm/huge_memory.c
> >>>> @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
> >>>> try_to_unmap_flush();
> >>>> }
> >>>>
> >>>> +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
> >>>> + unsigned long addr, pmd_t *pmdp,
> >>>> + struct folio *folio)
> >>>> +{
> >>>> + struct mm_struct *mm = vma->vm_mm;
> >>>> + int ref_count, map_count;
> >>>> + pmd_t orig_pmd = *pmdp;
> >>>> + struct mmu_gather tlb;
> >>>> + struct page *page;
> >>>> +
> >>>> + if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
> >>>> + return false;
> >>>> + if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> >>>> + return false;
> >>>> +
> >>>> + page = pmd_page(orig_pmd);
> >>>> + if (unlikely(page_folio(page) != folio))
> >>>> + return false;
> >>>> +
> >>>> + tlb_gather_mmu(&tlb, mm);
> >>>> + orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
> >>>> + tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
> >>>> +
> >>>> + /*
> >>>> + * Syncing against concurrent GUP-fast:
> >>>> + * - clear PMD; barrier; read refcount
> >>>> + * - inc refcount; barrier; read PMD
> >>>> + */
> >>>> + smp_mb();
> >>>> +
> >>>> + ref_count = folio_ref_count(folio);
> >>>> + map_count = folio_mapcount(folio);
> >>>> +
> >>>> + /*
> >>>> + * Order reads for folio refcount and dirty flag
> >>>> + * (see comments in __remove_mapping()).
> >>>> + */
> >>>> + smp_rmb();
> >>>> +
> >>>> + /*
> >>>> + * If the PMD or folio is redirtied at this point, or if there are
> >>>> + * unexpected references, we will give up to discard this folio
> >>>> + * and remap it.
> >>>> + *
> >>>> + * The only folio refs must be one from isolation plus the rmap(s).
> >>>> + */
> >>>> + if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
> >>>> + pmd_dirty(orig_pmd)) {
> >>>> + set_pmd_at(mm, addr, pmdp, orig_pmd);
> >>>> + return false;
> >>>> + }
> >>>> +
> >>>> + folio_remove_rmap_pmd(folio, page, vma);
> >>>> + zap_deposited_table(mm, pmdp);
> >>>> + add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> >>>> + folio_put(folio);
> >>>
> >>> IIUC, you missed handling mlock vma, see mlock_drain_local() in
> >>> try_to_unmap_one().
> >>
> >> Good spot!
> >>
> >> I suddenly realized that I overlooked another thing: If we detect that a
> >> PMD-mapped THP is within the range of the VM_LOCKED VMA, we
> >> should check whether the TTU_IGNORE_MLOCK flag is set in
> >> try_to_unmap_one(). If the flag is set, we will remove the PMD mapping
> >> from the folio. Otherwise, the folio should be mlocked, which avoids
> >> splitting the folio and then mlocking each page again.
> >
> > My previous response above is flawed - sorry :(
> >
> > If we detect that a PMD-mapped THP is within the range of the
> > VM_LOCKED VMA.
> >
> > 1) If the TTU_IGNORE_MLOCK flag is set, we will try to remove the
> > PMD mapping from the folio, as this series has done.
>
> Right.
>
> > 2) If the flag is not set, the large folio should be mlocked to prevent it
> > from being picked during memory reclaim? Currently, we just leave it
>
> Yes. From commit 1acbc3f93614 ("mm: handle large folio when large folio
> in VM_LOCKED VMA range"), large folios of the mlocked VMA will be
> handled during page reclaim phase.
>
> > as is and do not to mlock it, IIUC.
>
> Original code already handle the mlock case after the PMD-mapped THP is
> split in try_to_unmap_one():

Yep. But this series doesn't do the TTU_SPLIT_HUGE_PMD immediately.

> /*
> * If the folio is in an mlock()d vma, we must not swap
> it out.
> */
> if (!(flags & TTU_IGNORE_MLOCK) &&
> (vma->vm_flags & VM_LOCKED)) {
> /* Restore the mlock which got missed */

IIUC, we could detect a PMD-mapped THP here. So, I'm not sure if we
need to mlock it to prevent it from being picked again during memory
reclaim. The change is as follows:

diff --git a/mm/rmap.c b/mm/rmap.c
index ed7f82036986..2a9d037ab23c 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1673,7 +1673,8 @@ static bool try_to_unmap_one(struct folio
*folio, struct vm_area_struct *vma,
if (!(flags & TTU_IGNORE_MLOCK) &&
(vma->vm_flags & VM_LOCKED)) {
/* Restore the mlock which got missed */
- if (!folio_test_large(folio))
+ if (!folio_test_large(folio) ||
+ (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)))
mlock_vma_folio(folio, vma);
goto walk_done_err;
}

Thanks,
Lance

> if (!folio_test_large(folio))
> mlock_vma_folio(folio, vma);
> page_vma_mapped_walk_done(&pvmw);
> ret = false;
> break;
> }

2024-05-07 14:52:22

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 <[email protected]>
> Signed-off-by: Lance Yang <[email protected]>
> ---
> 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

Attachments:

signature.asc (871.00 B)
OpenPGP digital signature

2024-05-07 16:25:05

On 8 May 2024, at 1:43, Lance Yang wrote:

> On Tue, May 7, 2024 at 11:26 PM Zi Yan <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Lance Yang <[email protected]>
>>> ---
>>> 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

Attachments:

signature.asc (871.00 B)
OpenPGP digital signature

2024-05-08 14:40:43

by Lance Yang

[permalink] [raw]

Subject: Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

On Wed, May 8, 2024 at 10:07 PM Zi Yan <[email protected]> wrote:
>
> On 8 May 2024, at 1:43, Lance Yang wrote:
>
> > On Tue, May 7, 2024 at 11:26 PM Zi Yan <[email protected]> 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 <[email protected]>
> >>> Signed-off-by: Lance Yang <[email protected]>
> >>> ---
> >>> 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/[email protected]/

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

2024-05-08 15:03:53

by Zi Yan

[permalink] [raw]

Subject: Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

On 8 May 2024, at 10:35, Lance Yang wrote:

> On Wed, May 8, 2024 at 10:07 PM Zi Yan <[email protected]> wrote:
>>
>> On 8 May 2024, at 1:43, Lance Yang wrote:
>>
>>> On Tue, May 7, 2024 at 11:26 PM Zi Yan <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: Lance Yang <[email protected]>
>>>>> ---
>>>>> 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/[email protected]/
>
> 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

Attachments:

signature.asc (871.00 B)
OpenPGP digital signature

2024-05-08 15:10:00

[permalink] [raw]

Subject: Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

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 <[email protected]> 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/[email protected]/
>
>>>>
>>>> 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

Attachments:

signature.asc (871.00 B)
OpenPGP digital signature