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

[Dev Jain]

On 07/07/26 7:48 pm, David Hildenbrand (Arm) wrote:
> On 7/7/26 14:11, 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.
>>
>> No functional change intended.
>>
>> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
>> ---
>>  mm/rmap.c | 87 +++++++++++++++++++++++++++++++------------------------
>>  1 file changed, 49 insertions(+), 38 deletions(-)
>>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 934773dfa2f2a..00b571c2a1bab 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -2067,6 +2067,52 @@ static bool try_to_unmap_hugetlb_one(struct folio *folio,
>>  	return ret;
>>  }
>>  
>> +static inline bool ttu_anon_lazyfree_folio(struct vm_area_struct *vma,
>> +		struct folio *folio)
>> +{
>> +	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.
>> +		 */
>> +		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.
>> +		 */
>> +		return false;
>> +	}
>> +
>> +	return true;
> 
> You could simply do
> 
> 
> /*
>  * Additional references could be due to GUP or from a speculative
>  * lookup. ...

Are you also suggesting to shorten the comment?


>  */
> return ref_count == 1 + map_count;

Yes much better.


> 
> 
> Nothing else jumped at me
> 
> Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>