Re: [PATCH v2] mm/migrate: rename PAGE_ migration flags to FOLIO_

[Zi Yan]

On 25 Mar 2026, at 7:04, Garg, Shivank wrote:

> On 3/25/2026 2:55 PM, David Hildenbrand (Arm) wrote:
>> On 3/25/26 10:21, Garg, Shivank wrote:
>>>
>>>>>
>>>>>  static void __migrate_folio_record(struct folio *dst,
>>>>> -				   int old_page_state,
>>>>> +				   int old_folio_state,
>>>>>  				   struct anon_vma *anon_vma)
>>>>>  {
>>>>> -	dst->private = (void *)anon_vma + old_page_state;
>>>>> +	dst->private = (void *)anon_vma + old_folio_state;
>>>>>  }
>>>>>
>>>>>  static void __migrate_folio_extract(struct folio *dst,
>>>>> -				   int *old_page_state,
>>>>> +				   int *old_folio_state,
>>>>>  				   struct anon_vma **anon_vmap)
>>>>>  {
>>>>>  	unsigned long private = (unsigned long)dst->private;
>>>>>
>>>>> -	*anon_vmap = (struct anon_vma *)(private & ~PAGE_OLD_STATES);
>>>>> -	*old_page_state = private & PAGE_OLD_STATES;
>>>>> +	*anon_vmap = (struct anon_vma *)(private & ~FOLIO_OLD_STATES);
>>>>> +	*old_folio_state = private & FOLIO_OLD_STATES;
>>>>>  	dst->private = NULL;
>>>>>  }
>>>>
>>>> Just an observation on folio->private, it is void*, but page->private
>>>> is unsigned long. It confused me a bit. There are folio_get_private()
>>>> and folio_change_private(), I wonder if we want to use them here
>>>> instead of direct ->private accesses. Feel free to ignore this,
>>>> since it is irrelevant to this patch.
>>>
>>> Would something like this as a follow-up patch work?
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 6d4a85f903d8..55d1af6c9759 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1143,17 +1143,17 @@ enum {
>>>  static void __migrate_folio_record(struct folio *dst,
>>>  		int old_folio_state, struct anon_vma *anon_vma)
>>>  {
>>> -	dst->private = (void *)anon_vma + old_folio_state;
>>> +	folio_change_private(dst, (void *)anon_vma + old_folio_state);
>>>  }
>>>
>>>  static void __migrate_folio_extract(struct folio *dst,
>>>  		int *old_folio_state, struct anon_vma **anon_vmap)
>>>  {
>>> -	unsigned long private = (unsigned long)dst->private;
>>> +	unsigned long private = (unsigned long)folio_get_private(dst);
>>>
>>>  	*anon_vmap = (struct anon_vma *)(private & ~FOLIO_OLD_STATES);
>>>  	*old_folio_state = private & FOLIO_OLD_STATES;
>>> -	dst->private = NULL;
>>> +	folio_change_private(dst, NULL);
>>>  }
>>
>>
>> Isn't folio_change_private() part of the
>> folio_attach_private()/folio_detach_private() interface that has
>> completely different semantics?
>>
>> "The page must previously have had data attached and the data must be
>> detached before the folio will be freed."
>>
>> (btw, that comment should refer to pages)
>>
>> So I would not use folio_change_private(). An accidental
>> folio_attach_private() / folio_detach_private() would be rather undesired.

Hi David,

In terms of folio_change_private(), I did not think it is related to
folio_{attach,detach}_private(), since the latter change folio refcount during
the operation. If folio_change_private() is related to attach/detach,
I imagine it would check folio refcount before touches ->private. But
that is my interpretation.

BTW, do you know why we have set_page_private() but no folio_set_private()?
I would suggest folio_set_private() if it exists.

>>
>
> Makes sense. I'll drop this.
>
> I think using folio_get_private() is fine.

Hi Shivank,

Thanks. Sorry for the confusion.

--
Best Regards,
Yan, Zi