Re: [PATCH v3 08/18] mm/huge_memory: move unmap and remap into the split helpers

[David Hildenbrand (Arm)]

On 8/27/26 19:35, Kairui Song wrote:
> On Fri, Aug 28, 2026 at 12:32 AM David Hildenbrand (Arm)
> <david@xxxxxxxxxx> wrote:
>>
>> On 8/20/26 20:55, Kairui Song via B4 Relay wrote:
>>> From: Kairui Song <kasong@xxxxxxxxxxx>
>>>
>>> To prepare for further cleanup, move the unmap/remap handling from
>>> __folio_split() into the split helpers.  Only anon folios need to
>>> be remapped, so remap_page() is now only called for anon splits and
>>> the anon check in remap_page() is redundant and can be removed.
>>
>> I can understand why we would want to move the remap_page().
>>
>> But why the unmap_folio()?
>>
>>>
>>> Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>
>>> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
>>> ---
>>>  mm/huge_memory.c | 58 ++++++++++++++++++++++++++++++--------------------------
>>>  1 file changed, 31 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 01c8cf428595..af9c2edd1fba 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -3589,9 +3589,6 @@ static void remap_page(struct folio *folio, unsigned long nr, int flags)
>>>  {
>>>       int i = 0;
>>>
>>> -     /* If unmap_folio() uses try_to_migrate() on file, remove this check */
>>> -     if (!folio_test_anon(folio))
>>> -             return;
>>>       for (;;) {
>>>               remove_migration_ptes(folio, folio, TTU_RMAP_LOCKED | flags);
>>>               i += folio_nr_pages(folio);
>>
>> I think we should rename that function now to unmap_folio, but likely better
>> unmap_anon_folio().
>>
>>> @@ -3933,19 +3930,23 @@ static unsigned int folio_cache_ref_count(const struct folio *folio)
>>>       return folio_nr_pages(folio);
>>>  }
>>>
>>> -static int __folio_freeze_split_unmapped_anon(struct folio *folio, unsigned int new_order,
>>> -                                           struct page *split_at, bool do_lru,
>>> -                                           struct list_head *list, enum split_type split_type)
>>> +static int __folio_split_unmap_and_freeze_anon(struct folio *folio, unsigned int new_order,
>>> +                                            struct page *split_at, bool do_lru, bool unmap,
>>> +                                            struct list_head *list, enum split_type split_type)
>>>  {
>>>       struct folio *end_folio = folio_next(folio);
>>>       struct swap_cluster_info *ci = NULL;
>>>       struct folio *new_folio, *next;
>>>       int old_order = folio_order(folio);
>>> +     enum ttu_flags ttu_flags = 0;
>>>       struct list_lru_one *lru;
>>>       struct lruvec *lruvec;
>>>       bool dequeue_deferred;
>>>       int ret = 0;
>>>
>>> +     if (unmap)
>>> +             unmap_folio(folio);
>>
>> I really hate that "bool unmap" parameter.
>>
>> folio_split_unmapped() has this VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio);
>>
>> So you can really just do
>>
>> if (folio_mapped())
>>         unmap_folio(folio);
>>
>> Or have an early exit and hide it in unmap_folio().
> 
> The only special case here is device private memory, from folio_split_unmapped.
> 
> Maybe I can just keep the unmap_folfio where it is, but
> __folio_split_unmap_and_freeze_anon need to know if the folio need to
> be remapped.
> 
> Will a "bool remap" parameter look good?

I'd avoid that and rather have something like the following in the function.

if (folio_mapped()) {
	folio_unmapped = true;
	unmap_folio(folio);
}

...

if (folio_unmapped)
	remap_anon_folio(folio)


...

-- 
Cheers,

David