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

[Dev Jain]

On 07/07/26 7:56 pm, David Hildenbrand (Arm) wrote:
> 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@xxxxxxx>
>> ---
>>  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).

Okay

> 
>> +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;
> }

Okay


> 
>> +{
>> +	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 :)

Ack

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

Okay
> 
>> +	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?

arch_unmap_one -> adi_save_tags is saving tags in an mm related structure. So
I don't think there is any need to have a reverse operation in case of
unmap failure, since the ADI tags are still there on the page.

> 
>> +		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;
>> +}
>> +
>