Re: [PATCH v3 3/9] mm/rmap: refactor some code around lazyfree folio unmapping

[David Hildenbrand (Arm)]

On 5/6/26 11:44, Dev Jain wrote:
> For lazyfree folio unmapping, after clearing the ptes we must abort the
> operation if the folio got dirtied or it has unexpected references.
> 
> Refactor this logic into a function which will return whether we need
> to abort or not.
> 
> If we abort, we restore the ptes and bail out of try_to_unmap_one.
> Otherwise adjust the rss stats of the mm and jump to a label.
> 
> Also rename that label from "discard" to "finish_unmap"; the former
> is appropriate in the lazyfree context, but the code following the label
> is executed for other successful unmap code paths too, so 'discard' does
> not sound correct for them.
> 
> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> ---
>  mm/rmap.c | 95 ++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 55 insertions(+), 40 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index a98acdea0530a..bd4e3639e26ed 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1978,6 +1978,56 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio,
>  				     FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY);
>  }
>  
> +static inline bool can_unmap_lazyfree_folio_range(struct vm_area_struct *vma,
> +		struct folio *folio, unsigned long address, pte_t *ptep,
> +		pte_t pteval, unsigned long nr_pages)


Similar comment: ttu_...*

> +{
> +	struct mm_struct *mm = vma->vm_mm;
> +	int ref_count, map_count;
> +
> +	/*
> +	 * Synchronize with gup_pte_range():
> +	 * - clear PTE; barrier; read refcount
> +	 * - inc refcount; barrier; read PTE
> +	 */
> +	smp_mb();
> +
> +	ref_count = folio_ref_count(folio);
> +	map_count = folio_mapcount(folio);
> +
> +	/*
> +	 * Order reads for page refcount and dirty flag
> +	 * (see comments in __remove_mapping()).
> +	 */
> +	smp_rmb();
> +
> +	if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
> +		/*
> +		 * redirtied either using the page table or a previously
> +		 * obtained GUP reference.
> +		 */
> +		set_ptes(mm, address, ptep, pteval, nr_pages);
> +		folio_set_swapbacked(folio);
> +		return false;
> +	}
> +
> +	if (ref_count != 1 + map_count) {
> +		/*
> +		 * Additional reference. Could be a GUP reference or any
> +		 * speculative reference. GUP users must mark the folio
> +		 * dirty if there was a modification. This folio cannot be
> +		 * reclaimed right now either way, so act just like nothing
> +		 * happened.
> +		 * We'll come back here later and detect if the folio was
> +		 * dirtied when the additional reference is gone.
> +		 */
> +		set_ptes(mm, address, ptep, pteval, nr_pages);
> +		return false;
> +	}
> +
> +	return true;


Doing the set_ptes() in a function called "can_unmap_lazyfree_folio_range" is
not appropriate.

Can we just leave that in the caller? We only do the when we return false.

And hey, then you can call this function ttu_can_unmap_lazyfree_folio() and
avoid passing pte ranges. :)


-- 
Cheers,

David