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
