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

[Zi Yan]

On 8 Jul 2026, at 23:31, Wandun wrote:

> 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@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxx>
>>> Closes: https://lore.kernel.org/all/DU0PR01MB10385345F7153F334100981888259A@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ [1]
>>> Suggested-by: Vlastimil Babka <vbabka@xxxxxxx>
>>> Signed-off-by: Wandun Chen <chenwandun@xxxxxxxxxxx>
>>> Link: https://lore.kernel.org/linux-rt-users/33275585-f2db-4779-89f0-3ae24b455a67@xxxxxxx/#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.
>

For this race condition, since VMA has VM_LOCKED_BIT set, maybe you can
extend rwc->invalid_vma semantics to return 1:SKIP, 0:OK, -1:STOP.
Then install an invalid_vma function when !compaction_allow_unevictable().
The invalid_vma function returns -1:STOP for any vma with VM_LOCKED_BIT set.
__rmap_walk_file() and rmap_walk_anon() will need to break when -1:STOP
is returned.

This approach will not require a new TTU flag, although it requires more
code changes.

Best Regards,
Yan, Zi