From: "Kiryl Shutsemau (Meta)" <[email protected]> collapse_single_pmd() did both halves of a collapse behind one call, and dropped mmap_lock somewhere in the middle. Which of its paths dropped it was not something a caller could see, so it was handed back a bool and had to keep track.
Both callers did that badly: khugepaged broke out of its VMA walk after every table, collapsed or not, and MADV_COLLAPSE carried lock state through its loop and re-took the lock only to hand it back. Split it in two, with the lock as the boundary: - collapse_scan_pmd() judges one table and returns with mmap_lock still held. It only reads, and almost every table it is offered has nothing in it, so a caller walks a whole VMA under the one lock it took to get there. - collapse_run_pmd() is called without the lock, which the caller gives up first, and takes it again per round. What it does is slow enough that a writer would otherwise wait behind all of it. Whether there is anything to run is the scan's return value, so no caller has to ask about the lock. The file side is what makes this more than a rename. A file collapse works on the page cache and never sees a VMA, but the file and the offset have to come from one: the scan takes them while it still has the VMA, in cc->scan_file and cc->scan_pgoff, and the run is what gives the reference back. A scan that found file work therefore has to be run, and collapse_control_release() warns and drops the reference rather than rest on callers getting that right. The orders a VMA allows now come in as an argument. khugepaged's walk already computes that mask once per VMA, where the old shape recomputed it twice for every table. MADV_COLLAPSE keeps its own copy only while it holds the VMA, and clears it beside vma = NULL: after the lock is given up, the next lookup may return a different VMA. khugepaged now stays in its VMA walk across every table it refuses, and gives the lock up only for a table it is going to collapse. MADV_COLLAPSE does the same. It gives up the lock the VMA walk left held, so lru_add_drain_all() does not wait on every CPU under it, then takes it once and walks its whole range. It drops that lock only around a collapse, and looks the VMA up again only then, since nothing else can have moved it. Holding on across refusals is not new for anonymous memory -- the old entry already returned with the lock held when a table had nothing in it -- but it was never true of file ranges, and never something a caller could rely on. Assisted-by: Claude-Code:claude-opus-5 Signed-off-by: Kiryl Shutsemau (Meta) <[email protected]> --- mm/collapse.c | 167 +++++++++++++++++++++++++++++++----------- mm/collapse.h | 48 ++++++++++++- mm/khugepaged.c | 188 +++++++++++++++++++++++------------------------- mm/mremap.c | 2 +- 4 files changed, 262 insertions(+), 143 deletions(-) diff --git a/mm/collapse.c b/mm/collapse.c index 21bfbc038044..f0d80204c2bd 100644 --- a/mm/collapse.c +++ b/mm/collapse.c @@ -401,6 +401,10 @@ static unsigned int candidate_offset(const struct collapse_candidate *cand, void collapse_control_release(struct collapse_control *cc) { + /* Only a scan that was never run leaves this behind */ + if (WARN_ON_ONCE(cc->scan_file)) + fput(cc->scan_file); + kfree(cc->candidates); kfree(cc->saved_ptes); kfree(cc->retries); @@ -413,6 +417,10 @@ int collapse_control_init(struct collapse_control *cc) { cc->nr_candidates = 0; cc->nr_retries = 0; + cc->select_orders = 0; + cc->scan_refusal = SCAN_FAIL; + cc->scan_file = NULL; + cc->scan_pgoff = 0; cc->candidates = kmalloc_objs(*cc->candidates, COLLAPSE_MAX_CANDIDATES); cc->saved_ptes = kmalloc_objs(*cc->saved_ptes, COLLAPSE_SAVED_PTES); cc->retries = kmalloc_objs(*cc->retries, COLLAPSE_RETRY_STORE_SIZE); @@ -2116,7 +2124,8 @@ static void collapse_anon_scan_init(struct collapse_control *cc) */ static enum scan_result collapse_scan_anon_pmd(struct vm_area_struct *vma, unsigned long start, unsigned long end, - struct collapse_control *cc) + struct collapse_control *cc, + unsigned long vma_orders) { const unsigned long pmd_addr = start & HPAGE_PMD_MASK; struct mm_struct *mm = vma->vm_mm; @@ -2135,12 +2144,7 @@ static enum scan_result collapse_scan_anon_pmd(struct vm_area_struct *vma, /* Cleared only once a table has turned out to be there */ collapse_anon_scan_init(cc); - cc->select_orders = collapse_possible_orders(vma, vma->vm_flags, - cc->policy.tva_type); - if (!cc->select_orders) { - cc->scan_refusal = SCAN_VMA_CHECK; - return cc->scan_refusal; - } + cc->select_orders = vma_orders; /* The scan narrows select_orders to whatever is left worth trying */ cc->scan_refusal = collapse_scan_table(vma, pmd, start, end, cc); @@ -2596,7 +2600,7 @@ static void count_collapse_event(unsigned int order, enum vm_event_item vm_event count_mthp_stat(order, mthp_event); } -static void collapse_control_init_scan(struct collapse_control *cc) +static void collapse_file_scan_init(struct collapse_control *cc) { memset(cc->node_load, 0, sizeof(cc->node_load)); nodes_clear(cc->alloc_nmask); @@ -3493,7 +3497,7 @@ static enum scan_result collapse_file(struct mm_struct *mm, unsigned long addr, return result; } -static enum scan_result collapse_scan_file(struct mm_struct *mm, +static enum scan_result collapse_pagecache_pmd(struct mm_struct *mm, unsigned long addr, struct file *file, pgoff_t start, struct collapse_control *cc) { @@ -3508,7 +3512,7 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm, present = 0; swap = 0; - collapse_control_init_scan(cc); + collapse_file_scan_init(cc); rcu_read_lock(); xas_for_each(&xas, folio, start + HPAGE_PMD_NR - 1) { if (xas_retry(&xas, folio)) @@ -3600,46 +3604,65 @@ static enum scan_result collapse_scan_file(struct mm_struct *mm, } /* - * Try to collapse a single PMD starting at a PMD aligned addr, and return - * the results. + * Judge one table's worth of a file VMA. All it needs of the VMA is the file and + * the offset, which it takes while it still has both; the collapse works on the + * page cache and never sees a VMA. */ -enum scan_result collapse_single_pmd(unsigned long addr, - unsigned long end, struct vm_area_struct *vma, - bool *lock_dropped, struct collapse_control *cc) +static enum scan_result collapse_scan_file_pmd(struct vm_area_struct *vma, + unsigned long addr, struct collapse_control *cc) { - struct mm_struct *mm = vma->vm_mm; + enum scan_result result; + pmd_t *pmd; + + /* + * A file collapse only ever builds a PMD, so the whole table has to be + * the VMA's -- a PMD shared with another VMA would need all of them + * locked. Not the question collapse_possible_orders() answered, which is + * whether the VMA may use the order at all: this is whether the table at + * @addr is wholly inside it. While a file VMA collapses at PMD order + * alone its callers hand over whole tables and this cannot fire, but the + * anonymous side already hands over parts of one. + */ + if (!thp_vma_suitable_order(vma, addr, HPAGE_PMD_ORDER)) + return SCAN_ADDRESS_RANGE; + + /* + * A PMD that is huge already has nothing left to collapse, and skipping + * it here is what keeps mmap_lock out of a collapse that would find + * nothing. Everything else is worth the page cache scan, pmd_none() + * included: a file range can be collapsed out of the cache without being + * mapped first, which is why this is not the test the anonymous side + * makes. + */ + result = find_pmd_or_thp_or_none(vma->vm_mm, addr & HPAGE_PMD_MASK, &pmd); + if (result == SCAN_PMD_MAPPED) + return result; + + cc->scan_file = get_file(vma->vm_file); + cc->scan_pgoff = linear_page_index(vma, addr); + + return SCAN_SUCCEED; +} + +/* + * Build a PMD over what the page cache holds, and map it over the range if a huge + * folio is already there but mapped by PTEs. Runs with no mmap_lock, which the + * caller gave up, and takes it again only for that last step. + */ +static enum scan_result collapse_file_pmd(struct mm_struct *mm, + unsigned long addr, struct collapse_control *cc) +{ + struct file *file = cc->scan_file; bool triggered_wb = false; enum scan_result result; - struct file *file; - pgoff_t pgoff; - mmap_assert_locked(mm); - - if (vma_is_anonymous(vma)) { - result = collapse_scan_anon_pmd(vma, addr, end, cc); - if (!cc->select_orders) - goto end; - - /* collapse_anon_pmd() takes mmap_lock itself, where it needs it */ - mmap_read_unlock(mm); - *lock_dropped = true; - - result = collapse_anon_pmd(mm, addr, end, cc); - goto end; - } - - file = get_file(vma->vm_file); - pgoff = linear_page_index(vma, addr); - - mmap_read_unlock(mm); - *lock_dropped = true; retry: - result = collapse_scan_file(mm, addr, file, pgoff, cc); + result = collapse_pagecache_pmd(mm, addr, file, cc->scan_pgoff, cc); /* Dirty pages are worth a writeback and one more try, if asked for */ if (cc->policy.writeback_dirty && result == SCAN_PAGE_DIRTY_OR_WRITEBACK && !triggered_wb && mapping_can_writeback(file->f_mapping)) { - const loff_t lstart = (loff_t)pgoff << PAGE_SHIFT; + const loff_t lstart = (loff_t)cc->scan_pgoff << PAGE_SHIFT; const loff_t lend = lstart + HPAGE_PMD_SIZE - 1; filemap_write_and_wait_range(file->f_mapping, lstart, lend); @@ -3647,6 +3670,7 @@ enum scan_result collapse_single_pmd(unsigned long addr, goto retry; } fput(file); + cc->scan_file = NULL; if (result == SCAN_PTE_MAPPED_HUGEPAGE) { mmap_read_lock(mm); @@ -3659,6 +3683,67 @@ enum scan_result collapse_single_pmd(unsigned long addr, result = SCAN_SUCCEED; mmap_read_unlock(mm); } -end: + return result; } + +/* + * Scan one table's worth of @vma and decide whether there is anything to collapse + * in it. The caller holds mmap_lock for reading and still holds it when this + * returns: what is looked at is either the VMA or a page table that the lock + * keeps in place. + * + * Returns whether collapse_run_pmd() has anything to do, and a scan that found + * something has to be run: the file side takes a reference on the file while it + * still has the VMA to take it from, and the run is what gives it back. What the + * scan turned down is left in cc->scan_refusal either way. + */ +bool collapse_scan_pmd(struct vm_area_struct *vma, unsigned long addr, + unsigned long end, struct collapse_control *cc, + unsigned long vma_orders) +{ + struct mm_struct *mm = vma->vm_mm; + + mmap_assert_locked(mm); + + /* + * What the scan answers with, so cleared before it runs. + * collapse_anon_scan_init() clears the orders too, but only once the + * table has turned out to be there. + */ + cc->select_orders = 0; + + /* Ours to give back only if the last scan was never run */ + if (WARN_ON_ONCE(cc->scan_file)) { + fput(cc->scan_file); + cc->scan_file = NULL; + } + + if (unlikely(collapse_test_exit_or_disable(mm))) + cc->scan_refusal = SCAN_ANY_PROCESS; + else if (addr < vma->vm_start || end > vma->vm_end) + cc->scan_refusal = SCAN_ADDRESS_RANGE; + else if (!vma_orders) + cc->scan_refusal = SCAN_VMA_CHECK; + else if (vma_is_anonymous(vma)) + collapse_scan_anon_pmd(vma, addr, end, cc, vma_orders); + else + cc->scan_refusal = collapse_scan_file_pmd(vma, addr, cc); + + return cc->select_orders || cc->scan_file; +} + +/* + * Collapse what the scan selected. Called with no mmap_lock: the caller gives it + * up first, because a collapse takes it again for each round and revalidates + * under it, and holding it across the whole collapse would keep a writer to the + * address space waiting for it. + */ +enum scan_result collapse_run_pmd(struct mm_struct *mm, unsigned long addr, + unsigned long end, struct collapse_control *cc) +{ + if (cc->scan_file) + return collapse_file_pmd(mm, addr, cc); + else + return collapse_anon_pmd(mm, addr, end, cc); +} diff --git a/mm/collapse.h b/mm/collapse.h index dc60806fb81e..4af7bb9c4261 100644 --- a/mm/collapse.h +++ b/mm/collapse.h @@ -150,6 +150,13 @@ struct collapse_control { */ enum scan_result scan_refusal; + /* + * A reference the file side takes while it still has the VMA, since the + * collapse runs without it, and the offset it decided on. + */ + struct file *scan_file; + pgoff_t scan_pgoff; + /* Why the last window was refused */ enum scan_result select_result; @@ -187,12 +194,47 @@ static inline int collapse_test_exit_or_disable(struct mm_struct *mm) mm_flags_test(MMF_DISABLE_THP_COMPLETELY, mm); } +/* + * A caller states what it allows in the policy, takes a control for the arrays a + * round needs, and then hands over one PTE table's worth of a VMA at a time: + * + * collapse_control_init(cc); once per control + * fill in cc->policy; what this caller allows + * collapse_scan_pmd(vma, addr, end, cc); per table, as often as wanted + * collapse_run_pmd(mm, addr, end, cc); when the scan found work + * collapse_control_release(cc); + * + * The caller holds mmap_lock for reading and passes a range within one PTE table + * of @vma. A range the VMA does not cover is refused, which is also how a caller + * learns that its own range shrank. + * + * A scan returns with that lock still held: it only reads, and almost every table + * it is offered has nothing to collapse, so a caller walks a whole VMA under the + * one lock it took to get there. + * + * A collapse is called without it: the caller gives the lock up first, and with it + * @vma and anything derived under it, so a caller carrying on has to look up + * again. What the collapse does -- allocate, quiesce, copy, flush -- is slow + * enough that a writer would wait behind it, so it takes the lock again per round + * instead, and revalidates rather than trusting what the scan saw. + * + * A scan that found something has to be run: the file side takes a reference on + * the file while it still has the VMA to take it from, and the run is what gives + * it back. What it turned down is left in cc->scan_refusal, for a caller that has + * to report why a table was not collapsed. + * + * A control is not reentrant: it carries the arrays a round works out of, so one + * per collapsing thread. + */ int collapse_control_init(struct collapse_control *cc); void collapse_control_release(struct collapse_control *cc); -enum scan_result collapse_single_pmd(unsigned long addr, unsigned long end, - struct vm_area_struct *vma, bool *lock_dropped, - struct collapse_control *cc); +bool collapse_scan_pmd(struct vm_area_struct *vma, unsigned long addr, + unsigned long end, struct collapse_control *cc, + unsigned long vma_orders); +enum scan_result collapse_run_pmd(struct mm_struct *mm, unsigned long addr, + unsigned long end, struct collapse_control *cc); +/* Which orders a VMA may collapse to, empty when it may not collapse at all */ unsigned long collapse_possible_orders(struct vm_area_struct *vma, vm_flags_t vm_flags, enum tva_type tva_flags); diff --git a/mm/khugepaged.c b/mm/khugepaged.c index b7fc93e11d6b..47c134cd4129 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -485,64 +485,6 @@ static void collapse_policy_khugepaged(struct collapse_policy *p) p->tva_type = TVA_KHUGEPAGED; } -/* MADV_COLLAPSE was asked for explicitly, so it is not held to those. */ -static void collapse_policy_forced(struct collapse_policy *p) -{ - p->max_ptes_none = HPAGE_PMD_NR; - p->max_ptes_swap = HPAGE_PMD_NR; - p->max_ptes_shared = HPAGE_PMD_NR; - p->strict_sub_pmd = false; - p->skip_lazyfree = false; - p->require_referenced = false; - p->install_pmd = true; - p->writeback_dirty = true; - p->gfp = GFP_TRANSHUGE; - p->tva_type = TVA_FORCED_COLLAPSE; -} - -/* - * If mmap_lock temporarily dropped, revalidate vma - * after taking the mmap_lock again. - * Returns enum scan_result value. - */ - -static enum scan_result hugepage_vma_revalidate(struct mm_struct *mm, unsigned long address, - bool expect_anon, struct vm_area_struct **vmap, - struct collapse_control *cc, unsigned int order) -{ - struct vm_area_struct *vma; - enum tva_type type = cc->policy.tva_type; - - if (unlikely(collapse_test_exit_or_disable(mm))) - return SCAN_ANY_PROCESS; - - *vmap = vma = find_vma(mm, address); - if (!vma) - return SCAN_VMA_NULL; - - /* - * We cannot collapse VMA regions that do not span the full PMD. This is - * due to the potential of the PMD being shared by another VMA leaving - * us vulnerable to a race condition. Always check the PMD order here to - * ensure its not shared by another VMA. We'd need to lock all VMAs in - * the PMD range to support this. - */ - if (!thp_vma_suitable_order(vma, address, PMD_ORDER)) - return SCAN_ADDRESS_RANGE; - if (!thp_vma_allowable_orders(vma, vma->vm_flags, type, BIT(order))) - return SCAN_VMA_CHECK; - /* - * Anon VMA expected, the address may be unmapped then - * remapped to file after khugepaged reacquired the mmap_lock. - * - * thp_vma_allowable_orders() may return true for qualified file - * vmas. - */ - if (expect_anon && (!(*vmap)->anon_vma || !vma_is_anonymous(*vmap))) - return SCAN_PAGE_ANON; - return SCAN_SUCCEED; -} - static void collect_mm_slot(struct mm_slot *slot) { struct mm_struct *mm = slot->mm; @@ -636,8 +578,7 @@ static void collapse_scan_mm_slot(unsigned int progress_max, khugepaged_scan.address = hstart; while (khugepaged_scan.address < hend) { - unsigned long pmd_addr, range_end; - bool lock_dropped = false; + unsigned long pmd_addr, range_end, start; /* One table's worth at most, and never past the VMA */ pmd_addr = khugepaged_scan.address & HPAGE_PMD_MASK; @@ -649,24 +590,24 @@ static void collapse_scan_mm_slot(unsigned int progress_max, VM_WARN_ON_ONCE(khugepaged_scan.address < hstart); - *result = collapse_single_pmd(khugepaged_scan.address, - range_end, vma, - &lock_dropped, cc); - if (*result == SCAN_SUCCEED) - ++khugepaged_pages_collapsed; + start = khugepaged_scan.address; /* move to next address */ khugepaged_scan.address = range_end; - if (lock_dropped) - /* - * We released mmap_lock so break loop. Note - * that we drop mmap_lock before all hugepage - * allocations, so if allocation fails, we are - * guaranteed to break here and report the - * correct result back to caller. - */ - goto breakouterloop_mmap_lock; - if (cc->progress >= progress_max) - goto breakouterloop; + + /* If nothing to collapse, the lock is still ours */ + if (!collapse_scan_pmd(vma, start, range_end, cc, orders)) { + *result = cc->scan_refusal; + if (cc->progress >= progress_max) + goto breakouterloop; + continue; + } + + /* collapse_run_pmd() takes its own locks, so give this up */ + mmap_read_unlock(mm); + *result = collapse_run_pmd(mm, start, range_end, cc); + if (*result == SCAN_SUCCEED) + ++khugepaged_pages_collapsed; + goto breakouterloop_mmap_lock; } } breakouterloop: @@ -904,6 +845,21 @@ bool current_is_khugepaged(void) return kthread_func(current) == khugepaged; } +/* MADV_COLLAPSE was asked for explicitly, so it is not held to those. */ +static void collapse_policy_forced(struct collapse_policy *p) +{ + p->max_ptes_none = HPAGE_PMD_NR; + p->max_ptes_swap = HPAGE_PMD_NR; + p->max_ptes_shared = HPAGE_PMD_NR; + p->strict_sub_pmd = false; + p->skip_lazyfree = false; + p->require_referenced = false; + p->install_pmd = true; + p->writeback_dirty = true; + p->gfp = GFP_TRANSHUGE; + p->tva_type = TVA_FORCED_COLLAPSE; +} + static int madvise_collapse_errno(enum scan_result r) { /* @@ -942,9 +898,10 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start, struct collapse_control *cc; struct mm_struct *mm = vma->vm_mm; unsigned long hstart, hend, addr; + /* What the VMA allows; valid only while its lock is held */ + unsigned long vma_orders; enum scan_result last_fail = SCAN_FAIL; int thps = 0; - bool mmap_unlocked = false; int err; BUG_ON(vma->vm_start > start); @@ -971,28 +928,67 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start, } mmgrab(mm); + + /* + * Nothing below wants the lock the VMA walk left held, and + * lru_add_drain_all() waits on every CPU, so give it up first. The + * walk carries on under mmap_lock and its own caller is what drops it, + * so reporting this only tells the walk that its VMA is now stale. + */ + mmap_read_unlock(mm); + *lock_dropped = true; + vma = NULL; + vma_orders = 0; lru_add_drain_all(); for (addr = hstart; addr < hend; addr += HPAGE_PMD_SIZE) { - enum scan_result result = SCAN_FAIL; + enum scan_result result; - if (mmap_unlocked) { + /* + * A collapse gives the lock up, and the VMA has to be found + * again after one: it can shrink while nothing is held. A scan + * that finds nothing to collapse leaves the lock alone, so a + * range that is already collapsed walks it without relocking. + * + * Reschedule only here, where nothing is held: a preemption + * point under a lock is a writer waiting longer. + */ + if (!vma) { cond_resched(); mmap_read_lock(mm); - mmap_unlocked = false; - *lock_dropped = true; - result = hugepage_vma_revalidate(mm, addr, false, &vma, - cc, HPAGE_PMD_ORDER); - if (result != SCAN_SUCCEED) { - last_fail = result; - goto out_nolock; + vma = vma_lookup(mm, addr); + if (!vma) { + mmap_read_unlock(mm); + hend = addr; + break; } - - hend = min(hend, vma->vm_end & HPAGE_PMD_MASK); + vma_orders = collapse_possible_orders(vma, + vma->vm_flags, TVA_FORCED_COLLAPSE); } - result = collapse_single_pmd(addr, addr + HPAGE_PMD_SIZE, vma, - &mmap_unlocked, cc); + /* If nothing to collapse, the lock is still ours */ + if (!collapse_scan_pmd(vma, addr, addr + HPAGE_PMD_SIZE, cc, + vma_orders)) { + result = cc->scan_refusal; + } else { + /* collapse_run_pmd() takes its own locks, so give this up */ + mmap_read_unlock(mm); + vma = NULL; + /* The mask belonged to that lock, not to this range */ + vma_orders = 0; + + result = collapse_run_pmd(mm, addr, + addr + HPAGE_PMD_SIZE, cc); + } + + /* + * The VMA shrank under us, so the rest of the range was never + * ours to collapse: stop, and expect only what came before. + */ + if (result == SCAN_VMA_NULL || result == SCAN_ADDRESS_RANGE) { + hend = addr; + break; + } switch (result) { case SCAN_SUCCEED: @@ -1015,18 +1011,14 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start, default: last_fail = result; /* Other error, exit */ - goto out_maybelock; + goto out; } } -out_maybelock: - /* Caller expects us to hold mmap_lock on return */ - if (mmap_unlocked) { - *lock_dropped = true; +out: + /* The VMA walk this returns to expects the lock it was holding */ + if (!vma) mmap_read_lock(mm); - } -out_nolock: - mmap_assert_locked(mm); mmdrop(mm); collapse_control_release(cc); kfree(cc); diff --git a/mm/mremap.c b/mm/mremap.c index e8df5cdb0ac9..a25ac3db787a 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -244,7 +244,7 @@ static int move_ptes(struct pagetable_move_control *pmc, goto out; } /* - * Now new_pte is none, so collapse_scan_file() path can not find + * Now new_pte is none, so collapse_pagecache_pmd() path can not find * this by traversing file->f_mapping, so there is no concurrency with * retract_page_tables(). In addition, we already hold the exclusive * mmap_lock, so this new_pte page is stable, so there is no need to get -- 2.54.0
