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

[Zi Yan <ziy@nvidia.com>]

On 25 Mar 2026, at 10:53, David Hildenbrand (Arm) wrote:

> On 3/25/26 15:21, Zi Yan wrote:
>> On 25 Mar 2026, at 7:04, Garg, Shivank wrote:
>>
>>> On 3/25/2026 2:55 PM, David Hildenbrand (Arm) wrote:
>>>>
>>>>
>>>> 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,
>
> Hi,
>
>>
>> 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.
>
> I mean, given that
>
> a) It's located in pagemap.h in between folio_attach_private() and
> folio_detach_private()
>
> b) It clearly states that "The page must previously have had data
> attached and the data must be detached before the folio will be freed."
>
> This is the wrong API to use?
>
> Sure, it sets folio->private but in different context.
>
> I can spot one user in mm/hugetlb.c, that likely also should not be
> using this API, because there likely was no previous attach/detach.
>
>>
>> BTW, do you know why we have set_page_private() but no folio_set_private()?
>> I would suggest folio_set_private() if it exists.
>
> folio_set_private() sets ... PG_private. :)
>
> folio_test_private() checks PG_private and folio_get_private() returns
> page->private.
>
> A cursed interface.

Oh man. folio_get_private() should be renamed to folio_get_private_data(),
so that we can have folio_set_private_data().

>
> What we should likely do is:
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 3944b51ebac6..702cb2c0bc0e 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -426,6 +426,7 @@ struct folio {
>                         union {
>                                 void *private;
>                                 swp_entry_t swap;
> +                               unsigned long migrate_info;
>                         };
>                         atomic_t _mapcount;
>                         atomic_t _refcount;
>
> And not using folio->private in the first place. Just like we did for swap.

Yes, this sounds like a much better approach.


Best Regards,
Yan, Zi