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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/12/26 02:03, 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 small 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 on this workload the scanner
> often flushes the whole mm as it walks the PTE tables.
> 
> This slows scanning and keeps mm->tlb_flush_pending raised longer.  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 scan 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 don't ask for a batched flush when the batch holds a single small
> folio that is not mapped shared; try_to_migrate_one() then uses
> ptep_clear_flush() for each PTE it encounters.  Large folios retain
> their existing flush behavior.  migrate_pages_batch() computes the
> flags, since it knows how many folios it is migrating.  PMD-mapped
> THPs are not affected, since set_pmd_migration_entry() flushes them
> regardless of TTU_BATCH_FLUSH.
> 
> Signed-off-by: Zhiteng Qiu <qiuzhiteng@hygon.cn>
> ---
> Changes in v4:
>  - Compute the try_to_migrate() flags in migrate_pages_batch() with a
>    helper and pass them to migrate_folio_unmap() instead of a bool, as
>    suggested by David.
>  - Go back to v2's check, limited to small folios, using
>    folio_maybe_mapped_shared() as Zi suggested.  folio_mapcount() would
>    stop being a good hint for large folios once try_to_unmap() batches
>    them better (David).
>  - Shorter comment (Zi).
> 
> Changes in v3:
>  - Use folio_mapcount() for the check, as Ying suggested.
>  - 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.
> 
> v3: https://lore.kernel.org/r/20261109122915.2150586-1-qiuzhiteng@hygon.cn
> v2: https://lore.kernel.org/r/20261107143456.1590658-1-qiuzhiteng@hygon.cn
> v1: https://lore.kernel.org/r/20261106041628.1177963-1-qiuzhiteng@hygon.cn
> 
> The table was measured on v7.3 with the same check as v4.  On mm-new
> with v4, three balancing on/off pairs gave a 1.9-2.9% loss and
> 13.2-14.6 TLB interrupts per migrated page.
> 
> I re-ran the tests on v4: mm/migration selftests 6/6, LTP move_pages
> and migrate_pages 14/14, 30 minutes with compaction forced every 30 s,
> and 4.5 hours of NUMA balancing racing with fork(), MADV_DONTNEED and
> move_pages(), 4 of them on a CONFIG_DEBUG_VM kernel, all without
> warnings.  Splitting partially mapped 2 MiB THPs during NUMA migration
> still gets the batched flush (0.115-0.120 TLB interrupts per migrated
> page), 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.
> 
>  mm/migrate.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index cb699ea59d33..8eb14b0a5d73 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1218,11 +1218,28 @@ static void migrate_folio_done(struct folio *src,
>  		folio_put(src);
>  }
>  
> +/*
> + * Batching the TLB flush only helps when the rmap walk can clear more than
> + * one PTE.  A single small folio mapped once has nothing to batch, and the
> + * batched unmap would leave mm->tlb_flush_batched pending.
> + */

These comments belongs split up into the function above the relevant checks.

> +static enum ttu_flags migrate_ttu_flags(struct folio *folio,
> +					enum migrate_mode mode,
> +					bool single_folio)

Just pass the from list, please and check for ; internally.

Also, use two-tab indent here.

Further, i would suggest calling this

	folio_migrate_ttu_flags


> +{
> +	if (mode != MIGRATE_ASYNC)
> +		return 0;
> +	if (single_folio && !folio_test_large(folio) &&
> +	    !folio_maybe_mapped_shared(folio))
> +		return 0;

/* Helpful comment. */
if (!list_is_singular(from))
	return TTU_BATCH_FLUSH;
/* Helpful comment. */
if (folio_test_large(folio))
	return TTU_BATCH_FLUSH;
/* Helpful comment. */
if (folio_maybe_mapped_shared(folio))
	return TTU_BATCH_FLUSH;
return 0;

> +	return TTU_BATCH_FLUSH;
> +}
> +
>  /* Obtain the lock on page, remove all ptes. */
>  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)
> +		enum ttu_flags ttu, struct list_head *ret)
>  {
>  	struct folio *dst;
>  	int rc = -EAGAIN;
> @@ -1344,7 +1361,7 @@ 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);
> +		try_to_migrate(src, ttu);
>  		old_folio_state |= FOLIO_WAS_MAPPED;
>  	}
>  
> @@ -1835,9 +1852,11 @@ static int migrate_pages_batch(struct list_head *from,
>  	LIST_HEAD(unmap_folios);
>  	LIST_HEAD(dst_folios);
>  	bool nosplit = (reason == MR_NUMA_MISPLACED);
> +	bool single_folio = list_is_singular(from);
> +	enum ttu_flags ttu;
>  
>  	VM_WARN_ON_ONCE(mode != MIGRATE_ASYNC &&
> -			!list_empty(from) && !list_is_singular(from));
> +			!list_empty(from) && !single_folio);
>  
>  	for (pass = 0; pass < nr_pass && retry; pass++) {
>  		retry = 0;
> @@ -1924,8 +1943,10 @@ static int migrate_pages_batch(struct list_head *from,
>  				continue;
>  			}
>  
> +			ttu = migrate_ttu_flags(folio, mode, single_folio);
>  			rc = migrate_folio_unmap(get_new_folio, put_new_folio,
> -					private, folio, &dst, mode, ret_folios);
> +					private, folio, &dst, mode, ttu,
> +					ret_folios);

While at it, might as well fixup the indentation.



-- 
Cheers,

David