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

[Dev Jain]

On 11/05/26 12:58 pm, David Hildenbrand (Arm) wrote:
> 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_...*

Ack

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

Yep great I'll do that.

> 
>