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 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?

> 
> >+
> >+            i += nr;
> >+            addr += nr * PAGE_SIZE;
> >+    }
> >+
> >+    cand->state = CAND_FROZEN;
> >+    return SCAN_SUCCEED;
> >+
> >+unfreeze:
> >+    collapse_unfreeze_candidate(mm, cand, pte, nr_saved, nr_frozen);
> >+    return result;
> 
> And collapse_unfreeze_candidate() restores the source PTEs and refcount,
> then unlocks and puts the source folio :)
> 
> The source folio is not added back to the deferred split queue, so an
> underused or partially mapped folio can remain off the queue.
> 
> Should collapse unqueue the source folio after folio_ref_freeze()
> succeeds, remember whether it was on the deferred split queue and whether
> PG_partially_mapped was set, then requeue it if
> collapse_unfreeze_candidate() restores the source folio?
> 
> [...]
> 
> Cheers, Lance

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Reply via email to