Re: [PATCH v2 13/17] mm/huge_memory: drop the unused do_lru argument of the file split helper

[Kairui Song]

Sashiko reports (TLDR: false positives):

--- Patch [13]: [PATCH v2 13/17] mm/huge_memory: drop the unused
do_lru argument of the file split helper ---
commit 56c82d4146d59b581770011278661a173c988702
Author: Kairui Song <kasong@xxxxxxxxxxx>

mm/huge_memory: drop the unused do_lru argument of the file split helper

The only caller of __folio_split_unmap_and_freeze_file() always passes
do_lru as true, so the argument and the branches gated on it are dead
code.  Drop it.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ce02608b37f43..72e7d24139e6f 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c

[ ... ]

> @@ -4206,9 +4204,7 @@ static int __folio_split_unmap_and_freeze_file(struct folio *folio, unsigned int
>   }
>
>   /* lock lru list/PageCompound, ref frozen by page_ref_freeze */
> - if (do_lru)
> - lruvec = folio_lruvec_lock(folio);
> -
> + lruvec = folio_lruvec_lock(folio);
>   ret = __split_frozen_folio(folio, new_order, split_at, &xas,
>     mapping, split_type);

This is a pre-existing issue, but does this code permanently leak NR_FILE_THPS
or NR_SHMEM_THPS stats if __split_frozen_folio() fails?

Right before this lock acquisition in __folio_split_unmap_and_freeze_file(),
the stats are eagerly decremented. If __split_frozen_folio() returns an error
(like -ENOMEM from xas_try_split() during a non-uniform split), the error
path fails to restore these stats.

Since the split failed, the folio remains pmd_mappable(). When the folio
is later evicted or freed, filemap_unaccount_folio() will observe it is still
pmd_mappable() and decrement the stats again, causing a double-decrement
underflow.

[ ... ]

> @@ -4226,8 +4222,7 @@ static int __folio_split_unmap_and_freeze_file(struct folio *folio, unsigned int
>   folio_ref_unfreeze(new_folio,
>     folio_cache_ref_count(new_folio) + 1);
>
> - if (do_lru)
> - lru_add_split_folio(folio, new_folio, lruvec, list);
> + lru_add_split_folio(folio, new_folio, lruvec, list);
>
>   /* Add the new folio to the page cache. */
>   if (new_folio->index < end) {

This is a pre-existing issue, but does calling __xa_store() here inside
the page cache insertion loop leak memory and cause latency spikes?

__folio_split_unmap_and_freeze_file() correctly sets up a local XA_STATE
with mapping_set_update(&xas, mapping). However, the page cache insertion
loop (just below this snippet) uses __xa_store(), which creates a fresh local
XA_STATE that lacks the workingset_update_node callback.

Without this callback, xa_nodes are not added to their memcg's list_lru,
leaking nodes under memory pressure.

Additionally, because __xa_store() bypasses the advanced XA_STATE and
restarts from the root for every sub-folio, it turns an O(1) advancement
into an O(log N) tree walk for each slot while holding the xas_lock_irq
spinlock, which can cause severe latency.

---

For the stats question: the decrement is done up front, but no restore is
needed on the error path.  The uniform path preallocates all the xarray
nodes and checks for errors before the stats are touched, so it cannot
fail there.  The non-uniform path can fail - xas_try_split() does one
GFP_NOWAIT allocation when reducing the order-6 chunk - but by then the
folio has already been split below PMD order, so a failed split can only
leave behind an order-6 remnant, which is not pmd sized.

For the __xa_store() one: it doesn't need to allocate nodes,
every slot it writes already exists after xas_split()/xas_try_split().
Nodes the split creates are tracked through
workingset_update_node() (mapping_set_update() is set on the xas), so
nothing leaks.  The fresh root walk per sub-folio is real, not a bug,
It may be worth optimizing later.