Re: [PATCH 3/7] mm/migrate: skip data copy for already-copied folios

[Garg, Shivank]

On 5/20/2026 8:51 PM, Garg, Shivank wrote:
> 
> 
> On 5/11/2026 9:05 PM, David Hildenbrand (Arm) wrote:
>> On 4/28/26 17:50, Shivank Garg wrote:
>>> Add a FOLIO_ALREADY_COPIED flag to the dst->migrate_info migration
>>> state. When set, __migrate_folio() skips folio_mc_copy() and
>>> performs metadata-only migration. All callers currently pass
>>> already_copied=false. The batch-copy path enables it later in a
>>> subsequent patch.
>>>
>>> Move the dst->migrate_info state enum earlier in the file so
>>> __migrate_folio() and move_to_new_folio() can see FOLIO_ALREADY_COPIED.
>>>
>>> Signed-off-by: Shivank Garg <shivankg@xxxxxxx>
>>> ---
>>>  mm/migrate.c | 53 +++++++++++++++++++++++++++++++---------------------
>>>  1 file changed, 32 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 03c2a6f7e5e4..c493e67e359d 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -850,6 +850,19 @@ void folio_migrate_flags(struct folio *newfolio, struct folio *folio)
>>>  }
>>>  EXPORT_SYMBOL(folio_migrate_flags);
>>>  
>>> +/*
>>> + * To record some information during migration, we use the migrate_info
>>> + * field of struct folio of the newly allocated destination folio.
>>> + * This is safe because nobody is using it except us.
>>> + */
>>> +enum {
>>> +	FOLIO_WAS_MAPPED = BIT(0),
>>> +	FOLIO_WAS_MLOCKED = BIT(1),
>>> +	FOLIO_ALREADY_COPIED = BIT(2),
>>
>> I wonder whether we want to talk about "folio content copied", to not confuse it
>> with folio flags copied etc.
>>
>> FOLIO_CONTENT_COPIED.
>>
>> Thoughts?
> 
> Good point, this is more precise.
> 

Sashiko Comment:
Does expanding FOLIO_OLD_STATES to include BIT(2) silently corrupt anon_vma
pointers on 32-bit architectures?
The anon_vma pointer is packed with FOLIO_OLD_STATES inside dst->migrate_info.
To separate them, __migrate_folio_extract() uses info & ~FOLIO_OLD_STATES.
Since anon_vma objects are allocated from a kmem_cache with align=0,
ARCH_SLAB_MINALIGN defaults to 4 bytes on many 32-bit architectures. This
means a valid anon_vma pointer can legitimately end in 0x4 or 0xC (meaning
bit 2 is set).
When __migrate_folio_extract() masks the pointer with ~7, it will silently
clear bit 2 from the anon_vma pointer. Any subsequent call to put_anon_vma()
with this corrupted pointer could cause a use-after-free or a kernel panic.
--

#define ARCH_SLAB_MINALIGN __alignof__(unsigned long long)

I initially assumed this to be always 8, confusing it to be same as size of
unsigned long long.
But the GCC docs note that alignment can be smaller in size:

https://gcc.gnu.org/onlinedocs/gcc/Alignment.html
"For example, if the target machine requires a double value to be aligned on
an 8-byte boundary, then __alignof__ (double) is 8. This is true on many RISC
machines. On more traditional machine designs, __alignof__ (double) is 4 or
even 2." 

If my understanding is right, Sashiko concern is valid, and I can't safely
use BIT(2).
I see few option from here. Either I can gate batch copy for CONFIG_64BIT,
or I can force anon_vma_cachep to 8-byte alignment via kmem_cache_create()
align arg. Or I can change the migrate_folio() callback to pass already_copied
info to change to dst->migrate_info enum.

I'm not sure if touching the anon_vma cache would be preferred? Gating the change
to CONFIG_64BIT feels less risky to me.

Thanks,
Shivank



>>> +	FOLIO_OLD_STATES = FOLIO_WAS_MAPPED | FOLIO_WAS_MLOCKED |
>>> +			   FOLIO_ALREADY_COPIED,
>>> +};
>>> +
>>>  /************************************************************
>>>   *                    Migration functions
>>>   ***********************************************************/
>>> @@ -859,14 +872,20 @@ static int __migrate_folio(struct address_space *mapping, struct folio *dst,
>>>  			   enum migrate_mode mode)
>>>  {
>>>  	int rc, expected_count = folio_expected_ref_count(src) + 1;
>>> +	bool already_copied = (dst->migrate_info & FOLIO_ALREADY_COPIED);
>>
>> const, and no need for ().
> 
> Will fix.
> 
>>> +
>>> +	if (already_copied)
>>> +		dst->migrate_info = 0;
>>
>> Hm, why is that required? Might deserve a comment.
>>
>> Likely you want to clear the "already copied" marker?
>>
>> 	dst->migrate_info &= ~FOLIO_ALREADY_COPIED;
>>
>> ?
> 
> Yes,
> In current code, migrate_info only carry FOLIO_ALREADY_COPIED.
> So, =0 and &=~ are equivalent and similar for |= case. I mirrored the
> how anon_vma/old_folio_state get handled __migrate_folio_extract().
> 
> But with your other suggestion of marking already_copied immediately
> after folio_mc_copy(), then bitwise clear and set will be required.
> I'll do this.
> 
> 
> 
>> But I wonder if this really belongs exactly here.
>>
> 
> I did not understand this.
> 
>  
>>>  
>>>  	/* Check whether src does not have extra refs before we do more work */
>>>  	if (folio_ref_count(src) != expected_count)
>>>  		return -EAGAIN;
>>>  
>>> -	rc = folio_mc_copy(dst, src);
>>> -	if (unlikely(rc))
>>> -		return rc;
>>> +	if (!already_copied) {
>>> +		rc = folio_mc_copy(dst, src);
>>> +		if (unlikely(rc))
>>> +			return rc;
>>> +	}
>>>  
>>>  	rc = __folio_migrate_mapping(mapping, dst, src, expected_count);
>>>  	if (rc)
>>> @@ -1090,7 +1109,7 @@ static int fallback_migrate_folio(struct address_space *mapping,
>>>   *     0 - success
>>>   */
>>>  static int move_to_new_folio(struct folio *dst, struct folio *src,
>>> -				enum migrate_mode mode)
>>> +		enum migrate_mode mode, bool already_copied)
>>>  {
>>>  	struct address_space *mapping = folio_mapping(src);
>>>  	int rc = -EAGAIN;
>>> @@ -1098,6 +1117,9 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
>>>  	VM_BUG_ON_FOLIO(!folio_test_locked(src), src);
>>>  	VM_BUG_ON_FOLIO(!folio_test_locked(dst), dst);
>>>  
>>> +	if (already_copied)
>>> +		dst->migrate_info = FOLIO_ALREADY_COPIED;
>>
>> |= ?
>>
> 
> Will do.
>  
>>> +
>>>  	if (!mapping)
>>>  		rc = migrate_folio(mapping, dst, src, mode);
>>>  	else if (mapping_inaccessible(mapping))
>>> @@ -1129,17 +1151,6 @@ static int move_to_new_folio(struct folio *dst, struct folio *src,
>>>  	return rc;
>>>  }
>>>  
>>> -/*
>>> - * To record some information during migration, we use the migrate_info
>>> - * field of struct folio of the newly allocated destination folio.
>>> - * This is safe because nobody is using it except us.
>>> - */
>>> -enum {
>>> -	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_folio_state, struct anon_vma *anon_vma)
>>>  {
>>> @@ -1353,7 +1364,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>>  static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
>>>  			      struct folio *src, struct folio *dst,
>>>  			      enum migrate_mode mode, enum migrate_reason reason,
>>> -			      struct list_head *ret)
>>> +			      struct list_head *ret, bool already_copied)
>>>  {
>>>  	int rc;
>>>  	int old_folio_state = 0;
>>> @@ -1379,7 +1390,7 @@ static int migrate_folio_move(free_folio_t put_new_folio, unsigned long private,
>>>  		src_partially_mapped = folio_test_partially_mapped(src);
>>>  	}
>>>  
>>> -	rc = move_to_new_folio(dst, src, mode);
>>> +	rc = move_to_new_folio(dst, src, mode, already_copied);
>>>  	if (rc)
>>>  		goto out;
>>>  
>>> @@ -1536,7 +1547,7 @@ static int unmap_and_move_huge_page(new_folio_t get_new_folio,
>>>  	}
>>>  
>>>  	if (!folio_mapped(src))
>>> -		rc = move_to_new_folio(dst, src, mode);
>>> +		rc = move_to_new_folio(dst, src, mode, false);
>>
>> ... mode, /* already_copied = */ false
>>
> 
> will fix
> 
>>>  
>>>  	if (page_was_mapped)
>>>  		remove_migration_ptes(src, !rc ? dst : src, ttu);
>>> @@ -1720,7 +1731,7 @@ static void migrate_folios_move(struct list_head *src_folios,
>>>  		struct list_head *ret_folios,
>>>  		struct migrate_pages_stats *stats,
>>>  		int *retry, int *thp_retry, int *nr_failed,
>>> -		int *nr_retry_pages)
>>> +		int *nr_retry_pages, bool already_copied)
>>>  {
>>>  	struct folio *folio, *folio2, *dst, *dst2;
>>>  	bool is_thp;
>>> @@ -1737,7 +1748,7 @@ static void migrate_folios_move(struct list_head *src_folios,
>>>  
>>>  		rc = migrate_folio_move(put_new_folio, private,
>>>  				folio, dst, mode,
>>> -				reason, ret_folios);
>>> +				reason, ret_folios, already_copied);
>>>  		/*
>>>  		 * The rules are:
>>>  		 *	0: folio will be freed
>>> @@ -1994,7 +2005,7 @@ static int migrate_pages_batch(struct list_head *from,
>>>  		migrate_folios_move(&unmap_folios, &dst_folios,
>>>  				put_new_folio, private, mode, reason,
>>>  				ret_folios, stats, &retry, &thp_retry,
>>> -				&nr_failed, &nr_retry_pages);
>>> +				&nr_failed, &nr_retry_pages, false);
>>>  	}
>>
>> dito.
>>
> 
> will fix.
> 
> Thanks,
> Shivank