On 7/7/26 22:33, Lorenzo Stoakes wrote:
> On Tue, Jul 07, 2026 at 08:59:23PM +0800, Wandun Chen wrote:
>> From: Wandun Chen <[email protected]>
>>
>> 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?
Alexander Krabler reported a 1.1ms latency in [1]
https://lore.kernel.org/all/du0pr01mb10385345f7153f3341009818882...@du0pr01mb10385.eurprd01.prod.exchangelabs.com/
>
> 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.
Got it.
>
>>
>> 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?
You're right, the Fixes tag was wrong.
Setting sysctl_compact_unevictable_allowed to zero was introduced in v5.7 by:
commit 6923aa0d8c62 ("mm/compaction: Disable
compact_unevictable_allowed on RT")
Before this commit, mlocked page can be migrated, and dont need to wait for
migration complete.
After this commit, if don't wait for migration complete, this may result in
latency spikes.
(The batch mlock feature increases the probability of hitting migration
entries.)
Will update Fixes tag in next version.
>
> But in any case, why is this separate from the previous commit? I sthis an
> entirely separate fix?
These two patches address two aspects of the same problem,
and I split them into separate patches mainly to make review easier.
Patch 1 mainly prevents pages to be migrated due to batch mlock;
patch 2 mainly ensures that pages undergoing migration have finished
migrating once the mlock system call completes.
>
>> Suggested-by: Vlastimil Babka <[email protected]>
>> Signed-off-by: Wandun Chen <[email protected]>
>> Link:
>> https://lore.kernel.org/lkml/[email protected]/#t
>
> A nit but please strip that #t.
Got it.
>
>> ---
>> 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.
Got it.
>
> 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?
munlock also would call mlock_pte_range, if page is under undergoing migration,
munlock dont need to wait migration complete.
>
>
>> + !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.
Indeed, there are workloads that are affected by this.
In RT kernels, RT processes typically use mlock to prevent their memory from
being swapped out, and sysctl_compact_unevictable_is set to default 0 in the
RT kernels, so mlocked memory won't be migrated and won't encounter migration
entries on access.
Using compaction_allow_unevictable here is mainly to check whether the system
is latency-sensitive. In RT kernels, sysctl_compact_unevictable_is set to 0 by
default.
If there is no waiting, the migration may won't have completed by the time of
the subsequent memory access, and then we would have to wait for migration
to finish in the page fault path. Since
pmd_migration_entry_wait/migration_entry_wait
has no priority-propagation mechanism, if the process executing mlock is an
RT process, this can lead to priority inversion and latency spike occurs.
>
>> + softleaf_is_migration(softleaf_from_pmd(*pmd)))) {
>
> Use pmd_is_migration_entry()? :)
Got it.
>
> 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.
Got it, will do in next version.
>
> So:
>
> static bool wait_for_migration(const struct vm_area_struct *vma, softleaf_t
> entry)
> {
>>>>> As above I'm not sure should even be checking this?
> if (!vma_test(vma, VMA_LOCKED_BIT))
> return false;
>>>>> 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.
Got it.
>
>> 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
Thanks again.
Wandun
