Re: [PATCH v2 1/4] mm/migrate: do not migrate folios mapped into VM_LOCKED VMAs under compaction

[Wandun <chenwandun1@gmail.com>]

On 7/7/26 21:44, Lorenzo Stoakes wrote:
> (Being really nitty, your subject line is too long)
> 
> Please don't reference legacy VMA flags for newer patches 'do not migrate
> folios mapped into mlocked VMAs...' works just as well.
Got it.
> 
> On Tue, Jul 07, 2026 at 08:59:22PM +0800, Wandun Chen wrote:
>> From: Wandun Chen <chenwandun@lixiang.com>
>>
>> When compact_unevictable_allowed=0, unevictable pages should not be
>> migrated. However, mlock_folio_batch in the mlock[all] syscall introduces
>> a race, mlock_folio() sets PG_mlocked immediately but defers PG_unevictable
>> to mlock_folio_batch(), causing pages that are about to become unevictable
>> to be migrated, which violates the intent of compact_unevictable_allowed,
>> and causes spike latency in RT kernels [1].
>>
>> In order to fix this, migration is forbidden for pages mapped into VMAs
>> marked with VM_LOCKED. In addition, two early-return paths are introduced,
> 
> Please don't reference legacy VMA flags. -> VMA_LOCKED_BIT.
Got it.
> 
>> filter out mlocked pages, return early to avoid unnecessary operations.
>>
>> Fixes: 90d07210ab55 ("mm: mlock: use folios and a folio batch internally")
> 
> Hmmmm why do you think my patch caused this? That was just a folio conversion?

Oh, I made a mistake, your patch was just a folio conversion.

Batching mlocked page was introduced in v5.18 by:
	commit 2fbb0c10d1e8 ("mm/munlock: mlock_page() munlock_page() batch by pagevec")

Setting sysctl_compact_unevictable_allowed to zero was introduced in v5.7 by:
	commit 6923aa0d8c62 ("mm/compaction: Disable compact_unevictable_allowed on RT")

So this issue exist after v5.18.

> 
> Also I didn't think we liked having fixes spotted about a series with non-fixes
> tags?
Got it, will split this series in next version.
> 
>> Reported-by: Alexander Krabler <Alexander.Krabler@kuka.com>
>> Closes: https://lore.kernel.org/all/DU0PR01MB10385345F7153F334100981888259A@DU0PR01MB10385.eurprd01.prod.exchangelabs.com/ [1]
>> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
>> Signed-off-by: Wandun Chen <chenwandun@lixiang.com>
>> Link: https://lore.kernel.org/linux-rt-users/33275585-f2db-4779-89f0-3ae24b455a67@suse.cz/#t
>> ---
>>  include/linux/compaction.h |  6 ++++++
>>  include/linux/rmap.h       |  3 +++
>>  mm/compaction.c            |  8 +++++++-
>>  mm/migrate.c               | 23 +++++++++++++++++++----
>>  mm/rmap.c                  | 12 +++++++++---
>>  5 files changed, 44 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
>> index f29ef0653546..04e60f65b976 100644
>> --- a/include/linux/compaction.h
>> +++ b/include/linux/compaction.h
>> @@ -106,6 +106,7 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
>>  extern void __meminit kcompactd_run(int nid);
>>  extern void __meminit kcompactd_stop(int nid);
>>  extern void wakeup_kcompactd(pg_data_t *pgdat, int order, int highest_zoneidx);
>> +extern bool compaction_allow_unevictable(void);
> 
> Don't use extern. We remove extern as we go it's not needed.
Got it.
> 
>>
>>  #else
>>  static inline void reset_isolation_suitable(pg_data_t *pgdat)
>> @@ -131,6 +132,11 @@ static inline void wakeup_kcompactd(pg_data_t *pgdat,
>>  {
>>  }
>>
>> +static inline bool compaction_allow_unevictable(void)
>> +{
>> +	return true;
>> +}
>> +
>>  #endif /* CONFIG_COMPACTION */
>>
>>  struct node;
>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
>> index 8dc0871e5f00..359c7426b6b9 100644
>> --- a/include/linux/rmap.h
>> +++ b/include/linux/rmap.h
>> @@ -102,6 +102,9 @@ enum ttu_flags {
>>  					 * do a final flush if necessary */
>>  	TTU_RMAP_LOCKED		= 0x80,	/* do not grab rmap lock:
>>  					 * caller holds it */
>> +	TTU_RESPECT_MLOCK	= 0x100,/* leave VM_LOCKED vmas mapped instead
> 
> -> VMA_LOCKED_BIT please. Also maybe just say mlock'd?
Got it.
> 
>> +					 * of installing a migration entry
>> +					 */
>>  };
>>
>>  #ifdef CONFIG_MMU
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index f08765ade014..5d256930e389 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1116,7 +1116,8 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>>  		is_unevictable = folio_test_unevictable(folio);
>>
>>  		/* Compaction might skip unevictable pages but CMA takes them */
>> -		if (!(mode & ISOLATE_UNEVICTABLE) && is_unevictable)
>> +		if (!(mode & ISOLATE_UNEVICTABLE) &&
>> +		    (is_unevictable || folio_test_mlocked(folio)))
> 
> Maybe just change is_unevictable to include this check?
> 
> Like:
> 
> 	is_unevictable = folio_test_unevictable(folio) ||
> 		folio_test_mlocked(folio);
> 
> ?
Sounds good, I'll fold mlock into is_unevictable so both checks
stay consistent.
> 
> Also later you have:
> 
> 		if (((mode & ISOLATE_ASYNC_MIGRATE) && is_dirty) ||
> 		    (mapping && is_unevictable)) {
> 		    	...
> 
> Which doesn't account for mlock as-is? Is that correct?
IIUC, The is_unevictable check here mainly serves as a cheap pre-filter
for inaccessible mappings (which are always unevictable folio).
It's unrelated to the mlock.

> 
> 
> 
>>  			goto isolate_fail_put;
>>
>>  		/*
>> @@ -1898,6 +1899,11 @@ typedef enum {
>>   * compactable pages.
>>   */
>>  static int sysctl_compact_unevictable_allowed __read_mostly = CONFIG_COMPACT_UNEVICTABLE_DEFAULT;
>> +
>> +bool compaction_allow_unevictable(void)
>> +{
>> +	return sysctl_compact_unevictable_allowed;
>> +}
> 
> You add this helper but isolate_migratepages() still references
> sysctl_compact_unevictable_allowed directly?
I'll route that reference through compaction_allow_unevictable()
in next version.
> 
>>  /*
>>   * Tunable for proactive compaction. It determines how
>>   * aggressively the kernel should compact memory in the
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index a786549551e3..3a15eb13e82b 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1202,7 +1202,7 @@ static void migrate_folio_done(struct folio *src,
>>  static int migrate_folio_unmap(new_folio_t get_new_folio,
>>  		free_folio_t put_new_folio, unsigned long private,
>>  		struct folio *src, struct folio **dstp, enum migrate_mode mode,
>> -		struct list_head *ret)
>> +		struct list_head *ret, enum migrate_reason reason)
>>  {
>>  	struct folio *dst;
>>  	int rc = -EAGAIN;
>> @@ -1210,6 +1210,7 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>  	struct anon_vma *anon_vma = NULL;
>>  	bool locked = false;
>>  	bool dst_locked = false;
>> +	enum ttu_flags ttu = 0;
> 
> You reference ttu only in an if-block below no? So why are you declaring
> this here? Move it to the if-block.
Got it. I'll move it to the if-block in next version.
> 
>>
>>  	dst = get_new_folio(src, private);
>>  	if (!dst)
>> @@ -1249,9 +1250,15 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>  		folio_lock(src);
>>  	}
>>  	locked = true;
>> -	if (folio_test_mlocked(src))
>> +	if (folio_test_mlocked(src)) {
>>  		old_folio_state |= FOLIO_WAS_MLOCKED;
>>
>> +		if (reason == MR_COMPACTION && !compaction_allow_unevictable()) {
> 
> This should really be a helper since you repeat yourself and it's not
> obvious what this is checking.
> 
> Like:
> 
> 	static migrate_mlock_allowed(enum migrate_reason reason)
> 	{
> 		/* Only compaction is disallowed. */
> 		if (reason != MR_COMPACTION)
> 			return true;
> 
> 		/* If we can compact unevictable folios, we are ok. */
> 		if (compaction_allow_unevictable())
> 			return true;
> 
> 		/* Conservative: if any folio could be mlock()'d, disallow. */
> 		return false;
> 	}
> 
> Then you could self-document what you're checking and avoid code duplication below.
Got it, it is more clear, thanks.
> 
>> +			rc = -EBUSY;
>> +			goto out;
>> +		}
>> +	}
>> +
>>  	if (folio_test_writeback(src)) {
>>  		/*
>>  		 * Only in the case of a full synchronous migration is it
>> @@ -1324,7 +1331,14 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
>>  		/* Establish migration ptes */
>>  		VM_BUG_ON_FOLIO(folio_test_anon(src) &&
>>  			       !folio_test_ksm(src) && !anon_vma, src);
> 
> Useful to convert VM_BUG_*() -> VM_WARN_*() (possibly _ONCE() here also) as we go!
Got it.
> 
>> -		try_to_migrate(src, mode == MIGRATE_ASYNC ? TTU_BATCH_FLUSH : 0);
>> +
>> +		if (mode == MIGRATE_ASYNC)
>> +			ttu |= TTU_BATCH_FLUSH;
>> +
>> +		if (reason == MR_COMPACTION && !compaction_allow_unevictable())
> 
> See above about deduplicating.
> 
>> +			ttu |= TTU_RESPECT_MLOCK;
> 
> Hmm. I don't love 'respect mlock'. I guess we only know about the reason
> being compaction here.
Right, we only know about the reason.
> 
> But I'm confused anyway. We have the folio, why aren't we just checking for
> PG_mlocked() here instead of getting the rmap to see if it's mapped
> anywhere with VMA_LOCKED_BIT?

There was a race scenario without patch 02, vma may already marked with
VMA_LOCKED_BIT but folio has't marked with mlocked, such as below:


CPUA: mlock()                                  		CPUB: compaction / migration

mmap_write_lock()
    mlock_fixup set VM_CLOKED
    mlock_pte_range
    	mlock_folio(page N)

                                             		isolate page N+50 (mlock hasn't reached it)
                                            		migrate_folio_unmap
                                       			folio_test_mlocked() --> false, but VMA:VM_LOCKED
                                       			try_to_migrate()
                                         			rmap_walk(P) [anon_vma rwsem / i_mmap_rwsem, read]
                                           			try_to_migrate_one -->install migration entry
        ...reaches page N+50
        skip migration entry (without patch 02)
mmap_write_unlock()


access page N + 50 --> wait for migration complete
							
							migrate_folios_move
							....
                                             		migrate complete



If apply patch 02, mlock itself will wait migration complete, so checking
VMA_LOCKED_BIT in the rmap path is no longer necessary, but this logic is
retained to avoid unnecessary migration operations.


> 
>> +
>> +		try_to_migrate(src, ttu);
>>  		old_folio_state |= FOLIO_WAS_MAPPED;
>>  	}
>>
>> @@ -1905,7 +1919,8 @@ static int migrate_pages_batch(struct list_head *from,
>>  			}
>>
>>  			rc = migrate_folio_unmap(get_new_folio, put_new_folio,
>> -					private, folio, &dst, mode, ret_folios);
>> +					private, folio, &dst, mode, ret_folios,
>> +					reason);
>>  			/*
>>  			 * The rules are:
>>  			 *	0: folio will be put on unmap_folios list,
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 0fb7a1b82cf3..3cb7f6337d38 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -2420,6 +2420,9 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>>  	unsigned long pfn;
>>  	unsigned long hsz = 0;
>>
>> +	if ((flags & TTU_RESPECT_MLOCK) && (vma->vm_flags & VM_LOCKED))
> 
> Please use the modern API for VMA flags. So:
> 
> 	if ((flags & TTU_RESPECT_MLOCK) && vma_test(VMA_LOCKED_BIT))
Got it.
> 
>> +		return false;
>> +
>>  	/*
>>  	 * When racing against e.g. zap_pte_range() on another cpu,
>>  	 * in between its ptep_get_and_clear_full() and folio_remove_rmap_*(),
>> @@ -2741,11 +2744,14 @@ void try_to_migrate(struct folio *folio, enum ttu_flags flags)
>>  	};
>>
>>  	/*
>> -	 * Migration always ignores mlock and only supports TTU_RMAP_LOCKED and
>> -	 * TTU_SPLIT_HUGE_PMD, TTU_SYNC, and TTU_BATCH_FLUSH flags.
>> +	 * Migration normally ignores mlock, but TTU_RESPECT_MLOCK asks it to
>> +	 * leave folios mapped into VM_LOCKED vmas alone.  Only TTU_RMAP_LOCKED,
> 
> Again -> VMA_LOCKED_BIT.
> 
>> +	 * TTU_SPLIT_HUGE_PMD, TTU_SYNC, TTU_BATCH_FLUSH and TTU_RESPECT_MLOCK
>> +	 * are supported.
>>  	 */
>>  	if (WARN_ON_ONCE(flags & ~(TTU_RMAP_LOCKED | TTU_SPLIT_HUGE_PMD |
>> -					TTU_SYNC | TTU_BATCH_FLUSH)))
>> +					TTU_SYNC | TTU_BATCH_FLUSH |
>> +					TTU_RESPECT_MLOCK)))
>>  		return;
>>
>>  	if (folio_is_zone_device(folio) &&
>> --
>> 2.43.0
>>
> 
> Thanks, Lorenzo

Thank you for the detailed review, Lorenzo.
many of the comments are very meaningful :)


Best regards
Wandun