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

["Zi Yan" <ziy@nvidia.com>]

On Fri Aug 7, 2026 at 5:17 PM EDT, 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.

While at it, can you rename __split_unmapped_folio() to
__split_frozen_folio() to reflect the actual folio state? It is causing
confusion and people tried to use __split_unmapped_folio() on non frozen
folios.

>
> While splitting, some cleanups become easy to apply, and helped dropping
> a few now redundant checks.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> ---
>  mm/huge_memory.c | 121 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 76 insertions(+), 45 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index cf8f90b94e42..56a356c30f30 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3934,22 +3934,19 @@ 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)
>  {
>  	struct folio *end_folio = folio_next(folio);
>  	struct swap_cluster_info *ci = NULL;
> -	struct folio *new_folio, *next;
> +	struct folio *new_folio;
>  	int old_order = folio_order(folio);
>  	struct list_lru_one *lru;
>  	struct lruvec *lruvec;
>  	bool dequeue_deferred;
>  	int ret = 0;
>  
> -	VM_WARN_ON_ONCE(!mapping && end);

We no longer need mapping here, the caller already makes sure mapping is
NULL. Great!

>  	/*
>  	 * If this folio can be on the deferred split queue, lock out
>  	 * the shrinker before freezing the ref. If the shrinker sees
> @@ -3957,7 +3954,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;
>  
> @@ -3987,24 +3984,73 @@ 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 after-split folios and put them back to the right
> +	 * place, keeping the head @folio frozen until the end. While the
> +	 * folio is in the swap cache, the sub entries must be updated with
> +	 * their after-split folios before the head is unfrozen, so a
> +	 * concurrent swap_cache_get_folio() cannot return the head folio
> +	 * for a sub entry. Keeping the head frozen throughout also stops a
> +	 * parallel folio_try_get() from observing a partially split folio.
> +	 */
> +	for (new_folio = folio_next(folio); new_folio != end_folio;
> +	     new_folio = folio_next(new_folio)) {

Please keep the existing for loop pattern by using next =
folio_next(new_folio) in the loop buddy.

Hugh pointed out an issue when I did the above for loop pattern[1].
Basically, folio_next() reads folio_nr_pages() and relies on a stable
new_folio input. In my old code, the input of folio_next() can be freed
and causing oops. In your code, that does not apply, but it can bite
people in the future the loop body changes and new_folio's lifetime ends
before the for loop finishes.

Maybe add a comment to explain why next = folio_next(new_folio) should
be used.

[1] https://lore.kernel.org/all/2fae27fe-6e2e-3587-4b68-072118d80cf8@google.com/

> +		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);
> +	}
> +
> +	zone_device_private_split_cb(folio, NULL);
> +	folio_ref_unfreeze(folio, folio_cache_ref_count(folio) + 1);
> +
> +	if (do_lru)
> +		lruvec_unlock(lruvec);
> +	if (ci)
> +		swap_cluster_unlock(ci);
> +
> +	return ret;
> +}
> +
> +static int __folio_freeze_split_unmapped_file(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)
> +{
> +	struct folio *end_folio = folio_next(folio);
> +	struct folio *new_folio, *next;
> +	struct lruvec *lruvec;
> +	int ret;
> +
> +	if (!folio_ref_freeze(folio, folio_cache_ref_count(folio) + 1))
> +		return -EAGAIN;
> +
> +	if (folio_test_pmd_mappable(folio) &&
> +	    new_order < HPAGE_PMD_ORDER) {
>  		int nr = folio_nr_pages(folio);
>  
> -		if (folio_test_pmd_mappable(folio) &&
> -		    new_order < HPAGE_PMD_ORDER) {
> -			if (folio_test_swapbacked(folio)) {
> -				lruvec_stat_mod_folio(folio,
> -						      NR_SHMEM_THPS, -nr);
> -			} else {
> -				lruvec_stat_mod_folio(folio,
> -						      NR_FILE_THPS, -nr);
> -			}
> +		if (folio_test_swapbacked(folio)) {
> +			lruvec_stat_mod_folio(folio,
> +					      NR_SHMEM_THPS, -nr);
> +		} else {
> +			lruvec_stat_mod_folio(folio,
> +					      NR_FILE_THPS, -nr);
>  		}
>  	}
>  
> -	if (folio_test_swapcache(folio))
> -		ci = swap_cluster_get_and_lock(folio);
> -
>  	/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
>  	if (do_lru)
>  		lruvec = folio_lruvec_lock(folio);
> @@ -4014,7 +4060,7 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
>  
>  	/*
>  	 * Unfreeze after-split folios and put them back to the right
> -	 * list. @folio should be kept frozon until page cache
> +	 * list. @folio should be kept frozen until page cache
>  	 * entries are updated with all the other after-split folios
>  	 * to prevent others seeing stale page cache entries.
>  	 * As a result, new_folio starts from the next folio of
> @@ -4026,27 +4072,12 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
>  
>  		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);
>  
> -		/*
> -		 * Anonymous folio with swap cache.
> -		 * NOTE: shmem in swap cache is not supported yet.
> -		 */
> -		if (ci) {
> -			__swap_cache_replace_folio(ci, folio, new_folio);
> -			continue;
> -		}
> -
> -		/* Anonymous folio without swap cache */
> -		if (!mapping)
> -			continue;
> -
>  		/* Add the new folio to the page cache. */
>  		if (new_folio->index < end) {
>  			__xa_store(&mapping->i_pages, new_folio->index,
> @@ -4065,7 +4096,6 @@ static int __folio_freeze_and_split_unmapped(struct folio *folio, unsigned int n
>  		folio_put_refs(new_folio, nr_pages);
>  	}
>  
> -	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.
> @@ -4076,8 +4106,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;
>  }
> @@ -4231,10 +4259,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);
>  	}
>  
> -	ret = __folio_freeze_and_split_unmapped(folio, new_order, split_at, &xas, mapping,
> -						true, list, split_type, end, &nr_shmem_dropped);
>  fail:
>  	if (mapping)
>  		xas_unlock(&xas);
> @@ -4334,9 +4366,8 @@ int folio_split_unmapped(struct folio *folio, unsigned int new_order)
>  		return -EAGAIN;
>  
>  	local_irq_disable();
> -	ret = __folio_freeze_and_split_unmapped(folio, new_order, &folio->page, NULL,
> -						NULL, false, NULL, SPLIT_TYPE_UNIFORM,
> -						0, NULL);
> +	ret = __folio_freeze_split_unmapped_anon(folio, new_order, &folio->page,
> +						 false, NULL, SPLIT_TYPE_UNIFORM);
>  	local_irq_enable();
>  	return ret;
>  }

There are some code duplications but overall looks good to me. The lru
lock, unfreeze loop, and the last unfreeze are replicated across two
functions. I cannot think of an easy alternative. A tiny improvement
might be instead of replicating unfreeze comments, changing one to point
to the other one and asking the code should be in sync.

-- 
Best Regards,
Yan, Zi