remap(), folio_ref_unfreeze(), lru_add_split_folio() are not relevant to
splitting unmapped folio operations. Move them out to the caller so that
__split_unmapped_folio() only handles unmapped folio splits. This makes
__split_unmapped_folio() reusable.

Convert VM_BUG_ON(mapping) to use VM_WARN_ON_ONCE_FOLIO().

Signed-off-by: Zi Yan <ziy@nvidia.com>
Acked-by: Balbir Singh <balbirs@nvidia.com>
---
 mm/huge_memory.c | 291 +++++++++++++++++++++++------------------------
 1 file changed, 144 insertions(+), 147 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 3eb1c34be601..a7ee731f974f 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3396,10 +3396,6 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
  *             order - 1 to new_order).
  * @split_at: in buddy allocator like split, the folio containing @split_at
  *            will be split until its order becomes @new_order.
- * @lock_at: the folio containing @lock_at is left locked for caller.
- * @list: the after split folios will be added to @list if it is not NULL,
- *        otherwise to LRU lists.
- * @end: the end of the file @folio maps to. -1 if @folio is anonymous memory.
  * @xas: xa_state pointing to folio->mapping->i_pages and locked by caller
  * @mapping: @folio->mapping
  * @uniform_split: if the split is uniform or not (buddy allocator like split)
@@ -3425,52 +3421,26 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
  *    @page, which is split in next for loop.
  *
  * After splitting, the caller's folio reference will be transferred to the
- * folio containing @page. The other folios may be freed if they are not mapped.
- *
- * In terms of locking, after splitting,
- * 1. uniform split leaves @page (or the folio contains it) locked;
- * 2. buddy allocator like (non-uniform) split leaves @folio locked.
- *
+ * folio containing @page. The caller needs to unlock and/or free after-split
+ * folios if necessary.
  *
  * For !uniform_split, when -ENOMEM is returned, the original folio might be
  * split. The caller needs to check the input folio.
  */
 static int __split_unmapped_folio(struct folio *folio, int new_order,
-		struct page *split_at, struct page *lock_at,
-		struct list_head *list, pgoff_t end,
-		struct xa_state *xas, struct address_space *mapping,
-		bool uniform_split)
+		struct page *split_at, struct xa_state *xas,
+		struct address_space *mapping, bool uniform_split)
 {
-	struct lruvec *lruvec;
-	struct address_space *swap_cache = NULL;
-	struct folio *origin_folio = folio;
-	struct folio *next_folio = folio_next(folio);
-	struct folio *new_folio;
 	struct folio *next;
 	int order = folio_order(folio);
 	int split_order;
 	int start_order = uniform_split ? new_order : order - 1;
-	int nr_dropped = 0;
 	int ret = 0;
 	bool stop_split = false;
 
-	if (folio_test_swapcache(folio)) {
-		VM_BUG_ON(mapping);
-
-		/* a swapcache folio can only be uniformly split to order-0 */
-		if (!uniform_split || new_order != 0)
-			return -EINVAL;
-
-		swap_cache = swap_address_space(folio->swap);
-		xa_lock(&swap_cache->i_pages);
-	}
-
 	if (folio_test_anon(folio))
 		mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
 
-	/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
-	lruvec = folio_lruvec_lock(folio);
-
 	folio_clear_has_hwpoisoned(folio);
 
 	/*
@@ -3480,9 +3450,9 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
 	for (split_order = start_order;
 	     split_order >= new_order && !stop_split;
 	     split_order--) {
-		int old_order = folio_order(folio);
-		struct folio *release;
 		struct folio *end_folio = folio_next(folio);
+		int old_order = folio_order(folio);
+		struct folio *new_folio;
 
 		/* order-1 anonymous folio is not supported */
 		if (folio_test_anon(folio) && split_order == 1)
@@ -3504,126 +3474,44 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
 				if (xas_error(xas)) {
 					ret = xas_error(xas);
 					stop_split = true;
-					goto after_split;
 				}
 			}
 		}
 
-		folio_split_memcg_refs(folio, old_order, split_order);
-		split_page_owner(&folio->page, old_order, split_order);
-		pgalloc_tag_split(folio, old_order, split_order);
-
-		__split_folio_to_order(folio, old_order, split_order);
+		if (!stop_split) {
+			folio_split_memcg_refs(folio, old_order, split_order);
+			split_page_owner(&folio->page, old_order, split_order);
+			pgalloc_tag_split(folio, old_order, split_order);
 
-after_split:
+			__split_folio_to_order(folio, old_order, split_order);
+		}
 		/*
-		 * Iterate through after-split folios and perform related
-		 * operations. But in buddy allocator like split, the folio
+		 * Iterate through after-split folios and update folio stats.
+		 * But in buddy allocator like split, the folio
 		 * containing the specified page is skipped until its order
 		 * is new_order, since the folio will be worked on in next
 		 * iteration.
 		 */
-		for (release = folio; release != end_folio; release = next) {
-			next = folio_next(release);
+		for (new_folio = folio; new_folio != end_folio; new_folio = next) {
+			next = folio_next(new_folio);
 			/*
-			 * for buddy allocator like split, the folio containing
-			 * page will be split next and should not be released,
-			 * until the folio's order is new_order or stop_split
-			 * is set to true by the above xas_split() failure.
+			 * for buddy allocator like split, new_folio containing
+			 * page could be split again, thus do not change stats
+			 * yet. Wait until new_folio's order is new_order or
+			 * stop_split is set to true by the above xas_split()
+			 * failure.
 			 */
-			if (release == page_folio(split_at)) {
-				folio = release;
+			if (new_folio == page_folio(split_at)) {
+				folio = new_folio;
 				if (split_order != new_order && !stop_split)
 					continue;
 			}
-			if (folio_test_anon(release)) {
-				mod_mthp_stat(folio_order(release),
-						MTHP_STAT_NR_ANON, 1);
-			}
-
-			/*
-			 * origin_folio should be kept frozon until page cache
-			 * entries are updated with all the other after-split
-			 * folios to prevent others seeing stale page cache
-			 * entries.
-			 */
-			if (release == origin_folio)
-				continue;
-
-			folio_ref_unfreeze(release, 1 +
-					((mapping || swap_cache) ?
-						folio_nr_pages(release) : 0));
-
-			lru_add_split_folio(origin_folio, release, lruvec,
-					list);
-
-			/* Some pages can be beyond EOF: drop them from cache */
-			if (release->index >= end) {
-				if (shmem_mapping(mapping))
-					nr_dropped += folio_nr_pages(release);
-				else if (folio_test_clear_dirty(release))
-					folio_account_cleaned(release,
-						inode_to_wb(mapping->host));
-				__filemap_remove_folio(release, NULL);
-				folio_put_refs(release, folio_nr_pages(release));
-			} else if (mapping) {
-				__xa_store(&mapping->i_pages,
-						release->index, release, 0);
-			} else if (swap_cache) {
-				__xa_store(&swap_cache->i_pages,
-						swap_cache_index(release->swap),
-						release, 0);
-			}
+			if (folio_test_anon(new_folio))
+				mod_mthp_stat(folio_order(new_folio),
+					      MTHP_STAT_NR_ANON, 1);
 		}
 	}
 
-	/*
-	 * Unfreeze origin_folio only after all page cache entries, which used
-	 * to point to it, have been updated with new folios. Otherwise,
-	 * a parallel folio_try_get() can grab origin_folio and its caller can
-	 * see stale page cache entries.
-	 */
-	folio_ref_unfreeze(origin_folio, 1 +
-		((mapping || swap_cache) ? folio_nr_pages(origin_folio) : 0));
-
-	unlock_page_lruvec(lruvec);
-
-	if (swap_cache)
-		xa_unlock(&swap_cache->i_pages);
-	if (mapping)
-		xa_unlock(&mapping->i_pages);
-
-	/* Caller disabled irqs, so they are still disabled here */
-	local_irq_enable();
-
-	if (nr_dropped)
-		shmem_uncharge(mapping->host, nr_dropped);
-
-	remap_page(origin_folio, 1 << order,
-			folio_test_anon(origin_folio) ?
-				RMP_USE_SHARED_ZEROPAGE : 0);
-
-	/*
-	 * At this point, folio should contain the specified page.
-	 * For uniform split, it is left for caller to unlock.
-	 * For buddy allocator like split, the first after-split folio is left
-	 * for caller to unlock.
-	 */
-	for (new_folio = origin_folio; new_folio != next_folio; new_folio = next) {
-		next = folio_next(new_folio);
-		if (new_folio == page_folio(lock_at))
-			continue;
-
-		folio_unlock(new_folio);
-		/*
-		 * Subpages may be freed if there wasn't any mapping
-		 * like if add_to_swap() is running on a lru page that
-		 * had its mapping zapped. And freeing these pages
-		 * requires taking the lru_lock so we do the put_page
-		 * of the tail pages after the split is complete.
-		 */
-		free_folio_and_swap_cache(new_folio);
-	}
 	return ret;
 }
 
@@ -3706,10 +3594,13 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
 {
 	struct deferred_split *ds_queue = get_deferred_split_queue(folio);
 	XA_STATE(xas, &folio->mapping->i_pages, folio->index);
+	struct folio *next_folio = folio_next(folio);
 	bool is_anon = folio_test_anon(folio);
 	struct address_space *mapping = NULL;
 	struct anon_vma *anon_vma = NULL;
 	int order = folio_order(folio);
+	struct folio *new_folio, *next;
+	int nr_shmem_dropped = 0;
 	int extra_pins, ret;
 	pgoff_t end;
 	bool is_hzp;
@@ -3833,13 +3724,18 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
 		 */
 		xas_lock(&xas);
 		xas_reset(&xas);
-		if (xas_load(&xas) != folio)
+		if (xas_load(&xas) != folio) {
+			ret = -EAGAIN;
 			goto fail;
+		}
 	}
 
 	/* Prevent deferred_split_scan() touching ->_refcount */
 	spin_lock(&ds_queue->split_queue_lock);
 	if (folio_ref_freeze(folio, 1 + extra_pins)) {
+		struct address_space *swap_cache = NULL;
+		struct lruvec *lruvec;
+
 		if (folio_order(folio) > 1 &&
 		    !list_empty(&folio->_deferred_list)) {
 			ds_queue->split_queue_len--;
@@ -3873,18 +3769,119 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
 			}
 		}
 
-		ret = __split_unmapped_folio(folio, new_order,
-				split_at, lock_at, list, end, &xas, mapping,
-				uniform_split);
+		if (folio_test_swapcache(folio)) {
+			if (mapping) {
+				VM_WARN_ON_ONCE_FOLIO(mapping, folio);
+				ret = -EINVAL;
+				goto fail;
+			}
+
+			/*
+			 * a swapcache folio can only be uniformly split to
+			 * order-0
+			 */
+			if (!uniform_split || new_order != 0) {
+				ret = -EINVAL;
+				goto fail;
+			}
+
+			swap_cache = swap_address_space(folio->swap);
+			xa_lock(&swap_cache->i_pages);
+		}
+
+		/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
+		lruvec = folio_lruvec_lock(folio);
+
+		ret = __split_unmapped_folio(folio, new_order, split_at, &xas,
+					     mapping, uniform_split);
+
+		/*
+		 * Unfreeze after-split folios and put them back to the right
+		 * list. @folio should be kept frozon until page cache entries
+		 * are updated with all the other after-split folios to prevent
+		 * others seeing stale page cache entries.
+		 */
+		for (new_folio = folio_next(folio); new_folio != next_folio;
+		     new_folio = next) {
+			next = folio_next(new_folio);
+
+			folio_ref_unfreeze(
+				new_folio,
+				1 + ((mapping || swap_cache) ?
+					     folio_nr_pages(new_folio) :
+					     0));
+
+			lru_add_split_folio(folio, new_folio, lruvec, list);
+
+			/* Some pages can be beyond EOF: drop them from cache */
+			if (new_folio->index >= end) {
+				if (shmem_mapping(mapping))
+					nr_shmem_dropped += folio_nr_pages(new_folio);
+				else if (folio_test_clear_dirty(new_folio))
+					folio_account_cleaned(
+						new_folio,
+						inode_to_wb(mapping->host));
+				__filemap_remove_folio(new_folio, NULL);
+				folio_put_refs(new_folio,
+					       folio_nr_pages(new_folio));
+			} else if (mapping) {
+				__xa_store(&mapping->i_pages, new_folio->index,
+					   new_folio, 0);
+			} else if (swap_cache) {
+				__xa_store(&swap_cache->i_pages,
+					   swap_cache_index(new_folio->swap),
+					   new_folio, 0);
+			}
+		}
+		/*
+		 * Unfreeze @folio only after all page cache entries, which
+		 * used to point to it, have been updated with new folios.
+		 * Otherwise, a parallel folio_try_get() can grab origin_folio
+		 * and its caller can see stale page cache entries.
+		 */
+		folio_ref_unfreeze(folio, 1 +
+			((mapping || swap_cache) ? folio_nr_pages(folio) : 0));
+
+		unlock_page_lruvec(lruvec);
+
+		if (swap_cache)
+			xa_unlock(&swap_cache->i_pages);
 	} else {
 		spin_unlock(&ds_queue->split_queue_lock);
-fail:
-		if (mapping)
-			xas_unlock(&xas);
-		local_irq_enable();
-		remap_page(folio, folio_nr_pages(folio), 0);
 		ret = -EAGAIN;
 	}
+fail:
+	if (mapping)
+		xas_unlock(&xas);
+
+	local_irq_enable();
+
+	if (nr_shmem_dropped)
+		shmem_uncharge(mapping->host, nr_shmem_dropped);
+
+	remap_page(folio, 1 << order,
+		   !ret && folio_test_anon(folio) ? RMP_USE_SHARED_ZEROPAGE :
+						    0);
+
+	/*
+	 * Unlock all after-split folios except the one containing @lock_at
+	 * page. If @folio is not split, it will be kept locked.
+	 */
+	for (new_folio = folio; new_folio != next_folio; new_folio = next) {
+		next = folio_next(new_folio);
+		if (new_folio == page_folio(lock_at))
+			continue;
+
+		folio_unlock(new_folio);
+		/*
+		 * Subpages may be freed if there wasn't any mapping
+		 * like if add_to_swap() is running on a lru page that
+		 * had its mapping zapped. And freeing these pages
+		 * requires taking the lru_lock so we do the put_page
+		 * of the tail pages after the split is complete.
+		 */
+		free_folio_and_swap_cache(new_folio);
+	}
 
 out_unlock:
 	if (anon_vma) {
-- 
2.47.2

On Mon, Jul 14, 2025 at 01:18:22PM -0400, Zi Yan wrote:
> remap(), folio_ref_unfreeze(), lru_add_split_folio() are not relevant to
> splitting unmapped folio operations. Move them out to the caller so that
> __split_unmapped_folio() only handles unmapped folio splits. This makes
> __split_unmapped_folio() reusable.

Nit but maybe worth mentioning the various renames etc.

>
> Convert VM_BUG_ON(mapping) to use VM_WARN_ON_ONCE_FOLIO().
>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> Acked-by: Balbir Singh <balbirs@nvidia.com>

After a lot of staring, 2 difftastic's at once and exactly 0 coverity
instances, I've convinced myself this looks right.

I think you really should have split this up into smaller patches, as this
is moving stuff around and changing stuff all at once with a lot of
complexity and moving parts.

However not going to make you do that, since you got acks and I don't want
to hold this up.

I have a few nits + queries below that need addressing however, see below.

> ---
>  mm/huge_memory.c | 291 +++++++++++++++++++++++------------------------
>  1 file changed, 144 insertions(+), 147 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 3eb1c34be601..a7ee731f974f 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3396,10 +3396,6 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>   *             order - 1 to new_order).
>   * @split_at: in buddy allocator like split, the folio containing @split_at
>   *            will be split until its order becomes @new_order.
> - * @lock_at: the folio containing @lock_at is left locked for caller.
> - * @list: the after split folios will be added to @list if it is not NULL,
> - *        otherwise to LRU lists.
> - * @end: the end of the file @folio maps to. -1 if @folio is anonymous memory.
>   * @xas: xa_state pointing to folio->mapping->i_pages and locked by caller
>   * @mapping: @folio->mapping
>   * @uniform_split: if the split is uniform or not (buddy allocator like split)
> @@ -3425,52 +3421,26 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>   *    @page, which is split in next for loop.
>   *
>   * After splitting, the caller's folio reference will be transferred to the
> - * folio containing @page. The other folios may be freed if they are not mapped.
> - *
> - * In terms of locking, after splitting,
> - * 1. uniform split leaves @page (or the folio contains it) locked;
> - * 2. buddy allocator like (non-uniform) split leaves @folio locked.

Are these no longer relevant? Shouldn't we retain this, or move it
elsewhere if appropriate?

> - *
> + * folio containing @page. The caller needs to unlock and/or free after-split
> + * folios if necessary.
>   *
>   * For !uniform_split, when -ENOMEM is returned, the original folio might be
>   * split. The caller needs to check the input folio.
>   */
>  static int __split_unmapped_folio(struct folio *folio, int new_order,
> -		struct page *split_at, struct page *lock_at,
> -		struct list_head *list, pgoff_t end,
> -		struct xa_state *xas, struct address_space *mapping,
> -		bool uniform_split)
> +		struct page *split_at, struct xa_state *xas,
> +		struct address_space *mapping, bool uniform_split)
>  {
> -	struct lruvec *lruvec;
> -	struct address_space *swap_cache = NULL;
> -	struct folio *origin_folio = folio;
> -	struct folio *next_folio = folio_next(folio);
> -	struct folio *new_folio;
>  	struct folio *next;
>  	int order = folio_order(folio);
>  	int split_order;
>  	int start_order = uniform_split ? new_order : order - 1;
> -	int nr_dropped = 0;
>  	int ret = 0;
>  	bool stop_split = false;
>
> -	if (folio_test_swapcache(folio)) {
> -		VM_BUG_ON(mapping);

Good to get rid of this.

> -
> -		/* a swapcache folio can only be uniformly split to order-0 */
> -		if (!uniform_split || new_order != 0)
> -			return -EINVAL;
> -
> -		swap_cache = swap_address_space(folio->swap);
> -		xa_lock(&swap_cache->i_pages);
> -	}
> -
>  	if (folio_test_anon(folio))
>  		mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
>
> -	/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
> -	lruvec = folio_lruvec_lock(folio);
> -
>  	folio_clear_has_hwpoisoned(folio);
>
>  	/*
> @@ -3480,9 +3450,9 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>  	for (split_order = start_order;
>  	     split_order >= new_order && !stop_split;
>  	     split_order--) {
> -		int old_order = folio_order(folio);
> -		struct folio *release;
>  		struct folio *end_folio = folio_next(folio);
> +		int old_order = folio_order(folio);
> +		struct folio *new_folio;
>
>  		/* order-1 anonymous folio is not supported */
>  		if (folio_test_anon(folio) && split_order == 1)
> @@ -3504,126 +3474,44 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>  				if (xas_error(xas)) {
>  					ret = xas_error(xas);
>  					stop_split = true;
> -					goto after_split;
>  				}
>  			}
>  		}
>
> -		folio_split_memcg_refs(folio, old_order, split_order);
> -		split_page_owner(&folio->page, old_order, split_order);
> -		pgalloc_tag_split(folio, old_order, split_order);
> -
> -		__split_folio_to_order(folio, old_order, split_order);
> +		if (!stop_split) {
> +			folio_split_memcg_refs(folio, old_order, split_order);
> +			split_page_owner(&folio->page, old_order, split_order);
> +			pgalloc_tag_split(folio, old_order, split_order);
>
> -after_split:
> +			__split_folio_to_order(folio, old_order, split_order);
> +		}
>  		/*
> -		 * Iterate through after-split folios and perform related
> -		 * operations. But in buddy allocator like split, the folio
> +		 * Iterate through after-split folios and update folio stats.

Good to spell out what the 'related operations' are :) Of course you're
changing this so this loop does some and the other loop does the post-split
rest.

> +		 * But in buddy allocator like split, the folio
>  		 * containing the specified page is skipped until its order
>  		 * is new_order, since the folio will be worked on in next
>  		 * iteration.
>  		 */
> -		for (release = folio; release != end_folio; release = next) {
> -			next = folio_next(release);
> +		for (new_folio = folio; new_folio != end_folio; new_folio = next) {
> +			next = folio_next(new_folio);
>  			/*
> -			 * for buddy allocator like split, the folio containing
> -			 * page will be split next and should not be released,
> -			 * until the folio's order is new_order or stop_split
> -			 * is set to true by the above xas_split() failure.
> +			 * for buddy allocator like split, new_folio containing
> +			 * page could be split again, thus do not change stats
> +			 * yet. Wait until new_folio's order is new_order or
> +			 * stop_split is set to true by the above xas_split()
> +			 * failure.
>  			 */
> -			if (release == page_folio(split_at)) {
> -				folio = release;
> +			if (new_folio == page_folio(split_at)) {
> +				folio = new_folio;
>  				if (split_order != new_order && !stop_split)
>  					continue;
>  			}
> -			if (folio_test_anon(release)) {
> -				mod_mthp_stat(folio_order(release),
> -						MTHP_STAT_NR_ANON, 1);
> -			}
> -
> -			/*
> -			 * origin_folio should be kept frozon until page cache
> -			 * entries are updated with all the other after-split
> -			 * folios to prevent others seeing stale page cache
> -			 * entries.
> -			 */
> -			if (release == origin_folio)
> -				continue;
> -
> -			folio_ref_unfreeze(release, 1 +
> -					((mapping || swap_cache) ?
> -						folio_nr_pages(release) : 0));
> -
> -			lru_add_split_folio(origin_folio, release, lruvec,
> -					list);
> -
> -			/* Some pages can be beyond EOF: drop them from cache */
> -			if (release->index >= end) {
> -				if (shmem_mapping(mapping))
> -					nr_dropped += folio_nr_pages(release);
> -				else if (folio_test_clear_dirty(release))
> -					folio_account_cleaned(release,
> -						inode_to_wb(mapping->host));
> -				__filemap_remove_folio(release, NULL);
> -				folio_put_refs(release, folio_nr_pages(release));
> -			} else if (mapping) {
> -				__xa_store(&mapping->i_pages,
> -						release->index, release, 0);
> -			} else if (swap_cache) {
> -				__xa_store(&swap_cache->i_pages,
> -						swap_cache_index(release->swap),
> -						release, 0);
> -			}
> +			if (folio_test_anon(new_folio))
> +				mod_mthp_stat(folio_order(new_folio),
> +					      MTHP_STAT_NR_ANON, 1);
>  		}
>  	}
>
> -	/*
> -	 * Unfreeze origin_folio only after all page cache entries, which used
> -	 * to point to it, have been updated with new folios. Otherwise,
> -	 * a parallel folio_try_get() can grab origin_folio and its caller can
> -	 * see stale page cache entries.
> -	 */
> -	folio_ref_unfreeze(origin_folio, 1 +
> -		((mapping || swap_cache) ? folio_nr_pages(origin_folio) : 0));
> -
> -	unlock_page_lruvec(lruvec);
> -
> -	if (swap_cache)
> -		xa_unlock(&swap_cache->i_pages);
> -	if (mapping)
> -		xa_unlock(&mapping->i_pages);
> -
> -	/* Caller disabled irqs, so they are still disabled here */
> -	local_irq_enable();
> -
> -	if (nr_dropped)
> -		shmem_uncharge(mapping->host, nr_dropped);
> -
> -	remap_page(origin_folio, 1 << order,
> -			folio_test_anon(origin_folio) ?
> -				RMP_USE_SHARED_ZEROPAGE : 0);
> -
> -	/*
> -	 * At this point, folio should contain the specified page.
> -	 * For uniform split, it is left for caller to unlock.
> -	 * For buddy allocator like split, the first after-split folio is left
> -	 * for caller to unlock.
> -	 */
> -	for (new_folio = origin_folio; new_folio != next_folio; new_folio = next) {
> -		next = folio_next(new_folio);
> -		if (new_folio == page_folio(lock_at))
> -			continue;
> -
> -		folio_unlock(new_folio);
> -		/*
> -		 * Subpages may be freed if there wasn't any mapping
> -		 * like if add_to_swap() is running on a lru page that
> -		 * had its mapping zapped. And freeing these pages
> -		 * requires taking the lru_lock so we do the put_page
> -		 * of the tail pages after the split is complete.
> -		 */
> -		free_folio_and_swap_cache(new_folio);
> -	}
>  	return ret;
>  }
>
> @@ -3706,10 +3594,13 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  {
>  	struct deferred_split *ds_queue = get_deferred_split_queue(folio);
>  	XA_STATE(xas, &folio->mapping->i_pages, folio->index);
> +	struct folio *next_folio = folio_next(folio);
>  	bool is_anon = folio_test_anon(folio);
>  	struct address_space *mapping = NULL;
>  	struct anon_vma *anon_vma = NULL;
>  	int order = folio_order(folio);
> +	struct folio *new_folio, *next;
> +	int nr_shmem_dropped = 0;
>  	int extra_pins, ret;
>  	pgoff_t end;
>  	bool is_hzp;

There's some VM_BUG_ON_FOLIO()'s in the code:

	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);

That should probably be VM_WARN_ON() or VM_WARN_ON_ONCE(), maybe worth
changing here too?

> @@ -3833,13 +3724,18 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  		 */
>  		xas_lock(&xas);
>  		xas_reset(&xas);
> -		if (xas_load(&xas) != folio)
> +		if (xas_load(&xas) != folio) {
> +			ret = -EAGAIN;

It is beyond words that the original logic manually set ret == -EAGAIN...

And this is the only place we 'goto fail'.

Yikes this code is a horror show.


>  			goto fail;
> +		}
>  	}
>
>  	/* Prevent deferred_split_scan() touching ->_refcount */
>  	spin_lock(&ds_queue->split_queue_lock);
>  	if (folio_ref_freeze(folio, 1 + extra_pins)) {
> +		struct address_space *swap_cache = NULL;
> +		struct lruvec *lruvec;
> +
>  		if (folio_order(folio) > 1 &&
>  		    !list_empty(&folio->_deferred_list)) {
>  			ds_queue->split_queue_len--;
> @@ -3873,18 +3769,119 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>  			}
>  		}
>
> -		ret = __split_unmapped_folio(folio, new_order,
> -				split_at, lock_at, list, end, &xas, mapping,
> -				uniform_split);
> +		if (folio_test_swapcache(folio)) {
> +			if (mapping) {
> +				VM_WARN_ON_ONCE_FOLIO(mapping, folio);
> +				ret = -EINVAL;
> +				goto fail;

It's a new code path (in prod we'd just carry on, or in debug we would
haven oops'd), but I think valid.

I wonder if this is almost over-cautious, as this would require a non-anon
folio to be in the swap-cache (since the is_anon path will set mapping
NUL).

But at the same time, probably worth keeping in at least for now.

> +			}
> +
> +			/*
> +			 * a swapcache folio can only be uniformly split to
> +			 * order-0
> +			 */
> +			if (!uniform_split || new_order != 0) {
> +				ret = -EINVAL;
> +				goto fail;
> +			}
> +
> +			swap_cache = swap_address_space(folio->swap);
> +			xa_lock(&swap_cache->i_pages);
> +		}
> +
> +		/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
> +		lruvec = folio_lruvec_lock(folio);
> +
> +		ret = __split_unmapped_folio(folio, new_order, split_at, &xas,
> +					     mapping, uniform_split);
> +
> +		/*
> +		 * Unfreeze after-split folios and put them back to the right
> +		 * list. @folio should be kept frozon until page cache entries
> +		 * are updated with all the other after-split folios to prevent
> +		 * others seeing stale page cache entries.
> +		 */
> +		for (new_folio = folio_next(folio); new_folio != next_folio;
> +		     new_folio = next) {

Hm now we have 'next' and 'next_folio', this is quite confusing.

Seems to me new_folio should be end_folio no, like the original? And maybe
then rename next to next_folio? As it is kinda inconsistent that it isn't
suffixed with _folio anyway.

> +			next = folio_next(new_folio);
> +

We're no longer doing what would here be new_folio == origin_folio
(previously, release == origin_folio).

Is this correct? Why do we no longer ned to do this?

Is it because __split_unmapped_folio() will somehow take care of this in
advance/render this meaningless?

This definitely needs to be mentioned in the commit message.

> +			folio_ref_unfreeze(
> +				new_folio,
> +				1 + ((mapping || swap_cache) ?
> +					     folio_nr_pages(new_folio) :
> +					     0));

Again, be nice to separate this out, but I think in a follow-up not here.

> +
> +			lru_add_split_folio(folio, new_folio, lruvec, list);
> +
> +			/* Some pages can be beyond EOF: drop them from cache */
> +			if (new_folio->index >= end) {
> +				if (shmem_mapping(mapping))
> +					nr_shmem_dropped += folio_nr_pages(new_folio);
> +				else if (folio_test_clear_dirty(new_folio))
> +					folio_account_cleaned(
> +						new_folio,
> +						inode_to_wb(mapping->host));
> +				__filemap_remove_folio(new_folio, NULL);
> +				folio_put_refs(new_folio,
> +					       folio_nr_pages(new_folio));
> +			} else if (mapping) {
> +				__xa_store(&mapping->i_pages, new_folio->index,
> +					   new_folio, 0);
> +			} else if (swap_cache) {
> +				__xa_store(&swap_cache->i_pages,
> +					   swap_cache_index(new_folio->swap),
> +					   new_folio, 0);
> +			}
> +		}
> +		/*
> +		 * Unfreeze @folio only after all page cache entries, which
> +		 * used to point to it, have been updated with new folios.
> +		 * Otherwise, a parallel folio_try_get() can grab origin_folio
> +		 * and its caller can see stale page cache entries.
> +		 */
> +		folio_ref_unfreeze(folio, 1 +
> +			((mapping || swap_cache) ? folio_nr_pages(folio) : 0));

This line is horrid, probably one for a future series but this sort of
calculation of what the number of refs should be post-freeze should clearly
be separated out or at least made abundantly clear in an open-coded
implementation.

> +
> +		unlock_page_lruvec(lruvec);
> +
> +		if (swap_cache)
> +			xa_unlock(&swap_cache->i_pages);
>  	} else {
>  		spin_unlock(&ds_queue->split_queue_lock);
> -fail:
> -		if (mapping)
> -			xas_unlock(&xas);
> -		local_irq_enable();
> -		remap_page(folio, folio_nr_pages(folio), 0);
>  		ret = -EAGAIN;
>  	}
> +fail:
> +	if (mapping)
> +		xas_unlock(&xas);
> +
> +	local_irq_enable();
> +
> +	if (nr_shmem_dropped)
> +		shmem_uncharge(mapping->host, nr_shmem_dropped);
> +
> +	remap_page(folio, 1 << order,
> +		   !ret && folio_test_anon(folio) ? RMP_USE_SHARED_ZEROPAGE :
> +						    0);

I really don't like this !ret but here, this isn't very readable.

Something like:

	int flags;

	...

	if (!ret && folio_test_anon(folio))
		flags = RMP_USE_SHARED_ZERO_PAGE;
	remap_page(folio, 1 << order, flags);

Would be better.

But really this is all screaming out to be separated into parts of
course. But that's one for a follow-up series...

> +
> +	/*
> +	 * Unlock all after-split folios except the one containing @lock_at
> +	 * page. If @folio is not split, it will be kept locked.
> +	 */
> +	for (new_folio = folio; new_folio != next_folio; new_folio = next) {
> +		next = folio_next(new_folio);
> +		if (new_folio == page_folio(lock_at))
> +			continue;
> +
> +		folio_unlock(new_folio);
> +		/*
> +		 * Subpages may be freed if there wasn't any mapping
> +		 * like if add_to_swap() is running on a lru page that
> +		 * had its mapping zapped. And freeing these pages
> +		 * requires taking the lru_lock so we do the put_page
> +		 * of the tail pages after the split is complete.
> +		 */
> +		free_folio_and_swap_cache(new_folio);
> +	}
>
>  out_unlock:
>  	if (anon_vma) {
> --
> 2.47.2
>

Generally I see why you're not using origin_folio any more since you can
just use folio everywhere, but I wonder if this makes things more
confusing.

On the other hand, this function is already hugely confusing so maybe not a
big deal and can be addressed in follow ups...

On 17 Jul 2025, at 10:07, Lorenzo Stoakes wrote:

> On Mon, Jul 14, 2025 at 01:18:22PM -0400, Zi Yan wrote:
>> remap(), folio_ref_unfreeze(), lru_add_split_folio() are not relevant to
>> splitting unmapped folio operations. Move them out to the caller so that
>> __split_unmapped_folio() only handles unmapped folio splits. This makes
>> __split_unmapped_folio() reusable.
>
> Nit but maybe worth mentioning the various renames etc.

You mean release -> new_folio, origin_folio is replaced by folio?
Sure, I can do that.

>
>>
>> Convert VM_BUG_ON(mapping) to use VM_WARN_ON_ONCE_FOLIO().
>>
>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>> Acked-by: Balbir Singh <balbirs@nvidia.com>
>
> After a lot of staring, 2 difftastic's at once and exactly 0 coverity
> instances, I've convinced myself this looks right.
>
> I think you really should have split this up into smaller patches, as this
> is moving stuff around and changing stuff all at once with a lot of
> complexity and moving parts.
>
> However not going to make you do that, since you got acks and I don't want
> to hold this up.
>
> I have a few nits + queries below that need addressing however, see below.

Since I need to address these nits, I might just split this up.
How about:

1.

>
>> ---
>>  mm/huge_memory.c | 291 +++++++++++++++++++++++------------------------
>>  1 file changed, 144 insertions(+), 147 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 3eb1c34be601..a7ee731f974f 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3396,10 +3396,6 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>   *             order - 1 to new_order).
>>   * @split_at: in buddy allocator like split, the folio containing @split_at
>>   *            will be split until its order becomes @new_order.
>> - * @lock_at: the folio containing @lock_at is left locked for caller.
>> - * @list: the after split folios will be added to @list if it is not NULL,
>> - *        otherwise to LRU lists.
>> - * @end: the end of the file @folio maps to. -1 if @folio is anonymous memory.
>>   * @xas: xa_state pointing to folio->mapping->i_pages and locked by caller
>>   * @mapping: @folio->mapping
>>   * @uniform_split: if the split is uniform or not (buddy allocator like split)
>> @@ -3425,52 +3421,26 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>   *    @page, which is split in next for loop.
>>   *
>>   * After splitting, the caller's folio reference will be transferred to the
>> - * folio containing @page. The other folios may be freed if they are not mapped.
>> - *
>> - * In terms of locking, after splitting,
>> - * 1. uniform split leaves @page (or the folio contains it) locked;
>> - * 2. buddy allocator like (non-uniform) split leaves @folio locked.
>
> Are these no longer relevant? Shouldn't we retain this, or move it
> elsewhere if appropriate?

With lock_at parameter, people can get this from the __folio_split()
call sites. But a comment is better than none. I will move it to
__folio_split()’s comment area.

>
>> - *
>> + * folio containing @page. The caller needs to unlock and/or free after-split
>> + * folios if necessary.
>>   *
>>   * For !uniform_split, when -ENOMEM is returned, the original folio might be
>>   * split. The caller needs to check the input folio.
>>   */
>>  static int __split_unmapped_folio(struct folio *folio, int new_order,
>> -		struct page *split_at, struct page *lock_at,
>> -		struct list_head *list, pgoff_t end,
>> -		struct xa_state *xas, struct address_space *mapping,
>> -		bool uniform_split)
>> +		struct page *split_at, struct xa_state *xas,
>> +		struct address_space *mapping, bool uniform_split)
>>  {
>> -	struct lruvec *lruvec;
>> -	struct address_space *swap_cache = NULL;
>> -	struct folio *origin_folio = folio;
>> -	struct folio *next_folio = folio_next(folio);
>> -	struct folio *new_folio;
>>  	struct folio *next;
>>  	int order = folio_order(folio);
>>  	int split_order;
>>  	int start_order = uniform_split ? new_order : order - 1;
>> -	int nr_dropped = 0;
>>  	int ret = 0;
>>  	bool stop_split = false;
>>
>> -	if (folio_test_swapcache(folio)) {
>> -		VM_BUG_ON(mapping);
>
> Good to get rid of this.
>
>> -
>> -		/* a swapcache folio can only be uniformly split to order-0 */
>> -		if (!uniform_split || new_order != 0)
>> -			return -EINVAL;
>> -
>> -		swap_cache = swap_address_space(folio->swap);
>> -		xa_lock(&swap_cache->i_pages);
>> -	}
>> -
>>  	if (folio_test_anon(folio))
>>  		mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
>>
>> -	/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
>> -	lruvec = folio_lruvec_lock(folio);
>> -
>>  	folio_clear_has_hwpoisoned(folio);
>>
>>  	/*
>> @@ -3480,9 +3450,9 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>  	for (split_order = start_order;
>>  	     split_order >= new_order && !stop_split;
>>  	     split_order--) {
>> -		int old_order = folio_order(folio);
>> -		struct folio *release;
>>  		struct folio *end_folio = folio_next(folio);
>> +		int old_order = folio_order(folio);
>> +		struct folio *new_folio;
>>
>>  		/* order-1 anonymous folio is not supported */
>>  		if (folio_test_anon(folio) && split_order == 1)
>> @@ -3504,126 +3474,44 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>  				if (xas_error(xas)) {
>>  					ret = xas_error(xas);
>>  					stop_split = true;
>> -					goto after_split;
>>  				}
>>  			}
>>  		}
>>
>> -		folio_split_memcg_refs(folio, old_order, split_order);
>> -		split_page_owner(&folio->page, old_order, split_order);
>> -		pgalloc_tag_split(folio, old_order, split_order);
>> -
>> -		__split_folio_to_order(folio, old_order, split_order);
>> +		if (!stop_split) {
>> +			folio_split_memcg_refs(folio, old_order, split_order);
>> +			split_page_owner(&folio->page, old_order, split_order);
>> +			pgalloc_tag_split(folio, old_order, split_order);
>>
>> -after_split:
>> +			__split_folio_to_order(folio, old_order, split_order);
>> +		}
>>  		/*
>> -		 * Iterate through after-split folios and perform related
>> -		 * operations. But in buddy allocator like split, the folio
>> +		 * Iterate through after-split folios and update folio stats.
>
> Good to spell out what the 'related operations' are :) Of course you're
> changing this so this loop does some and the other loop does the post-split
> rest.
>
>> +		 * But in buddy allocator like split, the folio
>>  		 * containing the specified page is skipped until its order
>>  		 * is new_order, since the folio will be worked on in next
>>  		 * iteration.
>>  		 */
>> -		for (release = folio; release != end_folio; release = next) {
>> -			next = folio_next(release);
>> +		for (new_folio = folio; new_folio != end_folio; new_folio = next) {
>> +			next = folio_next(new_folio);
>>  			/*
>> -			 * for buddy allocator like split, the folio containing
>> -			 * page will be split next and should not be released,
>> -			 * until the folio's order is new_order or stop_split
>> -			 * is set to true by the above xas_split() failure.
>> +			 * for buddy allocator like split, new_folio containing
>> +			 * page could be split again, thus do not change stats
>> +			 * yet. Wait until new_folio's order is new_order or
>> +			 * stop_split is set to true by the above xas_split()
>> +			 * failure.
>>  			 */
>> -			if (release == page_folio(split_at)) {
>> -				folio = release;
>> +			if (new_folio == page_folio(split_at)) {
>> +				folio = new_folio;
>>  				if (split_order != new_order && !stop_split)
>>  					continue;
>>  			}
>> -			if (folio_test_anon(release)) {
>> -				mod_mthp_stat(folio_order(release),
>> -						MTHP_STAT_NR_ANON, 1);
>> -			}
>> -
>> -			/*
>> -			 * origin_folio should be kept frozon until page cache
>> -			 * entries are updated with all the other after-split
>> -			 * folios to prevent others seeing stale page cache
>> -			 * entries.
>> -			 */
>> -			if (release == origin_folio)
>> -				continue;
>> -
>> -			folio_ref_unfreeze(release, 1 +
>> -					((mapping || swap_cache) ?
>> -						folio_nr_pages(release) : 0));
>> -
>> -			lru_add_split_folio(origin_folio, release, lruvec,
>> -					list);
>> -
>> -			/* Some pages can be beyond EOF: drop them from cache */
>> -			if (release->index >= end) {
>> -				if (shmem_mapping(mapping))
>> -					nr_dropped += folio_nr_pages(release);
>> -				else if (folio_test_clear_dirty(release))
>> -					folio_account_cleaned(release,
>> -						inode_to_wb(mapping->host));
>> -				__filemap_remove_folio(release, NULL);
>> -				folio_put_refs(release, folio_nr_pages(release));
>> -			} else if (mapping) {
>> -				__xa_store(&mapping->i_pages,
>> -						release->index, release, 0);
>> -			} else if (swap_cache) {
>> -				__xa_store(&swap_cache->i_pages,
>> -						swap_cache_index(release->swap),
>> -						release, 0);
>> -			}
>> +			if (folio_test_anon(new_folio))
>> +				mod_mthp_stat(folio_order(new_folio),
>> +					      MTHP_STAT_NR_ANON, 1);
>>  		}
>>  	}
>>
>> -	/*
>> -	 * Unfreeze origin_folio only after all page cache entries, which used
>> -	 * to point to it, have been updated with new folios. Otherwise,
>> -	 * a parallel folio_try_get() can grab origin_folio and its caller can
>> -	 * see stale page cache entries.
>> -	 */
>> -	folio_ref_unfreeze(origin_folio, 1 +
>> -		((mapping || swap_cache) ? folio_nr_pages(origin_folio) : 0));
>> -
>> -	unlock_page_lruvec(lruvec);
>> -
>> -	if (swap_cache)
>> -		xa_unlock(&swap_cache->i_pages);
>> -	if (mapping)
>> -		xa_unlock(&mapping->i_pages);
>> -
>> -	/* Caller disabled irqs, so they are still disabled here */
>> -	local_irq_enable();
>> -
>> -	if (nr_dropped)
>> -		shmem_uncharge(mapping->host, nr_dropped);
>> -
>> -	remap_page(origin_folio, 1 << order,
>> -			folio_test_anon(origin_folio) ?
>> -				RMP_USE_SHARED_ZEROPAGE : 0);
>> -
>> -	/*
>> -	 * At this point, folio should contain the specified page.
>> -	 * For uniform split, it is left for caller to unlock.
>> -	 * For buddy allocator like split, the first after-split folio is left
>> -	 * for caller to unlock.
>> -	 */
>> -	for (new_folio = origin_folio; new_folio != next_folio; new_folio = next) {
>> -		next = folio_next(new_folio);
>> -		if (new_folio == page_folio(lock_at))
>> -			continue;
>> -
>> -		folio_unlock(new_folio);
>> -		/*
>> -		 * Subpages may be freed if there wasn't any mapping
>> -		 * like if add_to_swap() is running on a lru page that
>> -		 * had its mapping zapped. And freeing these pages
>> -		 * requires taking the lru_lock so we do the put_page
>> -		 * of the tail pages after the split is complete.
>> -		 */
>> -		free_folio_and_swap_cache(new_folio);
>> -	}
>>  	return ret;
>>  }
>>
>> @@ -3706,10 +3594,13 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>  {
>>  	struct deferred_split *ds_queue = get_deferred_split_queue(folio);
>>  	XA_STATE(xas, &folio->mapping->i_pages, folio->index);
>> +	struct folio *next_folio = folio_next(folio);
>>  	bool is_anon = folio_test_anon(folio);
>>  	struct address_space *mapping = NULL;
>>  	struct anon_vma *anon_vma = NULL;
>>  	int order = folio_order(folio);
>> +	struct folio *new_folio, *next;
>> +	int nr_shmem_dropped = 0;
>>  	int extra_pins, ret;
>>  	pgoff_t end;
>>  	bool is_hzp;
>
> There's some VM_BUG_ON_FOLIO()'s in the code:
>
> 	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> 	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
>
> That should probably be VM_WARN_ON() or VM_WARN_ON_ONCE(), maybe worth
> changing here too?

Sure. I can convert them in a separate patch. Basically:

if (!folio_test_locked(folio)) {
	VM_WARN_ON_ONCE_FOLIO(1, folio);
	return -EINVAL;
}

if (!folio_test_large(folio)) {
	VM_WARN_ON_ONCE_FOLIO(1, folio);
	return -EINVAL;
}

>
>> @@ -3833,13 +3724,18 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>  		 */
>>  		xas_lock(&xas);
>>  		xas_reset(&xas);
>> -		if (xas_load(&xas) != folio)
>> +		if (xas_load(&xas) != folio) {
>> +			ret = -EAGAIN;
>
> It is beyond words that the original logic manually set ret == -EAGAIN...
>
> And this is the only place we 'goto fail'.
>
> Yikes this code is a horror show.
>
>
>>  			goto fail;
>> +		}
>>  	}
>>
>>  	/* Prevent deferred_split_scan() touching ->_refcount */
>>  	spin_lock(&ds_queue->split_queue_lock);
>>  	if (folio_ref_freeze(folio, 1 + extra_pins)) {
>> +		struct address_space *swap_cache = NULL;
>> +		struct lruvec *lruvec;
>> +
>>  		if (folio_order(folio) > 1 &&
>>  		    !list_empty(&folio->_deferred_list)) {
>>  			ds_queue->split_queue_len--;
>> @@ -3873,18 +3769,119 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>  			}
>>  		}
>>
>> -		ret = __split_unmapped_folio(folio, new_order,
>> -				split_at, lock_at, list, end, &xas, mapping,
>> -				uniform_split);
>> +		if (folio_test_swapcache(folio)) {
>> +			if (mapping) {
>> +				VM_WARN_ON_ONCE_FOLIO(mapping, folio);
>> +				ret = -EINVAL;
>> +				goto fail;
>
> It's a new code path (in prod we'd just carry on, or in debug we would
> haven oops'd), but I think valid.
>
> I wonder if this is almost over-cautious, as this would require a non-anon
> folio to be in the swap-cache (since the is_anon path will set mapping
> NUL).
>
> But at the same time, probably worth keeping in at least for now.

Originally, it is a VM_BUG_ON(mapping). I am converting it to a warning.
I will move it to a separate patch to avoid confusion.

>
>> +			}
>> +
>> +			/*
>> +			 * a swapcache folio can only be uniformly split to
>> +			 * order-0
>> +			 */
>> +			if (!uniform_split || new_order != 0) {
>> +				ret = -EINVAL;
>> +				goto fail;
>> +			}
>> +
>> +			swap_cache = swap_address_space(folio->swap);
>> +			xa_lock(&swap_cache->i_pages);
>> +		}
>> +
>> +		/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
>> +		lruvec = folio_lruvec_lock(folio);
>> +
>> +		ret = __split_unmapped_folio(folio, new_order, split_at, &xas,
>> +					     mapping, uniform_split);
>> +
>> +		/*
>> +		 * Unfreeze after-split folios and put them back to the right
>> +		 * list. @folio should be kept frozon until page cache entries
>> +		 * are updated with all the other after-split folios to prevent
>> +		 * others seeing stale page cache entries.
>> +		 */
>> +		for (new_folio = folio_next(folio); new_folio != next_folio;
>> +		     new_folio = next) {
>
> Hm now we have 'next' and 'next_folio', this is quite confusing.
>
> Seems to me new_folio should be end_folio no, like the original? And maybe
> then rename next to next_folio? As it is kinda inconsistent that it isn't
> suffixed with _folio anyway.

Sure. Will do. next_folio was coming from __split_unmapped_folio() code,
I should have renamed it. Thanks for pointing it out.

>
>> +			next = folio_next(new_folio);
>> +
>
> We're no longer doing what would here be new_folio == origin_folio
> (previously, release == origin_folio).
>
> Is this correct? Why do we no longer ned to do this?
>
> Is it because __split_unmapped_folio() will somehow take care of this in
> advance/render this meaningless?
>
> This definitely needs to be mentioned in the commit message.

Because “new_folio = folio_next(folio)” in the for loop initialization
part. The @folio is skipped at the very beginning. I will add a comment
to highlight this, since the code change is too subtle.

>
>> +			folio_ref_unfreeze(
>> +				new_folio,
>> +				1 + ((mapping || swap_cache) ?
>> +					     folio_nr_pages(new_folio) :
>> +					     0));
>
> Again, be nice to separate this out, but I think in a follow-up not here.

OK.

>
>> +
>> +			lru_add_split_folio(folio, new_folio, lruvec, list);
>> +
>> +			/* Some pages can be beyond EOF: drop them from cache */
>> +			if (new_folio->index >= end) {
>> +				if (shmem_mapping(mapping))
>> +					nr_shmem_dropped += folio_nr_pages(new_folio);
>> +				else if (folio_test_clear_dirty(new_folio))
>> +					folio_account_cleaned(
>> +						new_folio,
>> +						inode_to_wb(mapping->host));
>> +				__filemap_remove_folio(new_folio, NULL);
>> +				folio_put_refs(new_folio,
>> +					       folio_nr_pages(new_folio));
>> +			} else if (mapping) {
>> +				__xa_store(&mapping->i_pages, new_folio->index,
>> +					   new_folio, 0);
>> +			} else if (swap_cache) {
>> +				__xa_store(&swap_cache->i_pages,
>> +					   swap_cache_index(new_folio->swap),
>> +					   new_folio, 0);
>> +			}
>> +		}
>> +		/*
>> +		 * Unfreeze @folio only after all page cache entries, which
>> +		 * used to point to it, have been updated with new folios.
>> +		 * Otherwise, a parallel folio_try_get() can grab origin_folio
>> +		 * and its caller can see stale page cache entries.
>> +		 */
>> +		folio_ref_unfreeze(folio, 1 +
>> +			((mapping || swap_cache) ? folio_nr_pages(folio) : 0));
>
> This line is horrid, probably one for a future series but this sort of
> calculation of what the number of refs should be post-freeze should clearly
> be separated out or at least made abundantly clear in an open-coded
> implementation.

It is addressed in patch 2. And you already noticed it. :)

>
>> +
>> +		unlock_page_lruvec(lruvec);
>> +
>> +		if (swap_cache)
>> +			xa_unlock(&swap_cache->i_pages);
>>  	} else {
>>  		spin_unlock(&ds_queue->split_queue_lock);
>> -fail:
>> -		if (mapping)
>> -			xas_unlock(&xas);
>> -		local_irq_enable();
>> -		remap_page(folio, folio_nr_pages(folio), 0);
>>  		ret = -EAGAIN;
>>  	}
>> +fail:
>> +	if (mapping)
>> +		xas_unlock(&xas);
>> +
>> +	local_irq_enable();
>> +
>> +	if (nr_shmem_dropped)
>> +		shmem_uncharge(mapping->host, nr_shmem_dropped);
>> +
>> +	remap_page(folio, 1 << order,
>> +		   !ret && folio_test_anon(folio) ? RMP_USE_SHARED_ZEROPAGE :
>> +						    0);
>
> I really don't like this !ret but here, this isn't very readable.
>
> Something like:
>
> 	int flags;
>
> 	...
>
> 	if (!ret && folio_test_anon(folio))
> 		flags = RMP_USE_SHARED_ZERO_PAGE;
> 	remap_page(folio, 1 << order, flags);
>
> Would be better.
>
> But really this is all screaming out to be separated into parts of
> course. But that's one for a follow-up series...

Sure. Will add another patch to address this.

>
>> +
>> +	/*
>> +	 * Unlock all after-split folios except the one containing @lock_at
>> +	 * page. If @folio is not split, it will be kept locked.
>> +	 */
>> +	for (new_folio = folio; new_folio != next_folio; new_folio = next) {
>> +		next = folio_next(new_folio);
>> +		if (new_folio == page_folio(lock_at))
>> +			continue;
>> +
>> +		folio_unlock(new_folio);
>> +		/*
>> +		 * Subpages may be freed if there wasn't any mapping
>> +		 * like if add_to_swap() is running on a lru page that
>> +		 * had its mapping zapped. And freeing these pages
>> +		 * requires taking the lru_lock so we do the put_page
>> +		 * of the tail pages after the split is complete.
>> +		 */
>> +		free_folio_and_swap_cache(new_folio);
>> +	}
>>
>>  out_unlock:
>>  	if (anon_vma) {
>> --
>> 2.47.2
>>
>
> Generally I see why you're not using origin_folio any more since you can
> just use folio everywhere, but I wonder if this makes things more
> confusing.
>
> On the other hand, this function is already hugely confusing so maybe not a
> big deal and can be addressed in follow ups...

Since it is causing confusion. I will use origin_folio in the moved code.

I will send V4 to address your comments and add
“mm/huge_memory: refactor after-split (page) cache code.” in.

Best Regards,
Yan, Zi

On Thu, Jul 17, 2025 at 11:41:01AM -0400, Zi Yan wrote:
> On 17 Jul 2025, at 10:07, Lorenzo Stoakes wrote:
>
> > On Mon, Jul 14, 2025 at 01:18:22PM -0400, Zi Yan wrote:
> >> remap(), folio_ref_unfreeze(), lru_add_split_folio() are not relevant to
> >> splitting unmapped folio operations. Move them out to the caller so that
> >> __split_unmapped_folio() only handles unmapped folio splits. This makes
> >> __split_unmapped_folio() reusable.
> >
> > Nit but maybe worth mentioning the various renames etc.
>
> You mean release -> new_folio, origin_folio is replaced by folio?
> Sure, I can do that.

Yeah that kind of thing, just basically briefly mention the other stuff you
did.

Thanks!

>
> >
> >>
> >> Convert VM_BUG_ON(mapping) to use VM_WARN_ON_ONCE_FOLIO().
> >>
> >> Signed-off-by: Zi Yan <ziy@nvidia.com>
> >> Acked-by: Balbir Singh <balbirs@nvidia.com>
> >
> > After a lot of staring, 2 difftastic's at once and exactly 0 coverity
> > instances, I've convinced myself this looks right.
> >
> > I think you really should have split this up into smaller patches, as this
> > is moving stuff around and changing stuff all at once with a lot of
> > complexity and moving parts.
> >
> > However not going to make you do that, since you got acks and I don't want
> > to hold this up.
> >
> > I have a few nits + queries below that need addressing however, see below.
>
> Since I need to address these nits, I might just split this up.
> How about:
>
> 1.

Missing some text? :P


> >> @@ -3706,10 +3594,13 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> >>  {
> >>  	struct deferred_split *ds_queue = get_deferred_split_queue(folio);
> >>  	XA_STATE(xas, &folio->mapping->i_pages, folio->index);
> >> +	struct folio *next_folio = folio_next(folio);
> >>  	bool is_anon = folio_test_anon(folio);
> >>  	struct address_space *mapping = NULL;
> >>  	struct anon_vma *anon_vma = NULL;
> >>  	int order = folio_order(folio);
> >> +	struct folio *new_folio, *next;
> >> +	int nr_shmem_dropped = 0;
> >>  	int extra_pins, ret;
> >>  	pgoff_t end;
> >>  	bool is_hzp;
> >
> > There's some VM_BUG_ON_FOLIO()'s in the code:
> >
> > 	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> > 	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
> >
> > That should probably be VM_WARN_ON() or VM_WARN_ON_ONCE(), maybe worth
> > changing here too?
>
> Sure. I can convert them in a separate patch. Basically:
>
> if (!folio_test_locked(folio)) {
> 	VM_WARN_ON_ONCE_FOLIO(1, folio);
> 	return -EINVAL;
> }
>
> if (!folio_test_large(folio)) {
> 	VM_WARN_ON_ONCE_FOLIO(1, folio);
> 	return -EINVAL;
> }

Sounds good thanks!

>
> >
> >> @@ -3833,13 +3724,18 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> >>  		 */
> >>  		xas_lock(&xas);
> >>  		xas_reset(&xas);
> >> -		if (xas_load(&xas) != folio)
> >> +		if (xas_load(&xas) != folio) {
> >> +			ret = -EAGAIN;
> >
> > It is beyond words that the original logic manually set ret == -EAGAIN...
> >
> > And this is the only place we 'goto fail'.
> >
> > Yikes this code is a horror show.
> >
> >
> >>  			goto fail;
> >> +		}
> >>  	}
> >>
> >>  	/* Prevent deferred_split_scan() touching ->_refcount */
> >>  	spin_lock(&ds_queue->split_queue_lock);
> >>  	if (folio_ref_freeze(folio, 1 + extra_pins)) {
> >> +		struct address_space *swap_cache = NULL;
> >> +		struct lruvec *lruvec;
> >> +
> >>  		if (folio_order(folio) > 1 &&
> >>  		    !list_empty(&folio->_deferred_list)) {
> >>  			ds_queue->split_queue_len--;
> >> @@ -3873,18 +3769,119 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> >>  			}
> >>  		}
> >>
> >> -		ret = __split_unmapped_folio(folio, new_order,
> >> -				split_at, lock_at, list, end, &xas, mapping,
> >> -				uniform_split);
> >> +		if (folio_test_swapcache(folio)) {
> >> +			if (mapping) {
> >> +				VM_WARN_ON_ONCE_FOLIO(mapping, folio);
> >> +				ret = -EINVAL;
> >> +				goto fail;
> >
> > It's a new code path (in prod we'd just carry on, or in debug we would
> > haven oops'd), but I think valid.
> >
> > I wonder if this is almost over-cautious, as this would require a non-anon
> > folio to be in the swap-cache (since the is_anon path will set mapping
> > NUL).
> >
> > But at the same time, probably worth keeping in at least for now.
>
> Originally, it is a VM_BUG_ON(mapping). I am converting it to a warning.
> I will move it to a separate patch to avoid confusion.

Thanks

>
> >
> >> +			}
> >> +
> >> +			/*
> >> +			 * a swapcache folio can only be uniformly split to
> >> +			 * order-0
> >> +			 */
> >> +			if (!uniform_split || new_order != 0) {
> >> +				ret = -EINVAL;
> >> +				goto fail;
> >> +			}
> >> +
> >> +			swap_cache = swap_address_space(folio->swap);
> >> +			xa_lock(&swap_cache->i_pages);
> >> +		}
> >> +
> >> +		/* lock lru list/PageCompound, ref frozen by page_ref_freeze */
> >> +		lruvec = folio_lruvec_lock(folio);
> >> +
> >> +		ret = __split_unmapped_folio(folio, new_order, split_at, &xas,
> >> +					     mapping, uniform_split);
> >> +
> >> +		/*
> >> +		 * Unfreeze after-split folios and put them back to the right
> >> +		 * list. @folio should be kept frozon until page cache entries
> >> +		 * are updated with all the other after-split folios to prevent
> >> +		 * others seeing stale page cache entries.
> >> +		 */
> >> +		for (new_folio = folio_next(folio); new_folio != next_folio;
> >> +		     new_folio = next) {
> >
> > Hm now we have 'next' and 'next_folio', this is quite confusing.
> >
> > Seems to me new_folio should be end_folio no, like the original? And maybe
> > then rename next to next_folio? As it is kinda inconsistent that it isn't
> > suffixed with _folio anyway.
>
> Sure. Will do. next_folio was coming from __split_unmapped_folio() code,
> I should have renamed it. Thanks for pointing it out.

Thanks, yeah this was existing cofusion and not your fault, but good to
make it easier to understand.

>
> >
> >> +			next = folio_next(new_folio);
> >> +
> >
> > We're no longer doing what would here be new_folio == origin_folio
> > (previously, release == origin_folio).
> >
> > Is this correct? Why do we no longer ned to do this?
> >
> > Is it because __split_unmapped_folio() will somehow take care of this in
> > advance/render this meaningless?
> >
> > This definitely needs to be mentioned in the commit message.
>
> Because “new_folio = folio_next(folio)” in the for loop initialization
> part. The @folio is skipped at the very beginning. I will add a comment
> to highlight this, since the code change is too subtle.

Ahh yes, that is quite subtle, a comment would be helpful, thanks!
>
> >
> >> +			folio_ref_unfreeze(
> >> +				new_folio,
> >> +				1 + ((mapping || swap_cache) ?
> >> +					     folio_nr_pages(new_folio) :
> >> +					     0));
> >
> > Again, be nice to separate this out, but I think in a follow-up not here.
>
> OK.

Well, actually no - you fix this in the next patch :P

>
> >
> >> +
> >> +			lru_add_split_folio(folio, new_folio, lruvec, list);
> >> +
> >> +			/* Some pages can be beyond EOF: drop them from cache */
> >> +			if (new_folio->index >= end) {
> >> +				if (shmem_mapping(mapping))
> >> +					nr_shmem_dropped += folio_nr_pages(new_folio);
> >> +				else if (folio_test_clear_dirty(new_folio))
> >> +					folio_account_cleaned(
> >> +						new_folio,
> >> +						inode_to_wb(mapping->host));
> >> +				__filemap_remove_folio(new_folio, NULL);
> >> +				folio_put_refs(new_folio,
> >> +					       folio_nr_pages(new_folio));
> >> +			} else if (mapping) {
> >> +				__xa_store(&mapping->i_pages, new_folio->index,
> >> +					   new_folio, 0);
> >> +			} else if (swap_cache) {
> >> +				__xa_store(&swap_cache->i_pages,
> >> +					   swap_cache_index(new_folio->swap),
> >> +					   new_folio, 0);
> >> +			}
> >> +		}
> >> +		/*
> >> +		 * Unfreeze @folio only after all page cache entries, which
> >> +		 * used to point to it, have been updated with new folios.
> >> +		 * Otherwise, a parallel folio_try_get() can grab origin_folio
> >> +		 * and its caller can see stale page cache entries.
> >> +		 */
> >> +		folio_ref_unfreeze(folio, 1 +
> >> +			((mapping || swap_cache) ? folio_nr_pages(folio) : 0));
> >
> > This line is horrid, probably one for a future series but this sort of
> > calculation of what the number of refs should be post-freeze should clearly
> > be separated out or at least made abundantly clear in an open-coded
> > implementation.
>
> It is addressed in patch 2. And you already noticed it. :)

Haha yes, always like it when that happens :)

>
> >
> >> +
> >> +		unlock_page_lruvec(lruvec);
> >> +
> >> +		if (swap_cache)
> >> +			xa_unlock(&swap_cache->i_pages);
> >>  	} else {
> >>  		spin_unlock(&ds_queue->split_queue_lock);
> >> -fail:
> >> -		if (mapping)
> >> -			xas_unlock(&xas);
> >> -		local_irq_enable();
> >> -		remap_page(folio, folio_nr_pages(folio), 0);
> >>  		ret = -EAGAIN;
> >>  	}
> >> +fail:
> >> +	if (mapping)
> >> +		xas_unlock(&xas);
> >> +
> >> +	local_irq_enable();
> >> +
> >> +	if (nr_shmem_dropped)
> >> +		shmem_uncharge(mapping->host, nr_shmem_dropped);
> >> +
> >> +	remap_page(folio, 1 << order,
> >> +		   !ret && folio_test_anon(folio) ? RMP_USE_SHARED_ZEROPAGE :
> >> +						    0);
> >
> > I really don't like this !ret but here, this isn't very readable.
> >
> > Something like:
> >
> > 	int flags;
> >
> > 	...
> >
> > 	if (!ret && folio_test_anon(folio))
> > 		flags = RMP_USE_SHARED_ZERO_PAGE;
> > 	remap_page(folio, 1 << order, flags);
> >
> > Would be better.
> >
> > But really this is all screaming out to be separated into parts of
> > course. But that's one for a follow-up series...
>
> Sure. Will add another patch to address this.

Thanks!

>
> >
> >> +
> >> +	/*
> >> +	 * Unlock all after-split folios except the one containing @lock_at
> >> +	 * page. If @folio is not split, it will be kept locked.
> >> +	 */
> >> +	for (new_folio = folio; new_folio != next_folio; new_folio = next) {
> >> +		next = folio_next(new_folio);
> >> +		if (new_folio == page_folio(lock_at))
> >> +			continue;
> >> +
> >> +		folio_unlock(new_folio);
> >> +		/*
> >> +		 * Subpages may be freed if there wasn't any mapping
> >> +		 * like if add_to_swap() is running on a lru page that
> >> +		 * had its mapping zapped. And freeing these pages
> >> +		 * requires taking the lru_lock so we do the put_page
> >> +		 * of the tail pages after the split is complete.
> >> +		 */
> >> +		free_folio_and_swap_cache(new_folio);
> >> +	}
> >>
> >>  out_unlock:
> >>  	if (anon_vma) {
> >> --
> >> 2.47.2
> >>
> >
> > Generally I see why you're not using origin_folio any more since you can
> > just use folio everywhere, but I wonder if this makes things more
> > confusing.
> >
> > On the other hand, this function is already hugely confusing so maybe not a
> > big deal and can be addressed in follow ups...
>
> Since it is causing confusion. I will use origin_folio in the moved code.

Thanks!

>
> I will send V4 to address your comments and add
> “mm/huge_memory: refactor after-split (page) cache code.” in.

Much appreciated :)

>
> Best Regards,
> Yan, Zi

On 17 Jul 2025, at 13:44, Lorenzo Stoakes wrote:

> On Thu, Jul 17, 2025 at 11:41:01AM -0400, Zi Yan wrote:
>> On 17 Jul 2025, at 10:07, Lorenzo Stoakes wrote:
>>
>>> On Mon, Jul 14, 2025 at 01:18:22PM -0400, Zi Yan wrote:
>>>> remap(), folio_ref_unfreeze(), lru_add_split_folio() are not relevant to
>>>> splitting unmapped folio operations. Move them out to the caller so that
>>>> __split_unmapped_folio() only handles unmapped folio splits. This makes
>>>> __split_unmapped_folio() reusable.
>>>
>>> Nit but maybe worth mentioning the various renames etc.
>>
>> You mean release -> new_folio, origin_folio is replaced by folio?
>> Sure, I can do that.
>
> Yeah that kind of thing, just basically briefly mention the other stuff you
> did.
>
> Thanks!
>
>>
>>>
>>>>
>>>> Convert VM_BUG_ON(mapping) to use VM_WARN_ON_ONCE_FOLIO().
>>>>
>>>> Signed-off-by: Zi Yan <ziy@nvidia.com>
>>>> Acked-by: Balbir Singh <balbirs@nvidia.com>
>>>
>>> After a lot of staring, 2 difftastic's at once and exactly 0 coverity
>>> instances, I've convinced myself this looks right.
>>>
>>> I think you really should have split this up into smaller patches, as this
>>> is moving stuff around and changing stuff all at once with a lot of
>>> complexity and moving parts.
>>>
>>> However not going to make you do that, since you got acks and I don't want
>>> to hold this up.
>>>
>>> I have a few nits + queries below that need addressing however, see below.
>>
>> Since I need to address these nits, I might just split this up.
>> How about:
>>
>> 1.
>
> Missing some text? :P

Yeah, I meant to fill this up after going through your comments below.
The plan is:

1. Just move code from __split_unmapped_folio() to __folio_split().
2. one patch to remove after_split label
3. one patch to move fail label and related code
4. one patch to refactor remap_page() flag
5. one patch to convert VM_BUG* to VM_WARM*, three instances.
6. use folio_expected_ref_count() patch
7. mm/huge_memory: refactor after-split (page) cache code.

Maybe 2, 3, 4 can be squashed into a single refactor patch?

Best Regards,
Yan, Zi

On Thu, Jul 17, 2025 at 02:05:06PM -0400, Zi Yan wrote:
> Yeah, I meant to fill this up after going through your comments below.
> The plan is:
>
> 1. Just move code from __split_unmapped_folio() to __folio_split().
> 2. one patch to remove after_split label
> 3. one patch to move fail label and related code
> 4. one patch to refactor remap_page() flag
> 5. one patch to convert VM_BUG* to VM_WARM*, three instances.
> 6. use folio_expected_ref_count() patch
> 7. mm/huge_memory: refactor after-split (page) cache code.
>
> Maybe 2, 3, 4 can be squashed into a single refactor patch?
>
> Best Regards,
> Yan, Zi

Will leave to your discretion as to how best to structure :)

Cheers, Lorenzo

On 14.07.25 19:18, Zi Yan wrote:
> remap(), folio_ref_unfreeze(), lru_add_split_folio() are not relevant to
> splitting unmapped folio operations. Move them out to the caller so that
> __split_unmapped_folio() only handles unmapped folio splits. This makes
> __split_unmapped_folio() reusable.
> 
> Convert VM_BUG_ON(mapping) to use VM_WARN_ON_ONCE_FOLIO().
> 
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> Acked-by: Balbir Singh <balbirs@nvidia.com>
> ---

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb