These flags only track folio-specific state during migration and are
not used for movable_ops pages. Rename the enum values and the
old_page_state variable to match.

No functional change.

Suggested-by: David Hildenbrand <david@kernel.org>
Signed-off-by: Shivank Garg <shivankg@amd.com>
---

Applies cleanly on mm-new (02b045682c74).

v1: https://lore.kernel.org/all/20260323141935.389232-3-shivankg@amd.com

v2:
- Rename FOLIO_MF_* to FOLIO_*, per feedback from Willy.

 mm/migrate.c | 46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 05cb408846f2..7dd6c2f2e1ef 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1135,26 +1135,26 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
  * This is safe because nobody is using it except us.
  */
 enum {
-	PAGE_WAS_MAPPED = BIT(0),
-	PAGE_WAS_MLOCKED = BIT(1),
-	PAGE_OLD_STATES = PAGE_WAS_MAPPED | PAGE_WAS_MLOCKED,
+	FOLIO_WAS_MAPPED = BIT(0),
+	FOLIO_WAS_MLOCKED = BIT(1),
+	FOLIO_OLD_STATES = FOLIO_WAS_MAPPED | FOLIO_WAS_MLOCKED,
 };
 
 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;
 }
 
@@ -1209,7 +1209,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
 {
 	struct folio *dst;
 	int rc = -EAGAIN;
-	int old_page_state = 0;
+	int old_folio_state = 0;
 	struct anon_vma *anon_vma = NULL;
 	bool locked = false;
 	bool dst_locked = false;
@@ -1253,7 +1253,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
 	}
 	locked = true;
 	if (folio_test_mlocked(src))
-		old_page_state |= PAGE_WAS_MLOCKED;
+		old_folio_state |= FOLIO_WAS_MLOCKED;
 
 	if (folio_test_writeback(src)) {
 		/*
@@ -1302,7 +1302,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
 	dst_locked = true;
 
 	if (unlikely(page_has_movable_ops(&src->page))) {
-		__migrate_folio_record(dst, old_page_state, anon_vma);
+		__migrate_folio_record(dst, old_folio_state, anon_vma);
 		return 0;
 	}
 
@@ -1328,11 +1328,11 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
 		VM_BUG_ON_FOLIO(folio_test_anon(src) &&
 			       !folio_test_ksm(src) && !anon_vma, src);
 		try_to_migrate(src, mode == MIGRATE_ASYNC ? TTU_BATCH_FLUSH : 0);
-		old_page_state |= PAGE_WAS_MAPPED;
+		old_folio_state |= FOLIO_WAS_MAPPED;
 	}
 
 	if (!folio_mapped(src)) {
-		__migrate_folio_record(dst, old_page_state, anon_vma);
+		__migrate_folio_record(dst, old_folio_state, anon_vma);
 		return 0;
 	}
 
@@ -1344,7 +1344,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
 	if (rc == -EAGAIN)
 		ret = NULL;
 
-	migrate_folio_undo_src(src, old_page_state & PAGE_WAS_MAPPED,
+	migrate_folio_undo_src(src, old_folio_state & FOLIO_WAS_MAPPED,
 			       anon_vma, locked, ret);
 	migrate_folio_undo_dst(dst, dst_locked, put_new_folio, private);
 
@@ -1358,13 +1358,13 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
 			      struct list_head *ret)
 {
 	int rc;
-	int old_page_state = 0;
+	int old_folio_state = 0;
 	struct anon_vma *anon_vma = NULL;
 	bool src_deferred_split = false;
 	bool src_partially_mapped = false;
 	struct list_head *prev;
 
-	__migrate_folio_extract(dst, &old_page_state, &anon_vma);
+	__migrate_folio_extract(dst, &old_folio_state, &anon_vma);
 	prev = dst->lru.prev;
 	list_del(&dst->lru);
 
@@ -1395,10 +1395,10 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
 	 * isolated from the unevictable LRU: but this case is the easiest.
 	 */
 	folio_add_lru(dst);
-	if (old_page_state & PAGE_WAS_MLOCKED)
+	if (old_folio_state & FOLIO_WAS_MLOCKED)
 		lru_add_drain();
 
-	if (old_page_state & PAGE_WAS_MAPPED)
+	if (old_folio_state & FOLIO_WAS_MAPPED)
 		remove_migration_ptes(src, dst, 0);
 
 	/*
@@ -1439,11 +1439,11 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
 	 */
 	if (rc == -EAGAIN) {
 		list_add(&dst->lru, prev);
-		__migrate_folio_record(dst, old_page_state, anon_vma);
+		__migrate_folio_record(dst, old_folio_state, anon_vma);
 		return rc;
 	}
 
-	migrate_folio_undo_src(src, old_page_state & PAGE_WAS_MAPPED,
+	migrate_folio_undo_src(src, old_folio_state & FOLIO_WAS_MAPPED,
 			       anon_vma, true, ret);
 	migrate_folio_undo_dst(dst, true, put_new_folio, private);
 
@@ -1777,11 +1777,11 @@ static void migrate_folios_undo(struct list_head *src_folios,
 	dst = list_first_entry(dst_folios, struct folio, lru);
 	dst2 = list_next_entry(dst, lru);
 	list_for_each_entry_safe(folio, folio2, src_folios, lru) {
-		int old_page_state = 0;
+		int old_folio_state = 0;
 		struct anon_vma *anon_vma = NULL;
 
-		__migrate_folio_extract(dst, &old_page_state, &anon_vma);
-		migrate_folio_undo_src(folio, old_page_state & PAGE_WAS_MAPPED,
+		__migrate_folio_extract(dst, &old_folio_state, &anon_vma);
+		migrate_folio_undo_src(folio, old_folio_state & FOLIO_WAS_MAPPED,
 				anon_vma, true, ret_folios);
 		list_del(&dst->lru);
 		migrate_folio_undo_dst(dst, true, put_new_folio, private);

base-commit: 02b045682c74be16c7d1501563f02b0e92d42cdb
-- 
2.43.0

On 24 Mar 2026, at 7:47, Shivank Garg wrote:

> These flags only track folio-specific state during migration and are
> not used for movable_ops pages. Rename the enum values and the
> old_page_state variable to match.
>
> No functional change.
>
> Suggested-by: David Hildenbrand <david@kernel.org>
> Signed-off-by: Shivank Garg <shivankg@amd.com>
> ---
>
> Applies cleanly on mm-new (02b045682c74).
>
> v1: https://lore.kernel.org/all/20260323141935.389232-3-shivankg@amd.com
>
> v2:
> - Rename FOLIO_MF_* to FOLIO_*, per feedback from Willy.
>
>  mm/migrate.c | 46 +++++++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 05cb408846f2..7dd6c2f2e1ef 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1135,26 +1135,26 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
>   * This is safe because nobody is using it except us.
>   */
>  enum {
> -	PAGE_WAS_MAPPED = BIT(0),
> -	PAGE_WAS_MLOCKED = BIT(1),
> -	PAGE_OLD_STATES = PAGE_WAS_MAPPED | PAGE_WAS_MLOCKED,
> +	FOLIO_WAS_MAPPED = BIT(0),
> +	FOLIO_WAS_MLOCKED = BIT(1),
> +	FOLIO_OLD_STATES = FOLIO_WAS_MAPPED | FOLIO_WAS_MLOCKED,
>  };
>
>  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.

LGTM.

Reviewed-by: Zi Yan <ziy@nvidia.com>


Best Regards,
Yan, Zi

>>
>>  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);
 }
 

Thanks,
Shivank

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

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

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

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.

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.

-- 
Cheers,

David

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

On 3/25/26 16:00, Zi Yan wrote:
> On 25 Mar 2026, at 10:53, David Hildenbrand (Arm) wrote:
> 
>> On 3/25/26 15:21, Zi Yan wrote:
>>>
>>>
>>> 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().

Likely we should strive towards only using folio->private (and the API)
really for fs-private data (i.e., the pagemap.h interface), and add
proper custom members for all other use cases.

For page->private it's a different discussion (requires more work I
guess, because there are many more use cases.

-- 
Cheers,

David

On 25 Mar 2026, at 11:04, David Hildenbrand (Arm) wrote:

> On 3/25/26 16:00, Zi Yan wrote:
>> On 25 Mar 2026, at 10:53, David Hildenbrand (Arm) wrote:
>>
>>> On 3/25/26 15:21, Zi Yan wrote:
>>>>
>>>>
>>>> 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().
>
> Likely we should strive towards only using folio->private (and the API)
> really for fs-private data (i.e., the pagemap.h interface), and add
> proper custom members for all other use cases.
>
> For page->private it's a different discussion (requires more work I
> guess, because there are many more use cases.
>
Makes sense to me.

Best Regards,
Yan, Zi

On Wed, Mar 25, 2026 at 11:05:44AM -0400, Zi Yan wrote:
> On 25 Mar 2026, at 11:04, David Hildenbrand (Arm) wrote:
> 
> > On 3/25/26 16:00, Zi Yan wrote:
> >> On 25 Mar 2026, at 10:53, David Hildenbrand (Arm) wrote:
> >>
> >>> On 3/25/26 15:21, Zi Yan wrote:
> >>>>
> >>>>
> >>>> 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().
> >
> > Likely we should strive towards only using folio->private (and the API)
> > really for fs-private data (i.e., the pagemap.h interface), and add
> > proper custom members for all other use cases.
> >
> > For page->private it's a different discussion (requires more work I
> > guess, because there are many more use cases.
> >
> Makes sense to me.

The long term plan ...

 - Remove PG_private (we're pretty close actually).  That kills off
   folio_set_private() / folio_clear_private()
 - Reimplement folio_test_private().  It just checks whether folio->private is
   NULL.
 - Remove folio_get_private().  It's actually longer than just using
   folio->private and offers no advantages.

On 3/24/2026 7:08 PM, Zi Yan wrote:
> On 24 Mar 2026, at 7:47, Shivank Garg wrote:
> 
>> These flags only track folio-specific state during migration and are
>> not used for movable_ops pages. Rename the enum values and the
>> old_page_state variable to match.
>>
>> No functional change.
>>
>> Suggested-by: David Hildenbrand <david@kernel.org>
>> Signed-off-by: Shivank Garg <shivankg@amd.com>
>> ---
>>
>> Applies cleanly on mm-new (02b045682c74).
>>
>> v1: https://lore.kernel.org/all/20260323141935.389232-3-shivankg@amd.com
>>
>> v2:
>> - Rename FOLIO_MF_* to FOLIO_*, per feedback from Willy.
>>
>>  mm/migrate.c | 46 +++++++++++++++++++++++-----------------------
>>  1 file changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 05cb408846f2..7dd6c2f2e1ef 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1135,26 +1135,26 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
>>   * This is safe because nobody is using it except us.
>>   */
>>  enum {
>> -	PAGE_WAS_MAPPED = BIT(0),
>> -	PAGE_WAS_MLOCKED = BIT(1),
>> -	PAGE_OLD_STATES = PAGE_WAS_MAPPED | PAGE_WAS_MLOCKED,
>> +	FOLIO_WAS_MAPPED = BIT(0),
>> +	FOLIO_WAS_MLOCKED = BIT(1),
>> +	FOLIO_OLD_STATES = FOLIO_WAS_MAPPED | FOLIO_WAS_MLOCKED,
>>  };
>>
>>  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.
> 

Yeah, using folio_get_private() and folio_change_private() would be cleaner.
I'll keep it in mind as a follow-up cleanup but leaving this patch as a pure rename for now.

Thanks for suggestion.

> LGTM.
> 
> Reviewed-by: Zi Yan <ziy@nvidia.com>

Thank you :)

Best regards,
Shivank

On 3/24/26 12:47, Shivank Garg wrote:
> These flags only track folio-specific state during migration and are
> not used for movable_ops pages. Rename the enum values and the
> old_page_state variable to match.
> 
> No functional change.
> 
> Suggested-by: David Hildenbrand <david@kernel.org>
> Signed-off-by: Shivank Garg <shivankg@amd.com>
> ---
> 
> Applies cleanly on mm-new (02b045682c74).
> 
> v1: https://lore.kernel.org/all/20260323141935.389232-3-shivankg@amd.com
> 
> v2:
> - Rename FOLIO_MF_* to FOLIO_*, per feedback from Willy.
> 
>  mm/migrate.c | 46 +++++++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 05cb408846f2..7dd6c2f2e1ef 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1135,26 +1135,26 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
>   * This is safe because nobody is using it except us.
>   */
>  enum {
> -	PAGE_WAS_MAPPED = BIT(0),
> -	PAGE_WAS_MLOCKED = BIT(1),
> -	PAGE_OLD_STATES = PAGE_WAS_MAPPED | PAGE_WAS_MLOCKED,
> +	FOLIO_WAS_MAPPED = BIT(0),
> +	FOLIO_WAS_MLOCKED = BIT(1),
> +	FOLIO_OLD_STATES = FOLIO_WAS_MAPPED | FOLIO_WAS_MLOCKED,
>  };
>  
>  static void __migrate_folio_record(struct folio *dst,
> -				   int old_page_state,
> +				   int old_folio_state,
>  				   struct anon_vma *anon_vma)
>  {

While at it, you could just use two tabs to indent the second parameter
line.



> -	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)
>  {

Same here.


Thanks!

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David

On 3/24/2026 6:01 PM, David Hildenbrand (Arm) wrote:
> On 3/24/26 12:47, Shivank Garg wrote:
>> These flags only track folio-specific state during migration and are
>> not used for movable_ops pages. Rename the enum values and the
>> old_page_state variable to match.
>>
>> No functional change.
>>
>> Suggested-by: David Hildenbrand <david@kernel.org>
>> Signed-off-by: Shivank Garg <shivankg@amd.com>
>> ---
>>
>> Applies cleanly on mm-new (02b045682c74).
>>
>> v1: https://lore.kernel.org/all/20260323141935.389232-3-shivankg@amd.com
>>
>> v2:
>> - Rename FOLIO_MF_* to FOLIO_*, per feedback from Willy.
>>
>>  mm/migrate.c | 46 +++++++++++++++++++++++-----------------------
>>  1 file changed, 23 insertions(+), 23 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 05cb408846f2..7dd6c2f2e1ef 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1135,26 +1135,26 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
>>   * This is safe because nobody is using it except us.
>>   */
>>  enum {
>> -	PAGE_WAS_MAPPED = BIT(0),
>> -	PAGE_WAS_MLOCKED = BIT(1),
>> -	PAGE_OLD_STATES = PAGE_WAS_MAPPED | PAGE_WAS_MLOCKED,
>> +	FOLIO_WAS_MAPPED = BIT(0),
>> +	FOLIO_WAS_MLOCKED = BIT(1),
>> +	FOLIO_OLD_STATES = FOLIO_WAS_MAPPED | FOLIO_WAS_MLOCKED,
>>  };
>>  
>>  static void __migrate_folio_record(struct folio *dst,
>> -				   int old_page_state,
>> +				   int old_folio_state,
>>  				   struct anon_vma *anon_vma)
>>  {
> 
> While at it, you could just use two tabs to indent the second parameter
> line.
> 

Sure, will fix this.

> 
> 
>> -	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)
>>  {
> 
> Same here.
> 
> 
> Thanks!
> 
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
> 

Thanks,
Shivank