Re: [PATCH v2 2/4] mm/mlock: wait for migration to finish when mlocking a folio
From: Lorenzo Stoakes <ljs@kernel.org>
Date: 2026-07-07 14:33:51
Also in:
linux-mm, linux-rt-devel, lkml
On Tue, Jul 07, 2026 at 08:59:23PM +0800, Wandun Chen wrote:
From: Wandun Chen <redacted> In RT kernels, sysctl_compact_unevictable_allowed is false by default, when the mlock/mlockall system call try to lock all the present page, the mlock_pte_range function skips non-present entries. If these non-present entries are migration entries, and the migration is not guaranteed to have completed before the mlock/mlockall, it may result in a page fault on subsequent access, which then waits for the migration to finish, causing spike latency in RT kernels.
Is this really noticable and measurable? I suppose not too many ranges should be mock
Fix it by waiting for the migration to complete during the mlock/mlockall syscall when sysctl_compact_unevictable_allowed is false.
Probably better to reference the sysctl itself rather than an arbitrary varible name.
Fixes: 90d07210ab55 ("mm: mlock: use folios and a folio batch internally")Again I have no idea why you chose this as the fixes target, especially for this? But in any case, why is this separate from the previous commit? I sthis an entirely separate fix?
Suggested-by: Vlastimil Babka <redacted> Signed-off-by: Wandun Chen <redacted> Link: https://lore.kernel.org/lkml/c8793c0f-7156-4cb7-9e6e-7909397e2fff@kernel.org/#t (local)
A nit but please strip that #t.
quoted hunk ↗ jump to hunk
--- mm/mlock.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)diff --git a/mm/mlock.c b/mm/mlock.c index 97e49038d8d3..ac65de40b22b 100644 --- a/mm/mlock.c +++ b/mm/mlock.c@@ -25,6 +25,7 @@ #include <linux/memcontrol.h> #include <linux/mm_inline.h> #include <linux/secretmem.h> +#include <linux/compaction.h> #include "internal.h"@@ -361,8 +362,17 @@ static int mlock_pte_range(pmd_t *pmd, unsigned long addr, ptl = pmd_trans_huge_lock(pmd, vma); if (ptl) { - if (!pmd_present(*pmd)) + if (!pmd_present(*pmd)) { + if (unlikely((vma->vm_flags & VM_LOCKED) &&
-> vma_test(vma, VMA_LOCKED_BIT)... What is the basis for your unlikely()? Don't use likely()/unlikely() just because it feels right. Leave them out unless you have actual profiling data to prove it. Remove it please. But I'm also confused here - why are you checking for VMAs that already had VMA_LOCKED_BIT set when you are about to set it? Surely this shouldn't be predicated on VMA_LOCKED_BIT?
+ !compaction_allow_unevictable() &&
Why do we care about compaction here? Are we assuming the softleaf migration entry is present only for compaction reasons? So these migration entries could be there for any reason? Does it matter? Is it such a big deal, on mlock, to wait for migration entries? I think probably not. But then maybe some weird workload gets affected... hmm.
+ softleaf_is_migration(softleaf_from_pmd(*pmd)))) {Use pmd_is_migration_entry()? :) And technically you should use pmdp_get() I think? Maybe a situation where we don't care/need the READ_ONCE() though.
+ spin_unlock(ptl); + pmd_migration_entry_wait(vma->vm_mm, pmd); + walk->action = ACTION_AGAIN; + return 0; + }
This code is really disgusting, let's please not just copy/paste open-coded
nested horror shows like this.
A good rule of thumb is if you see a bunch of code 'poking out' like this and
certainly if you copy/paste it or something very much like it, break it out into
a function.
So:
static bool wait_for_migration(const struct vm_area_struct *vma, softleaf_t entry)
{quoted
quoted
quoted
As above I'm not sure should even be checking this?
if (!vma_test(vma, VMA_LOCKED_BIT)) return false;
quoted
quoted
quoted
As above I'm not sure this makes sense?
if (compaction_allowed_unevictable())
return false;
return softleaf_is_migration(entry);
}
Then call it like:
if (ptl) {
const pmd_t pmd = pmdp_get(pmd);
if (!pmd_present(pmd)) {
const softleaf_t entry = softleaf_from_pmd(pmd);
if (!wait_for_migration(vma, entry))
goto out;
spin_unlock(ptl);
pmd_migration_entry_wait(vma->vm_mm, pmd);
walk->action = ACTION_AGAIN;
return 0;
}
if (is_huge_zero_pmd(pmd))
goto out;
(etc. going *pmd -> pmd)
But we could probably do even better than that, and assuming your dubious checks
above aren't needed, we don't even need a helper then:
if (ptl) {
const pmd_t pmd = pmdp_get(pmd); // possibly just *pmd?
const softleaf_t entry = softleaf_from_pmd(pmd);
if (softleaf_is_migration(entry)) {
/* Wait for migration entries. */
spin_unlock(ptl);
pmd_migration_entry_wait(vma->vm_mm, pmd);
walk->action = ACTION_AGAIN;
return 0;
}
if (!pmd_present(pmd))
goto out;
etc.
And similar for the PTE stuff below.
This works because softleaf_from_pmd() (and pte) will give you a 'none' softleaf
if the entry is not a softlaf entry. So you can unconditionally call it on a
PMD.
quoted hunk ↗ jump to hunk
goto out; + } if (is_huge_zero_pmd(*pmd)) goto out; folio = pmd_folio(*pmd);@@ -383,8 +393,17 @@ static int mlock_pte_range(pmd_t *pmd, unsigned long addr, for (pte = start_pte; addr != end; pte++, addr += PAGE_SIZE) { ptent = ptep_get(pte); - if (!pte_present(ptent)) + if (!pte_present(ptent)) { + if (unlikely((vma->vm_flags & VM_LOCKED) && + !compaction_allow_unevictable() && + softleaf_is_migration(softleaf_from_pte(ptent)))) { + pte_unmap_unlock(start_pte, ptl); + migration_entry_wait(vma->vm_mm, pmd, addr); + walk->action = ACTION_AGAIN; + return 0; + } continue; + } folio = vm_normal_folio(vma, addr, ptent); if (!folio || folio_is_zone_device(folio)) continue; --2.43.0
Thanks, Lorenzo