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

[David Hildenbrand (Arm)]

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.

-- 
Cheers,

David