Re: [PATCH 4/5] mm/rmap: refactor anon swapbacked folio unmap in try_to_unmap_one

["David Hildenbrand (Arm)" <david@kernel.org>]

On 7/7/26 14:11, Dev Jain wrote:
> Refactor anonymous swapbacked folio unmap to ttu_anon_swapbacked_folio().
> 
> No functional change intended.
> 
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
>  mm/rmap.c | 105 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 62 insertions(+), 43 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 00b571c2a1bab..ade78df5be2bd 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -2113,6 +2113,64 @@ static inline bool ttu_anon_lazyfree_folio(struct vm_area_struct *vma,
>  	return true;
>  }
>  

BTW, no need for the "inline" for most of these functions (applies to other
patches as well).

> +static inline void set_swp_pte_at(struct mm_struct *mm, unsigned long address,
> +		pte_t *ptep, swp_entry_t entry, pte_t pteval, bool anon_exclusive)

It's confusing that we say "set_swp_pte_at", but the pte_t we was is actually
the old pte.

Can we instead have a function that creates us a swap_pte from the other data,
and then do the set_pte_at() in the caller?

static pte_t swp_pte_prepare(...)
{
	...
	return swp_pte;
}

> +{
> +	pte_t swp_pte = swp_entry_to_pte(entry);
> +
> +	if (anon_exclusive)
> +		swp_pte = pte_swp_mkexclusive(swp_pte);
> +
> +	if (likely(pte_present(pteval))) {
> +		if (pte_soft_dirty(pteval))
> +			swp_pte = pte_swp_mksoft_dirty(swp_pte);
> +		if (pte_uffd_wp(pteval))
> +			swp_pte = pte_swp_mkuffd_wp(swp_pte);
> +	} else {
> +		/* Device-exclusive entry */
> +		if (pte_swp_soft_dirty(pteval))
> +			swp_pte = pte_swp_mksoft_dirty(swp_pte);
> +		if (pte_swp_uffd_wp(pteval))
> +			swp_pte = pte_swp_mkuffd_wp(swp_pte);
> +	}
> +
> +	set_pte_at(mm, address, ptep, swp_pte);
> +}
> +
> +static inline bool ttu_anon_swapbacked_folio(struct vm_area_struct *vma, struct folio *folio,
> +		struct page *subpage, unsigned long address, pte_t *ptep,

Subpages do not exist :)

> +		pte_t pteval)
> +{
> +	bool anon_exclusive = folio_test_anon(folio) && PageAnonExclusive(subpage);

const?

> +	swp_entry_t entry = page_swap_entry(subpage);
> +	struct mm_struct *mm = vma->vm_mm;
> +
> +	if (folio_dup_swap(folio, subpage) < 0)
> +		return false;
> +
> +	/*
> +	 * arch_unmap_one() is expected to be a NOP on
> +	 * architectures where we could have PFN swap PTEs,
> +	 * so we'll not check/care.
> +	 */
> +	if (arch_unmap_one(mm, vma, address, pteval) < 0) {
> +		folio_put_swap(folio, subpage);
> +		return false;
> +	}
> +
> +	/* See folio_try_share_anon_rmap(): clear PTE first. */
> +	if (anon_exclusive && folio_try_share_anon_rmap_pte(folio, subpage)) {
> +		folio_put_swap(folio, subpage);

I recall I stumbled over this before, but there is no way to undo the
arch_unmap_one(), right?

> +		return false;
> +	}
> +
> +	mm_prepare_for_swap_entries(mm);
> +	dec_mm_counter(mm, MM_ANONPAGES);
> +	inc_mm_counter(mm, MM_SWAPENTS);
> +	set_swp_pte_at(mm, address, ptep, entry, pteval, anon_exclusive);
> +	return true;
> +}
> +

-- 
Cheers,

David