Re: [PATCH RFC v3] mm: Proper document tail pages fields for folio

[Peter Xu <peterx@redhat.com>]

On Wed, Aug 16, 2023 at 03:33:30PM +0200, David Hildenbrand wrote:
> On 15.08.23 23:25, Peter Xu wrote:
> > Tail page struct reuse is over-comlicated.  Not only because we have
> 
> It is complicated, agreed.
> 
> With the ->private for THP_SWAP gone, we would have to document less.
> Stating that 4*4byte / 4*8 byte are available after flags+head would
> be sufficient and I'd even drop the table.
> 
> 
> > implicit uses of tail page fields (mapcounts, or private for thp swap
> > support, etc., that we may still use in the page structs,
> 
> Instead of documenting that thp swap should no longer touch the private
> field of tail pages, maybe we can indeed fix that quite easily.
> 
> My simple tests passed so far. If there isn't something obvious missing,
> I can do more testing and send this as an official patch.

It'll be definitely good to fix it rather than document if possible.

Nothing wrong I spot quickly, you may just need a more complete cc list for
swap. One trivial comment below.

> 
> 
> From ec0f8b0dd8fb81c316b6a4c5fc9ae7563e625404 Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Wed, 16 Aug 2023 13:14:45 +0200
> Subject: [PATCH] mm/swap: stop using page->private on tail pages for THP_SWAP
> 
> Let's stop using page->private on tail pages, making it possible to
> just unconditionally reuse that field in the tail pages of large folios.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/arm64/mm/mteswap.c |  5 +++--
>  include/linux/swap.h    |  9 +++++++++
>  mm/huge_memory.c        | 15 ++++++---------
>  mm/memory.c             |  2 +-
>  mm/rmap.c               |  2 +-
>  mm/swap_state.c         |  4 ++--
>  mm/swapfile.c           |  4 ++--
>  7 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c
> index cd508ba80ab1..a31833e3ddc5 100644
> --- a/arch/arm64/mm/mteswap.c
> +++ b/arch/arm64/mm/mteswap.c
> @@ -33,8 +33,9 @@ int mte_save_tags(struct page *page)
>  	mte_save_page_tags(page_address(page), tag_storage);
> -	/* page_private contains the swap entry.val set in do_swap_page */
> -	ret = xa_store(&mte_pages, page_private(page), tag_storage, GFP_KERNEL);
> +	/* lookup the swap entry.val from the page */
> +	ret = xa_store(&mte_pages, page_swap_entry(page).val, tag_storage,
> +		       GFP_KERNEL);
>  	if (WARN(xa_is_err(ret), "Failed to store MTE tags")) {
>  		mte_free_tag_storage(tag_storage);
>  		return xa_err(ret);
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index bb5adc604144..84fe0e94f5cd 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -339,6 +339,15 @@ static inline swp_entry_t folio_swap_entry(struct folio *folio)
>  	return entry;
>  }
> +static inline swp_entry_t page_swap_entry(struct page *page)
> +{
> +	struct folio *folio = page_folio(page);
> +	swp_entry_t entry = folio_swap_entry(folio);
> +
> +	entry.val += page - &folio->page;
> +	return entry;
> +}
> +
>  static inline void folio_set_swap_entry(struct folio *folio, swp_entry_t entry)
>  {
>  	folio->private = (void *)entry.val;
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 0b709d2c46c6..f7e04cbcb063 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2451,18 +2451,15 @@ static void __split_huge_page_tail(struct page *head, int tail,
>  	page_tail->index = head->index + tail;
>  	/*
> -	 * page->private should not be set in tail pages with the exception
> -	 * of swap cache pages that store the swp_entry_t in tail pages.
> -	 * Fix up and warn once if private is unexpectedly set.
> -	 *
> -	 * What of 32-bit systems, on which folio->_pincount overlays
> -	 * head[1].private?  No problem: THP_SWAP is not enabled on 32-bit, and
> -	 * pincount must be 0 for folio_ref_freeze() to have succeeded.
> +	 * page->private should not be set in tail pages. Fix up and warn once
> +	 * if private is unexpectedly set.
>  	 */
> -	if (!folio_test_swapcache(page_folio(head))) {
> -		VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, page_tail);
> +	if (unlikely(page_tail->private)) {
> +		VM_WARN_ON_ONCE_PAGE(true, page_tail);
>  		page_tail->private = 0;
>  	}
> +	if (PageSwapCache(head))
> +		set_page_private(page_tail, (unsigned long)head->private + tail);
>  	/* Page flags must be visible before we make the page non-compound. */
>  	smp_wmb();
> diff --git a/mm/memory.c b/mm/memory.c
> index d003076b218d..ff13242c1589 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3882,7 +3882,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>  		 * changed.
>  		 */
>  		if (unlikely(!folio_test_swapcache(folio) ||
> -			     page_private(page) != entry.val))
> +			     page_swap_entry(page).val != entry.val))
>  			goto out_page;
>  		/*
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 1f04debdc87a..ec7f8e6c9e48 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1647,7 +1647,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  			 */
>  			dec_mm_counter(mm, mm_counter(&folio->page));
>  		} else if (folio_test_anon(folio)) {
> -			swp_entry_t entry = { .val = page_private(subpage) };
> +			swp_entry_t entry = page_swap_entry(subpage);
>  			pte_t swp_pte;
>  			/*
>  			 * Store the swap location in the pte.
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 01f15139b7d9..450819934e34 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -100,6 +100,7 @@ int add_to_swap_cache(struct folio *folio, swp_entry_t entry,
>  	folio_ref_add(folio, nr);
>  	folio_set_swapcache(folio);
> +	folio_set_swap_entry(folio, entry);
>  	do {
>  		xas_lock_irq(&xas);
> @@ -113,7 +114,6 @@ int add_to_swap_cache(struct folio *folio, swp_entry_t entry,
>  				if (shadowp)
>  					*shadowp = old;
>  			}
> -			set_page_private(folio_page(folio, i), entry.val + i);
>  			xas_store(&xas, folio);
>  			xas_next(&xas);
>  		}
> @@ -154,9 +154,9 @@ void __delete_from_swap_cache(struct folio *folio,
>  	for (i = 0; i < nr; i++) {
>  		void *entry = xas_store(&xas, shadow);
>  		VM_BUG_ON_PAGE(entry != folio, entry);
> -		set_page_private(folio_page(folio, i), 0);
>  		xas_next(&xas);
>  	}
> +	folio->private = 0;

I'd rather remove all direct reference to "private" for swap alongside, if
this would be the last spot (perhaps folio_set_swap_entry()).

>  	folio_clear_swapcache(folio);
>  	address_space->nrpages -= nr;
>  	__node_stat_mod_folio(folio, NR_FILE_PAGES, -nr);
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index d46933adf789..bd9d904671b9 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3369,7 +3369,7 @@ struct swap_info_struct *swp_swap_info(swp_entry_t entry)
>  struct swap_info_struct *page_swap_info(struct page *page)
>  {
> -	swp_entry_t entry = { .val = page_private(page) };
> +	swp_entry_t entry = page_swap_entry(page);
>  	return swp_swap_info(entry);
>  }
> @@ -3384,7 +3384,7 @@ EXPORT_SYMBOL_GPL(swapcache_mapping);
>  pgoff_t __page_file_index(struct page *page)
>  {
> -	swp_entry_t swap = { .val = page_private(page) };
> +	swp_entry_t swap = page_swap_entry(page);
>  	return swp_offset(swap);
>  }
>  EXPORT_SYMBOL_GPL(__page_file_index);
> -- 
> 2.41.0
> 
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 

-- 
Peter Xu