Re: [PATCH v4 5/5] mm/vmscan: flush TLB for every 31 folios evictions

[David Hildenbrand (Arm)]

On 5/25/26 16:57, Zhang Peng wrote:
> Currently we flush TLB for every dirty folio, which is a bottleneck for
> systems with many cores as this causes heavy IPI usage.
> 
> So instead, batch the folios, and flush once for every 31 folios (one
> folio_batch). These folios will be held in a folio_batch releasing their
> lock, then when folio_batch is full, do following steps:
> 
> - For each folio: lock - check still evictable (writeback, mapped,
>   dma_pinned)
>   - If no longer evictable, put back to LRU
> - Flush TLB once for the batch
> - Pageout the folios
> 
> Signed-off-by: Zhang Peng <bruzzhang@xxxxxxxxxxx>
> ---
>  mm/vmscan.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 71 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index abf3a2878456..c0d22afe67a5 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -1221,6 +1221,57 @@ static bool pageout_one(struct folio *folio,
>  	return false;
>  }
>  
> +static void pageout_batch(struct folio_batch *fbatch,
> +			  struct list_head *ret_folios,
> +			  struct folio_batch *free_folios,
> +			  struct scan_control *sc, struct reclaim_stat *stat,
> +			  struct swap_iocb **plug, struct list_head *folio_list,
> +			  unsigned int *nr_reclaimed)

Two tabs. But this is starting to look a bit messy. Could a helper struct be
used to reduce the parameter count to something readable?

> +{
> +	int i, count = folio_batch_count(fbatch);
> +	struct folio *folio;
> +
> +	/*
> +	 * Reuse fbatch in-place: reinit only clears the count, the
> +	 * underlying folios array is still accessible via saved count.
> +	 * Filter and re-add valid folios back into the same batch.
> +	 */
> +	folio_batch_reinit(fbatch);

That looks rather hacky. You reinit the batch to then traverse the batch? There
must be a cleaner way. Walking and modifying the same folio batch in one go is
rather crazy.

> +	for (i = 0; i < count; ++i) {
> +		folio = fbatch->folios[i];
> +		if (!folio_trylock(folio)) {
> +			list_add(&folio->lru, ret_folios);
> +			continue;
> +		}
> +
> +		VM_WARN_ON_FOLIO(folio_test_lru(folio), folio);
> +
> +		if (folio_test_writeback(folio) || folio_mapped(folio) ||
> +		    folio_maybe_dma_pinned(folio)) {

Took me a second to understand why exactly you test for these properties. Can
you add a comment how these check here mimic what we checked earlier, before
dopping the lock?

> +			folio_unlock(folio);
> +			list_add(&folio->lru, ret_folios);
> +			continue;
> +		}

So, IIUC, what could have happened after dropping the folio lock is that someone
would have remapped the folio to user space?

Either from the pagecache or from the swap PTE -> swapcache.

>From there, it could have gotten pinned through the page tables.

Is that correct?

What would happen if page migration finds the folio, locks it and wants to
migrate it?

-- 
Cheers,

David