Re: [RFC PATCH 16/57] mm/collapse: freeze the sources behind migration entries

[Lance Yang]

On Mon, Aug 24, 2026 at 05:13:52PM +0100, Usama Arif wrote:
>
>
>On 24/08/2026 16:49, Johannes Weiner wrote:
>> On Mon, Aug 24, 2026 at 03:13:10PM +0100, Kiryl Shutsemau wrote:
>>> On Mon, Aug 24, 2026 at 09:12:24PM +0800, Lance Yang wrote:
>>>>
>>>> On Sun, Aug 16, 2026 at 11:45:28PM +0100, Kiryl Shutsemau wrote:
>>>>> +		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.
>> 
>>> 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)

Yeah, returning LRU_SKIP looks right. I have this locally:

---8<---
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index e771244f42f3..a269775129c4 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3939,11 +3939,8 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n

 	VM_WARN_ON_ONCE(!mapping && end);
 	/*
-	 * If this folio can be on the deferred split queue, lock out
-	 * the shrinker before freezing the ref. If the shrinker sees
-	 * a 0-ref folio, it assumes it beat folio_put() to the list
-	 * lock and must clean up the LRU state - the same dequeue we
-	 * will do below as part of the split.
+	 * If this folio can be on the deferred split queue, hold the list_lru
+	 * lock across the refcount freeze and dequeue.
 	 */
 	dequeue_deferred = folio_test_anon(folio) && old_order > 1;
 	if (dequeue_deferred) {
@@ -4592,22 +4589,10 @@ static enum lru_status deferred_split_isolate(struct list_head *item,
 	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;
-	}
+	if (!folio_try_get(folio))
+		return LRU_SKIP;

-	/*
-	 * 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);
+	list_lru_isolate_move(lru, item, freeable);
 	return LRU_REMOVED;
 }
---

One detail, though ...

deferred_split_isolate()'s assumption predates patch 16. Patch 16 lets
collapse_freeze_candidate() leave refcount at zero and later unfreeze it
on rollback.

Existing split code takes a list_lru lock for deferred_split_lru before
folio_ref_freeze() and holds it through dequeue. The shrinker cannot see
that temporary zero.

collapse_freeze_candidate() does not take that lock and can roll back
later.

Say a candidate has two source folios. After the first one is frozen,
deferred_split_isolate() can see refcount 0, dequeue it, clear
PG_partially_mapped and decrement its accounting. If freezing the second
one fails, collapse rolls the first one back:

static void collapse_unfreeze_candidate(struct mm_struct *mm,
					struct collapse_candidate *cand,
					pte_t *pte, unsigned int nr_saved,
					unsigned int nr_frozen)
{
	unsigned long addr = cand->addr;
	unsigned int i = 0;

	while (i < nr_saved) {
		pte_t saved = cand->saved_ptes[i];
		struct folio *folio;
		unsigned int nr, k;

...
		folio = pte_folio(saved);
		nr = collapse_saved_span_len(cand, i, nr_saved);

		for (k = 0; k < nr; k++) {
			set_pte_at(mm, addr + k * PAGE_SIZE, pte + i + k,
				   cand->saved_ptes[i + k]);
		}
		if (i < nr_frozen) {
			folio_ref_unfreeze(folio,
					   folio_expected_ref_count(folio) + 1);
		}
		folio_unlock(folio);
		folio_put(folio);

		i += nr;
		addr += nr * PAGE_SIZE;
	}
}

PTEs and the folio's refcount are restored, but its deferred split queue
entry, PG_partially_mapped flag, and accounting are already gone. So yeah,
patch 16 has a real state-loss bug when collapse rolls back a partially
mapped folio.

LRU_SKIP looks right. A failed folio_try_get() only says refcount is zero.
It cannot tell a final folio_put() from a temporary freeze. A final put
still reaches:

void __folio_put(struct folio *folio)
{
...
	folio_unqueue_deferred_split(folio);
...
}

folio_unqueue_deferred_split() takes the same list_lru lock and clears
PG_partially_mapped + its accounting if the entry is still queued.

Cheers, Lance