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)
