Re: [PATCH v5 6/6] mm: madvise: Avoid split during MADV_PAGEOUT and MADV_COLD
From: Barry Song
Date: Tue Apr 02 2024 - 06:18:10 EST
On Thu, Mar 28, 2024 at 3:46 AM Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
>
> Rework madvise_cold_or_pageout_pte_range() to avoid splitting any large
> folio that is fully and contiguously mapped in the pageout/cold vm
> range. This change means that large folios will be maintained all the
> way to swap storage. This both improves performance during swap-out, by
> eliding the cost of splitting the folio, and sets us up nicely for
> maintaining the large folio when it is swapped back in (to be covered in
> a separate series).
>
> Folios that are not fully mapped in the target range are still split,
> but note that behavior is changed so that if the split fails for any
> reason (folio locked, shared, etc) we now leave it as is and move to the
> next pte in the range and continue work on the proceeding folios.
> Previously any failure of this sort would cause the entire operation to
> give up and no folios mapped at higher addresses were paged out or made
> cold. Given large folios are becoming more common, this old behavior
> would have likely lead to wasted opportunities.
>
> While we are at it, change the code that clears young from the ptes to
> use ptep_test_and_clear_young(), via the new mkold_ptes() batch helper
> function. This is more efficent than get_and_clear/modify/set,
> especially for contpte mappings on arm64, where the old approach would
> require unfolding/refolding and the new approach can be done in place.
>
> Reviewed-by: Barry Song <v-songbaohua@xxxxxxxx>
> Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
> ---
Hi Ryan,
I'm not entirely certain if this issue is related to this patch, but
I've encountered
the KNIC twice while using the latest mm-unstable kernel. Each time I attempted
to debug it, the issue vanished. I'm posting here to see if you have
any ideas on it :-)
[ 50.444066]
[ 50.444495] =====================================
[ 50.444954] WARNING: bad unlock balance detected!
[ 50.445443] 6.9.0-rc2-00257-g2d9f63c285db #128 Not tainted
[ 50.446233] -------------------------------------
[ 50.446684] singlethread/102 is trying to release lock
(ptlock_ptr(ptdesc)) at:
[ 50.447635] [<ffffb03155fe302c>]
madvise_cold_or_pageout_pte_range+0x80c/0xea0
[ 50.449066] but there are no more locks to release!
[ 50.449535]
[ 50.449535] other info that might help us debug this:
[ 50.450140] 1 lock held by singlethread/102:
[ 50.450688] #0: ffff0000c001f208 (&mm->mmap_lock){++++}-{4:4}, at:
do_madvise.part.0+0x178/0x518
[ 50.452321]
[ 50.452321] stack backtrace:
[ 50.452959] CPU: 3 PID: 102 Comm: singlethread Not tainted
6.9.0-rc2-00257-g2d9f63c285db #128
[ 50.453812] Hardware name: linux,dummy-virt (DT)
[ 50.454373] Call trace:
[ 50.454755] dump_backtrace+0x9c/0x100
[ 50.455246] show_stack+0x20/0x38
[ 50.455667] dump_stack_lvl+0xec/0x150
[ 50.456111] dump_stack+0x18/0x28
[ 50.456533] print_unlock_imbalance_bug+0x130/0x148
[ 50.457014] lock_release+0x2e0/0x360
[ 50.457487] _raw_spin_unlock+0x2c/0x78
[ 50.457997] madvise_cold_or_pageout_pte_range+0x80c/0xea0
[ 50.458635] walk_pgd_range+0x388/0x7d8
[ 50.459168] __walk_page_range+0x1e0/0x1f0
[ 50.459682] walk_page_range+0x1f0/0x2c8
[ 50.460225] madvise_pageout+0xf8/0x280
[ 50.460711] madvise_vma_behavior+0x310/0x9b8
[ 50.461169] madvise_walk_vmas+0xc0/0x128
[ 50.461605] do_madvise.part.0+0xf8/0x518
[ 50.462041] __arm64_sys_madvise+0x68/0x88
[ 50.462529] invoke_syscall+0x50/0x128
[ 50.463001] el0_svc_common.constprop.0+0x48/0xf8
[ 50.463508] do_el0_svc+0x28/0x40
[ 50.464004] el0_svc+0x50/0x150
[ 50.464492] el0t_64_sync_handler+0x13c/0x158
[ 50.465021] el0t_64_sync+0x1a4/0x1a8
[ 50.466959] ------------[ cut here ]------------
[ 50.467451] WARNING: CPU: 3 PID: 102 at
kernel/rcu/tree_plugin.h:431 __rcu_read_unlock+0x74/0x218
[ 50.468160] Modules linked in:
[ 50.468803] CPU: 3 PID: 102 Comm: singlethread Not tainted
6.9.0-rc2-00257-g2d9f63c285db #128
[ 50.469658] Hardware name: linux,dummy-virt (DT)
[ 50.470293] pstate: a3400005 (NzCv daif +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
[ 50.470991] pc : __rcu_read_unlock+0x74/0x218
[ 50.471594] lr : madvise_cold_or_pageout_pte_range+0x828/0xea0
[ 50.472236] sp : ffff800080abb7e0
[ 50.472622] pmr_save: 000000e0
[ 50.473010] x29: ffff800080abb7e0 x28: 0000ffffa467a000 x27: fffffdffc3128c00
[ 50.474006] x26: 0010000000000001 x25: 000000000000001b x24: ffff0000c32d73d0
[ 50.474971] x23: 0060000104a3afc3 x22: ffff0000c2492840 x21: 0400000000000001
[ 50.475943] x20: ff77fffffffffbff x19: ffff0000c3230000 x18: ffffffffffffffff
[ 50.477286] x17: 672d37353230302d x16: 3263722d302e392e x15: ffff800100abb227
[ 50.478373] x14: 0000000000000001 x13: 38613178302f3461 x12: 3178302b636e7973
[ 50.479354] x11: fffffffffffe0000 x10: ffffb03159697d08 x9 : ffffb03155fe3048
[ 50.480265] x8 : 00000000ffffefff x7 : ffffb03159697d08 x6 : 0000000000000000
[ 50.481154] x5 : 0000000000000001 x4 : ffff800080abbfe0 x3 : 0000000000000000
[ 50.482035] x2 : ffff4fd055074000 x1 : 00000000ffffffff x0 : 000000003fffffff
[ 50.483163] Call trace:
[ 50.483599] __rcu_read_unlock+0x74/0x218
[ 50.484152] madvise_cold_or_pageout_pte_range+0x828/0xea0
[ 50.484780] walk_pgd_range+0x388/0x7d8
[ 50.485328] __walk_page_range+0x1e0/0x1f0
[ 50.485725] walk_page_range+0x1f0/0x2c8
[ 50.486117] madvise_pageout+0xf8/0x280
[ 50.486547] madvise_vma_behavior+0x310/0x9b8
[ 50.486975] madvise_walk_vmas+0xc0/0x128
[ 50.487403] do_madvise.part.0+0xf8/0x518
[ 50.487845] __arm64_sys_madvise+0x68/0x88
[ 50.488374] invoke_syscall+0x50/0x128
[ 50.488946] el0_svc_common.constprop.0+0x48/0xf8
[ 50.489732] do_el0_svc+0x28/0x40
[ 50.490210] el0_svc+0x50/0x150
[ 50.490674] el0t_64_sync_handler+0x13c/0x158
[ 50.491257] el0t_64_sync+0x1a4/0x1a8
[ 50.491793] irq event stamp: 3087
[ 50.492243] hardirqs last enabled at (3087): [<ffffb031570d89d8>]
_raw_spin_unlock_irq+0x38/0x90
[ 50.492917] hardirqs last disabled at (3086): [<ffffb031570d8acc>]
_raw_spin_lock_irq+0x9c/0xc0
[ 50.493742] softirqs last enabled at (2470): [<ffffb03155c10d94>]
__do_softirq+0x534/0x588
[ 50.494567] softirqs last disabled at (2461): [<ffffb03155c17238>]
____do_softirq+0x18/0x30
[ 50.495328] ---[ end trace 0000000000000000 ]---
[ 50.497110] BUG: sleeping function called from invalid context at
kernel/locking/rwsem.c:1578
[ 50.497544] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid:
102, name: singlethread
[ 50.497652] preempt_count: ffffffff, expected: 0
[ 50.497728] RCU nest depth: -1, expected: 0
[ 50.497851] INFO: lockdep is turned off.
[ 50.498023] CPU: 3 PID: 102 Comm: singlethread Tainted: G W
6.9.0-rc2-00257-g2d9f63c285db #128
[ 50.498166] Hardware name: linux,dummy-virt (DT)
[ 50.498221] Call trace:
[ 50.498260] dump_backtrace+0x9c/0x100
[ 50.498378] show_stack+0x20/0x38
[ 50.498487] dump_stack_lvl+0xec/0x150
[ 50.498574] dump_stack+0x18/0x28
[ 50.498659] __might_resched+0x158/0x278
[ 50.498741] __might_sleep+0x50/0xa0
[ 50.498849] down_write+0x30/0x1a8
[ 50.498950] split_huge_page_to_list_to_order+0x3c8/0x1130
[ 50.499052] madvise_cold_or_pageout_pte_range+0x84c/0xea0
[ 50.499138] walk_pgd_range+0x388/0x7d8
[ 50.499224] __walk_page_range+0x1e0/0x1f0
[ 50.499334] walk_page_range+0x1f0/0x2c8
[ 50.499458] madvise_pageout+0xf8/0x280
[ 50.499554] madvise_vma_behavior+0x310/0x9b8
[ 50.499657] madvise_walk_vmas+0xc0/0x128
[ 50.499739] do_madvise.part.0+0xf8/0x518
[ 50.499851] __arm64_sys_madvise+0x68/0x88
[ 50.499953] invoke_syscall+0x50/0x128
[ 50.500037] el0_svc_common.constprop.0+0x48/0xf8
[ 50.500121] do_el0_svc+0x28/0x40
[ 50.500203] el0_svc+0x50/0x150
[ 50.500322] el0t_64_sync_handler+0x13c/0x158
[ 50.500422] el0t_64_sync+0x1a4/0x1a8
[ 50.501378] BUG: scheduling while atomic: singlethread/102/0x00000000
[ 50.517641] INFO: lockdep is turned off.
[ 50.518206] Modules linked in:
[ 50.521135] CPU: 2 PID: 102 Comm: singlethread Tainted: G W
6.9.0-rc2-00257-g2d9f63c285db #128
[ 50.522026] Hardware name: linux,dummy-virt (DT)
[ 50.522623] Call trace:
[ 50.522993] dump_backtrace+0x9c/0x100
[ 50.523527] show_stack+0x20/0x38
[ 50.523950] dump_stack_lvl+0xec/0x150
[ 50.524405] dump_stack+0x18/0x28
[ 50.524849] __schedule_bug+0x80/0xe0
[ 50.525309] __schedule+0xb1c/0xc00
[ 50.525750] schedule+0x58/0x170
[ 50.526227] schedule_preempt_disabled+0x2c/0x50
[ 50.526762] rwsem_down_write_slowpath+0x1ac/0x718
[ 50.527342] down_write+0xf8/0x1a8
[ 50.527857] split_huge_page_to_list_to_order+0x3c8/0x1130
[ 50.528437] madvise_cold_or_pageout_pte_range+0x84c/0xea0
[ 50.529012] walk_pgd_range+0x388/0x7d8
[ 50.529442] __walk_page_range+0x1e0/0x1f0
[ 50.529896] walk_page_range+0x1f0/0x2c8
[ 50.530342] madvise_pageout+0xf8/0x280
[ 50.530878] madvise_vma_behavior+0x310/0x9b8
[ 50.531395] madvise_walk_vmas+0xc0/0x128
[ 50.531849] do_madvise.part.0+0xf8/0x518
[ 50.532330] __arm64_sys_madvise+0x68/0x88
[ 50.532829] invoke_syscall+0x50/0x128
[ 50.533374] el0_svc_common.constprop.0+0x48/0xf8
[ 50.533992] do_el0_svc+0x28/0x40
[ 50.534498] el0_svc+0x50/0x150
[ 50.535029] el0t_64_sync_handler+0x13c/0x158
[ 50.535588] el0t_64_sync+0x1a4/0x1a8
> include/linux/pgtable.h | 30 ++++++++++++++
> mm/internal.h | 12 +++++-
> mm/madvise.c | 88 ++++++++++++++++++++++++-----------------
> mm/memory.c | 4 +-
> 4 files changed, 93 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 8185939df1e8..391f56a1b188 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -361,6 +361,36 @@ static inline int ptep_test_and_clear_young(struct vm_area_struct *vma,
> }
> #endif
>
> +#ifndef mkold_ptes
> +/**
> + * mkold_ptes - Mark PTEs that map consecutive pages of the same folio as old.
> + * @vma: VMA the pages are mapped into.
> + * @addr: Address the first page is mapped at.
> + * @ptep: Page table pointer for the first entry.
> + * @nr: Number of entries to mark old.
> + *
> + * May be overridden by the architecture; otherwise, implemented as a simple
> + * loop over ptep_test_and_clear_young().
> + *
> + * Note that PTE bits in the PTE range besides the PFN can differ. For example,
> + * some PTEs might be write-protected.
> + *
> + * Context: The caller holds the page table lock. The PTEs map consecutive
> + * pages that belong to the same folio. The PTEs are all in the same PMD.
> + */
> +static inline void mkold_ptes(struct vm_area_struct *vma, unsigned long addr,
> + pte_t *ptep, unsigned int nr)
> +{
> + for (;;) {
> + ptep_test_and_clear_young(vma, addr, ptep);
> + if (--nr == 0)
> + break;
> + ptep++;
> + addr += PAGE_SIZE;
> + }
> +}
> +#endif
> +
> #ifndef __HAVE_ARCH_PMDP_TEST_AND_CLEAR_YOUNG
> #if defined(CONFIG_TRANSPARENT_HUGEPAGE) || defined(CONFIG_ARCH_HAS_NONLEAF_PMD_YOUNG)
> static inline int pmdp_test_and_clear_young(struct vm_area_struct *vma,
> diff --git a/mm/internal.h b/mm/internal.h
> index eadb79c3a357..efee8e4cd2af 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -130,6 +130,8 @@ static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
> * @flags: Flags to modify the PTE batch semantics.
> * @any_writable: Optional pointer to indicate whether any entry except the
> * first one is writable.
> + * @any_young: Optional pointer to indicate whether any entry except the
> + * first one is young.
> *
> * Detect a PTE batch: consecutive (present) PTEs that map consecutive
> * pages of the same large folio.
> @@ -145,16 +147,18 @@ static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
> */
> static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
> pte_t *start_ptep, pte_t pte, int max_nr, fpb_t flags,
> - bool *any_writable)
> + bool *any_writable, bool *any_young)
> {
> unsigned long folio_end_pfn = folio_pfn(folio) + folio_nr_pages(folio);
> const pte_t *end_ptep = start_ptep + max_nr;
> pte_t expected_pte, *ptep;
> - bool writable;
> + bool writable, young;
> int nr;
>
> if (any_writable)
> *any_writable = false;
> + if (any_young)
> + *any_young = false;
>
> VM_WARN_ON_FOLIO(!pte_present(pte), folio);
> VM_WARN_ON_FOLIO(!folio_test_large(folio) || max_nr < 1, folio);
> @@ -168,6 +172,8 @@ static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
> pte = ptep_get(ptep);
> if (any_writable)
> writable = !!pte_write(pte);
> + if (any_young)
> + young = !!pte_young(pte);
> pte = __pte_batch_clear_ignored(pte, flags);
>
> if (!pte_same(pte, expected_pte))
> @@ -183,6 +189,8 @@ static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
>
> if (any_writable)
> *any_writable |= writable;
> + if (any_young)
> + *any_young |= young;
>
> nr = pte_batch_hint(ptep, pte);
> expected_pte = pte_advance_pfn(expected_pte, nr);
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 070bedb4996e..bd00b83e7c50 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -336,6 +336,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> LIST_HEAD(folio_list);
> bool pageout_anon_only_filter;
> unsigned int batch_count = 0;
> + int nr;
>
> if (fatal_signal_pending(current))
> return -EINTR;
> @@ -423,7 +424,8 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> return 0;
> flush_tlb_batched_pending(mm);
> arch_enter_lazy_mmu_mode();
> - for (; addr < end; pte++, addr += PAGE_SIZE) {
> + for (; addr < end; pte += nr, addr += nr * PAGE_SIZE) {
> + nr = 1;
> ptent = ptep_get(pte);
>
> if (++batch_count == SWAP_CLUSTER_MAX) {
> @@ -447,55 +449,67 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> continue;
>
> /*
> - * Creating a THP page is expensive so split it only if we
> - * are sure it's worth. Split it if we are only owner.
> + * If we encounter a large folio, only split it if it is not
> + * fully mapped within the range we are operating on. Otherwise
> + * leave it as is so that it can be swapped out whole. If we
> + * fail to split a folio, leave it in place and advance to the
> + * next pte in the range.
> */
> if (folio_test_large(folio)) {
> - int err;
> -
> - if (folio_likely_mapped_shared(folio))
> - break;
> - if (pageout_anon_only_filter && !folio_test_anon(folio))
> - break;
> - if (!folio_trylock(folio))
> - break;
> - folio_get(folio);
> - arch_leave_lazy_mmu_mode();
> - pte_unmap_unlock(start_pte, ptl);
> - start_pte = NULL;
> - err = split_folio(folio);
> - folio_unlock(folio);
> - folio_put(folio);
> - if (err)
> - break;
> - start_pte = pte =
> - pte_offset_map_lock(mm, pmd, addr, &ptl);
> - if (!start_pte)
> - break;
> - arch_enter_lazy_mmu_mode();
> - pte--;
> - addr -= PAGE_SIZE;
> - continue;
> + const fpb_t fpb_flags = FPB_IGNORE_DIRTY |
> + FPB_IGNORE_SOFT_DIRTY;
> + int max_nr = (end - addr) / PAGE_SIZE;
> + bool any_young;
> +
> + nr = folio_pte_batch(folio, addr, pte, ptent, max_nr,
> + fpb_flags, NULL, &any_young);
> + if (any_young)
> + ptent = pte_mkyoung(ptent);
> +
> + if (nr < folio_nr_pages(folio)) {
> + int err;
> +
> + if (folio_likely_mapped_shared(folio))
> + continue;
> + if (pageout_anon_only_filter && !folio_test_anon(folio))
> + continue;
> + if (!folio_trylock(folio))
> + continue;
> + folio_get(folio);
> + arch_leave_lazy_mmu_mode();
> + pte_unmap_unlock(start_pte, ptl);
> + start_pte = NULL;
> + err = split_folio(folio);
> + folio_unlock(folio);
> + folio_put(folio);
> + if (err)
> + continue;
> + start_pte = pte =
> + pte_offset_map_lock(mm, pmd, addr, &ptl);
> + if (!start_pte)
> + break;
> + arch_enter_lazy_mmu_mode();
> + nr = 0;
> + continue;
> + }
> }
>
> /*
> * Do not interfere with other mappings of this folio and
> - * non-LRU folio.
> + * non-LRU folio. If we have a large folio at this point, we
> + * know it is fully mapped so if its mapcount is the same as its
> + * number of pages, it must be exclusive.
> */
> - if (!folio_test_lru(folio) || folio_mapcount(folio) != 1)
> + if (!folio_test_lru(folio) ||
> + folio_mapcount(folio) != folio_nr_pages(folio))
> continue;
>
> if (pageout_anon_only_filter && !folio_test_anon(folio))
> continue;
>
> - VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
> -
> if (!pageout && pte_young(ptent)) {
> - ptent = ptep_get_and_clear_full(mm, addr, pte,
> - tlb->fullmm);
> - ptent = pte_mkold(ptent);
> - set_pte_at(mm, addr, pte, ptent);
> - tlb_remove_tlb_entry(tlb, pte, addr);
> + mkold_ptes(vma, addr, pte, nr);
> + tlb_remove_tlb_entries(tlb, pte, nr, addr);
> }
>
> /*
> diff --git a/mm/memory.c b/mm/memory.c
> index 9d844582ba38..b5b48f4cf2af 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -989,7 +989,7 @@ copy_present_ptes(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
> flags |= FPB_IGNORE_SOFT_DIRTY;
>
> nr = folio_pte_batch(folio, addr, src_pte, pte, max_nr, flags,
> - &any_writable);
> + &any_writable, NULL);
> folio_ref_add(folio, nr);
> if (folio_test_anon(folio)) {
> if (unlikely(folio_try_dup_anon_rmap_ptes(folio, page,
> @@ -1553,7 +1553,7 @@ static inline int zap_present_ptes(struct mmu_gather *tlb,
> */
> if (unlikely(folio_test_large(folio) && max_nr != 1)) {
> nr = folio_pte_batch(folio, addr, pte, ptent, max_nr, fpb_flags,
> - NULL);
> + NULL, NULL);
>
> zap_present_folio_ptes(tlb, vma, folio, page, pte, ptent, nr,
> addr, details, rss, force_flush,
> --
> 2.25.1
>