Re: [PATCH v2 14/17] mm/huge_memory: clean up after-split folio freeing in __folio_split

[Kairui Song]

Sashiko reports:

--- Patch [14]: [PATCH v2 14/17] mm/huge_memory: clean up after-split
folio freeing in __folio_split ---
commit 30068a849a3b68568f1661083bd4c5a685ac749a
Author: Kairui Song <kasong@xxxxxxxxxxx>

mm/huge_memory: clean up after-split folio freeing in __folio_split

This commit replaces free_folio_and_swap_cache() with direct calls to
folio_free_swap() and folio_put() in the __folio_split() after-split loop.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -4343,14 +4343,16 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>   if (new_folio == page_folio(lock_at))
>   continue;
>
> - folio_unlock(new_folio);
>   /*
>   * Subpages whose mapping has been zapped may be freed
>   * earlier, but freeing them requires taking the
> - * lru_lock, so we defer put_page() on tail pages until
> + * lru_lock, so we defer folio_put() on tail pages until
>   * after the split completes.
>   */
> - free_folio_and_swap_cache(new_folio);
> + if (is_swapcache)
> + folio_free_swap(new_folio);

Is it safe to unconditionally call folio_free_swap() here without checking
if the subfolios are mapped?

The commit message states "The sub folios are still locked and unmapped
here", but looking at the earlier split path, __folio_split_unmap_and_freeze()
calls remap_page() before returning to __folio_split(). This means the
subfolios can actually be mapped in memory by the time we reach this point.

Because they are remapped, calling folio_free_swap() here will unconditionally
free the swap cache for actively mapped pages, marking them dirty and forcing
unnecessary swap allocations and disk writebacks under memory pressure.

Previously, free_folio_and_swap_cache() called free_swap_cache(), which
explicitly checked !folio_mapped() before freeing the cache to prevent this.
Does bypassing this check introduce a performance regression by thrashing
the swap cache for successfully split mapped anonymous folios?

> + folio_unlock(new_folio);
> + folio_put(new_folio);
>   }
>
>  out:

---

Right, I need to check the mapped status. It might be actually fine to
blindly clean the swap cache, and it's always safe to do so, but let
me keep the old behavior for now. I will update it.