Re: [PATCH v3 04/18] mm/huge_memory: split the routine for splitting anon and file folio

["David Hildenbrand (Arm)" <david@kernel.org>]

On 8/20/26 20:55, Kairui Song via B4 Relay wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> No functional change intended. Before adding more logic, split
> __folio_freeze_and_split_unmapped() into an anon and a file variant so
> each path can evolve independently. The two paths shared little beyond
> the folio freeze call, the LRU locking, and the unfreeze skeleton, but
> differed in all other per-folio bookkeeping and routines.

Splitting that up makes sense.

> 
> While splitting, some cleanups become easy to apply, and helped drop a
> few now-redundant checks. Also introduce a folio iteration helper to
> avoid a common pitfall of iterating post-split sub-folios: a sub folio
> might get freed mid-iteration as pointed out by Zi [1].
> 
> Link: https://lore.kernel.org/linux-mm/DKJSFCLP967N.YBR4DNK1NM2N@nvidia.com/ [1]
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  mm/huge_memory.c | 119 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 75 insertions(+), 44 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 7fb603ac500f..c3fd6757c14c 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3933,11 +3933,9 @@ static unsigned int folio_cache_ref_count(const struct folio *folio)
>  	return folio_nr_pages(folio);
>  }
>  
> -static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int new_order,
> -					     struct page *split_at, struct xa_state *xas,
> -					     struct address_space *mapping, bool do_lru,
> -					     struct list_head *list, enum split_type split_type,
> -					     pgoff_t end, int *nr_shmem_dropped)
> +static int __folio_freeze_split_unmapped_anon(struct folio *folio, unsigned int new_order,
> +					      struct page *split_at, bool do_lru,
> +					      struct list_head *list, enum split_type split_type)

Switch to double tab indentation instead while at it. Same for the other function.

>  {
>  	struct folio *end_folio = folio_next(folio);
>  	struct swap_cluster_info *ci = NULL;
> @@ -3948,7 +3946,6 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
>  	bool dequeue_deferred;
>  	int ret = 0;
>  
> -	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
> @@ -3956,7 +3953,7 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
>  	 * lock and must clean up the LRU state - the same dequeue we
>  	 * will do below as part of the split.
>  	 */
> -	dequeue_deferred = folio_test_anon(folio) && old_order > 1;
> +	dequeue_deferred = old_order > 1;
>  	if (dequeue_deferred) {
>  		struct mem_cgroup *memcg;
>  
> @@ -3986,24 +3983,72 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
>  		rcu_read_unlock();
>  	}
>  
> -	if (mapping) {
> +	if (folio_test_swapcache(folio))
> +		ci = swap_cluster_get_and_lock(folio);
> +
> +	if (do_lru)
> +		lruvec = folio_lruvec_lock(folio);
> +
> +	ret = __split_unmapped_folio(folio, new_order, split_at, NULL,
> +				     NULL, split_type);
> +
> +	/*
> +	 * Unfreeze the post-split folios and put them back to the right

Why call it "post-split" here when it's "after-split" in the other comment?

> +	 * place. Keep the head @folio frozen until the end: sub entries
> +	 * in swap cache must be updated first, so a concurrent
> +	 * swap_cache_get_folio() cannot return the head folio for a sub
> +	 * entry (folio_try_get() will fail on the head @folio until unfreeze).
> +	 */
> +	for (new_folio = folio_next(folio); new_folio != end_folio;
> +	     new_folio = next) {
> +		next = folio_next(new_folio);
> +		zone_device_private_split_cb(folio, new_folio);
> +		folio_ref_unfreeze(new_folio,
> +				   folio_cache_ref_count(new_folio) + 1);
> +		if (do_lru)
> +			lru_add_split_folio(folio, new_folio, lruvec, list);
> +		if (ci)
> +			__swap_cache_replace_folio(ci, folio, new_folio);
> +	}

This smells like duplicate code now. That should better be factored out?

[...]

> -	zone_device_private_split_cb(folio, NULL);
>  	/*
>  	 * Unfreeze @folio only after all page cache entries, which
>  	 * used to point to it, have been updated with new folios.
> @@ -4075,8 +4105,6 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
>  
>  	if (do_lru)
>  		lruvec_unlock(lruvec);
> -	if (ci)
> -		swap_cluster_unlock(ci);
>  
>  	return ret;
>  }
> @@ -4230,10 +4258,14 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  			ret = -EAGAIN;
>  			goto fail;
>  		}
> +		ret = __folio_freeze_split_unmapped_file(folio, new_order, split_at, &xas, mapping,
> +							 true, list, split_type, end,
> +							 &nr_shmem_dropped);
> +	} else {
> +		ret = __folio_freeze_split_unmapped_anon(folio, new_order, split_at, true,
> +							 list, split_type);
>  	}


I was briefly wondering whether shmem folios in the swapcache would now go
through __folio_freeze_split_unmapped_anon(). But folio_check_splittable()
rejects them.

I think we should consider changing all the "if (mapping)" checks there to
"is_anon" instead.

is_anon implies no mapping and !is_anon implies that we need a mapping.


This patch LGTM, but I think we should clean up __folio_split() further. Maybe
taht happens in the next patches in this series :)

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David