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

[Garg, Shivank]

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.
>

Makes sense. I'll drop this.

I think using folio_get_private() is fine.

Thanks,
Shivank