Re: [PATCH] mm: Introduce free_folio_and_swap_cache() to replace free_page_and_swap_cache()

[Zi Yan]

On 10 Apr 2025, at 14:00, nifan.cxl@xxxxxxxxx wrote:

> From: Fan Ni <fan.ni@xxxxxxxxxxx>
>
> The function free_page_and_swap_cache() takes a struct page pointer as
> input parameter, but it will immediately convert it to folio and all
> operations following within use folio instead of page.  It makes more
> sense to pass in folio directly.
>
> Introduce free_folio_and_swap_cache(), which takes folio as input to
> replace free_page_and_swap_cache().  And apply it to all occurrences
> where free_page_and_swap_cache() was used.
>
> Signed-off-by: Fan Ni <fan.ni@xxxxxxxxxxx>
> ---
>  arch/s390/include/asm/tlb.h |  4 ++--
>  include/linux/swap.h        | 10 +++++++---
>  mm/huge_memory.c            |  2 +-
>  mm/khugepaged.c             |  2 +-
>  mm/swap_state.c             |  8 +++-----
>  5 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/arch/s390/include/asm/tlb.h b/arch/s390/include/asm/tlb.h
> index f20601995bb0..e5103e8e697d 100644
> --- a/arch/s390/include/asm/tlb.h
> +++ b/arch/s390/include/asm/tlb.h
> @@ -40,7 +40,7 @@ static inline bool __tlb_remove_folio_pages(struct mmu_gather *tlb,
>  /*
>   * Release the page cache reference for a pte removed by
>   * tlb_ptep_clear_flush. In both flush modes the tlb for a page cache page
> - * has already been freed, so just do free_page_and_swap_cache.
> + * has already been freed, so just do free_folio_and_swap_cache.
>   *
>   * s390 doesn't delay rmap removal.
>   */
> @@ -49,7 +49,7 @@ static inline bool __tlb_remove_page_size(struct mmu_gather *tlb,
>  {
>  	VM_WARN_ON_ONCE(delay_rmap);
>
> -	free_page_and_swap_cache(page);
> +	free_folio_and_swap_cache(page_folio(page));
>  	return false;
>  }

__tlb_remove_page_size() is ruining the fun of the conversion. But it will be
converted to use folio eventually.

>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index db46b25a65ae..9fc8856eeed9 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -450,7 +450,7 @@ static inline unsigned long total_swapcache_pages(void)
>  }
>
>  void free_swap_cache(struct folio *folio);
> -void free_page_and_swap_cache(struct page *);
> +void free_folio_and_swap_cache(struct folio *folio);
>  void free_pages_and_swap_cache(struct encoded_page **, int);
>  /* linux/mm/swapfile.c */
>  extern atomic_long_t nr_swap_pages;
> @@ -522,8 +522,12 @@ static inline void put_swap_device(struct swap_info_struct *si)
>  	do { (val)->freeswap = (val)->totalswap = 0; } while (0)
>  /* only sparc can not include linux/pagemap.h in this file
>   * so leave put_page and release_pages undeclared... */
> -#define free_page_and_swap_cache(page) \
> -	put_page(page)
> +#define free_folio_and_swap_cache(folio)	\
> +	do {					\
> +		if (!folio_test_slab(folio))	\
> +			folio_put(folio);	\
> +	} while (0)
> +

Like Matthew pointed out in another email, the test is not needed.

Otherwise, it looks good to me. Thanks.

Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>

Best Regards,
Yan, Zi