Re: [PATCH v3] mm/migrate: don't batch the TLB flush when there is nothing to batch

[David Hildenbrand (Arm)]

On 11/9/26 13:29, Zhiteng Qiu wrote:
> NUMA balancing migrates one folio at a time, but since commit
> 7e12beb8ca2a ("migrate_pages: batch flushing TLB") it still unmaps
> with TTU_BATCH_FLUSH like any other MIGRATE_ASYNC migration.  When the
> batch contains one folio mapped only once, there is nothing to batch,
> and the batched unmap can leave mm->tlb_flush_batched pending, so the
> NUMA scanner's next change_pte_range() can call flush_tlb_mm() before
> changing the PTE table.  With migrations running concurrently, that
> state keeps being refilled and the scanner ends up flushing the whole
> mm for many of the PTE tables it walks.
> 
> That slows each scan pass down a lot, and mm->tlb_flush_pending stays
> raised for the whole pass.  On x86, a migration that unmaps a
> PROT_NONE PTE while mm->tlb_flush_pending is set gets true from
> pte_accessible() and needs a flush too; with batching, the resulting
> arch_tlbbatch_flush() is a full flush.  The two feed each other.  In
> one run, 33% of the migrations overlapped a pass, although passes
> covered only 9% of the time.
> 
> On a 2-socket, 384-CPU x86 machine without INVLPGB, with v7.3, a
> 120 GiB anonymous working set on 4 KiB pages and 384 threads (three
> balancing on/off pairs per kernel):
> 
>                                      v7.3          patched
>   throughput loss from balancing     13.0-16.5%    0.5-6.3%
>   TLB interrupts per migrated page   202-210       12.7-13.6
>   average scanner mmu_gather time    ~90 ms        ~6.7 ms
> 
> The mmu_gather times were measured in separate instrumented runs.
> 
> So only ask for a batched flush when the rmap walk can meet more than
> one PTE, that is, when there is more than one folio in the batch or
> the folio is mapped more than once.  Otherwise try_to_migrate_one()
> uses ptep_clear_flush() on the one PTE.  PMD-mapped THPs are not
> affected, since set_pmd_migration_entry() flushes them regardless of
> TTU_BATCH_FLUSH.
> 
> Signed-off-by: Zhiteng Qiu <qiuzhiteng@xxxxxxxx>
> ---
> Changes in v3:
>  - Use folio_mapcount() for the check, as Ying suggested.  It counts
>    PTEs, so PTE-mapped large folios are still batched when more than
>    one PTE remains mapped.  The only difference from v2 is a large
>    folio with just one PTE still mapped, which is no longer batched
>    either.
>  - Cut the changelog down.
> 
> Changes in v2:
>  - Pass the try_to_migrate() flags directly and use single_folio in
>    the VM_WARN_ON_ONCE(), as suggested by Ying.
>  - Rewrote the changelog after looking into why b99a342d4f11 does not
>    avoid the flush here.
> 
> v2: https://lore.kernel.org/r/20261107143456.1590658-1-qiuzhiteng@xxxxxxxx
> v1: https://lore.kernel.org/r/20261106041628.1177963-1-qiuzhiteng@xxxxxxxx
> 
> The table was measured on v7.3 with the check as posted in v2.  v3
> only differs for a large folio with a single PTE left mapped.  On
> mm-new with v3, one balancing on/off pair gave a 3.2% loss and 13.5
> TLB interrupts per migrated page.
> 
> I re-ran the tests on v3: mm/migration selftests 6/6, LTP move_pages
> and migrate_pages 14/14, and 30 minutes with compaction forced every
> 30 s without warnings.  Splitting partially mapped 2 MiB THPs during
> NUMA migration still gets the batched flush (0.11-0.12 TLB
> interrupts per migrated page, same as base), and 64 KiB mTHP and a
> MAP_SHARED mapping shared by two processes behave like base.
> 
> No Fixes: tag, as this is a performance trade-off that has been there
> since v6.3 and nobody has reported it as a regression.  Happy to add
> one if you think it should have one.
> 

For the future, please don't send new revisions in reply to old revisions.

>  mm/migrate.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index cb699ea59d33..011cceadeda9 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1222,7 +1222,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)
> +		bool single_folio, struct list_head *ret)

Is there no way to avoid that boolean?

>  {
>  	struct folio *dst;
>  	int rc = -EAGAIN;
> @@ -1344,7 +1344,19 @@ 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);
> -		try_to_migrate(src, mode == MIGRATE_ASYNC ? TTU_BATCH_FLUSH : 0);
> +		enum ttu_flags ttu = 0;
> +
> +		/*
> +		 * Batching only helps if the rmap walk can find more than one
> +		 * PTE to clear.  With a single folio mapped once there is
> +		 * nothing to merge, and a batched unmap would only leave
> +		 * mm->tlb_flush_batched pending, making later page table
> +		 * changes in this mm (e.g. NUMA scanning) flush the whole mm.
> +		 */
> +		if (mode == MIGRATE_ASYNC &&
> +		    (!single_folio || folio_mapcount(src) > 1))

This is all information the caller could provide, and the caller could just
provide the TTU flag then instead of passing a new "bool".

Also, likely this check can be put into a separate helper function.

> +			ttu = TTU_BATCH_FLUSH;


I am not quite happy about the folio_mapcount() check, though: as soon as we
optimize try_to_unmap() to batch better for large folios, it would be just plain
wrong.

So this needs a second thought.

-- 
Cheers,

David