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

[David Hildenbrand (Arm)]

On 7/9/26 14:16, Dev Jain wrote:
> 
> 
> 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?


To reduce its indentation and then comment on the conditional

"Additional references could be due to GUP or from a speculative .."

Instead of "Additional reference."


-- 
Cheers,

David