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 <[email protected]> >>> >>> 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 <[email protected]> >>> Closes: >>> https://lore.kernel.org/all/du0pr01mb10385345f7153f3341009818882...@du0pr01mb10385.eurprd01.prod.exchangelabs.com/ >>> [1] >>> Suggested-by: Vlastimil Babka <[email protected]> >>> Signed-off-by: Wandun Chen <[email protected]> >>> Link: >>> https://lore.kernel.org/linux-rt-users/[email protected]/#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
