Re: [RFC PATCH 16/57] mm/collapse: freeze the sources behind migration entries
From: Usama Arif <usama.arif@linux.dev>
Date: 2026-08-24 16:13:55
Also in:
bpf, linux-kselftest, linux-mm, lkml
On 24/08/2026 16:49, Johannes Weiner wrote:
On Mon, Aug 24, 2026 at 03:13:10PM +0100, Kiryl Shutsemau wrote:quoted
On Mon, Aug 24, 2026 at 09:12:24PM +0800, Lance Yang wrote:quoted
On Sun, Aug 16, 2026 at 11:45:28PM +0100, Kiryl Shutsemau wrote:quoted
+ if (!folio_ref_freeze(folio, + folio_expected_ref_count(folio) + 1)) { + result = SCAN_PAGE_COUNT; + goto unfreeze; + } + nr_frozen = nr_saved;Just one thing I was wondering about ... can deferred_split_isolate() remove a source folio from deferred_split_lru while its refcount is frozen by collapse_freeze_candidate()? Assume an earlier span belongs to an anonymous large folio on the deferred split queue, then a later span fails folio_trylock(). collapse_freeze_candidate() continues after freezing each source folio and calls collapse_unfreeze_candidate() on a later failure: static noinline enum scan_result collapse_freeze_candidate(struct mm_struct *mm, struct collapse_candidate *cand, pte_t *pte) { ... for (i = 0, addr = cand->addr; i < nr_pages;) { ... if (!folio_trylock(folio)) { folio_put(folio); result = SCAN_PAGE_LOCK; goto unfreeze; } ... nr_saved = i + nr; if (!folio_ref_freeze(folio, folio_expected_ref_count(folio) + 1)) { result = SCAN_PAGE_COUNT; goto unfreeze; } nr_frozen = nr_saved; i += nr; addr += nr * PAGE_SIZE; } ... unfreeze: collapse_unfreeze_candidate(mm, cand, pte, nr_saved, nr_frozen); return result; } folio_ref_freeze() takes the source folio's refcount to zero: static inline int folio_ref_freeze(struct folio *folio, int count) { return page_ref_freeze(&folio->page, count); } static inline int page_ref_freeze(struct page *page, int count) { int ret = likely(atomic_cmpxchg(&page->_refcount, count, 0) == count); ... return ret; } While collapse_freeze_candidate() still holds the source folio lock, deferred_split_scan() can call deferred_split_isolate(): static unsigned long deferred_split_scan(struct shrinker *shrink, struct shrink_control *sc) { LIST_HEAD(dispose); struct folio *folio, *next; int split = 0; unsigned long isolated; isolated = list_lru_shrink_walk_irq(&deferred_split_lru, sc, deferred_split_isolate, &dispose); } static enum lru_status deferred_split_isolate(struct list_head *item, struct list_lru_one *lru, void *cb_arg) { struct folio *folio = container_of(item, struct folio, _deferred_list); struct list_head *freeable = cb_arg; if (folio_try_get(folio)) { list_lru_isolate_move(lru, item, freeable); return LRU_REMOVED; } /* * We lost race with folio_put(). Read folio state before the * isolate: folio_unqueue_deferred_split() checks list_empty() * locklessly, so once removed the folio can be freed any time. */ if (folio_test_partially_mapped(folio)) { folio_clear_partially_mapped(folio); mod_mthp_stat(folio_order(folio), MTHP_STAT_NR_ANON_PARTIALLY_MAPPED, -1); } list_lru_isolate(lru, item); return LRU_REMOVED; } And folio_try_get() fails because the source folio has a frozen refcount. deferred_split_isolate() treats the failure as a race with folio_put(), clears PG_partially_mapped and its MTHP_STAT_NR_ANON_PARTIALLY_MAPPED accounting when set, then removes the folio from deferred_split_lru ...Hm. So, the premise in deferred_split_isolate() is false: !folio_try_get() doesn't mean lost race with folio_put().I suppose you mean, not exclusively. But it can also mean that. And then the question is, who cleans up the partially_mapped state.quoted
I think deferred_split_isolate() should do something like: if (!folio_try_get(folio)) return LRU_SKIP; list_lru_isolate_move(lru, item, freeable); return LRU_REMOVED; Johannes, do I miss something?Ah. The idea being: leave the item on the LRU when there is a race with the refcount going zero; and then it's up to that other side to deal with/clean up the partially_mapped state as appropriate. Thus: folio_put() __folio_put() folio_unqueue_deferred_split() if __list_lru_del(): // clear partially_mapped state & stats will always succeed, even if it races with the shrinker. And you are also guaranteed on the collapse side that the state won't vanish from underneath you. I think that should work. Usama?
Kiryl's suggestion makes sense. I think there is a bug here. If we dont get the reference, whoever owns the reference should decide how partially_mapped is treated for that folio: - If its the last folio_put(), it will clear partially_mapped and cleanup after itself. - If its folio_ref_freeze(), clearing partially_mapped is wrong (which we are currently doing in deferred_split_isolate)