Re: [PATCH v3 2/2] mm/swap: scan by cluster in find_next_to_unuse()

On 08/07/26 at 04:32am, Youngjun Park wrote:
> find_next_to_unuse() walks every offset from 0 to si->max, and swapoff
> restarts that walk on each retry, so the cost scales with the size of
> the device rather than with the few slots the shmem and mmlist passes
> could not free.  It has caused stalls before.
> 
> The flat walk predates the swap table.  Slot state now lives in a per
> cluster table, and wait_for_allocation() stops all allocation before
> try_to_unuse() runs, so a cluster that holds no slot in use stays that
> way.  Skip such a cluster instead of reading all of its entries.
> 
> Commit dc644a073769 ("mm: add three more cond_resched() in swapoff")
> answered those stalls with a cond_resched() every 256 offsets.  A walk
> bounded by one cluster no longer needs that counter.  The loop now runs
> at most SWAPFILE_CLUSTER times before it returns or reschedules, the
> same bound swap_reclaim_full_clusters() already scans between
> cond_resched() calls.
> 
> The scan end is clamped to si->max, so the walk stops there rather than
> running into the masked tail of the last cluster.
> 
> ci->count is read without ci->lock, so READ_ONCE() marks the read for
> KCSAN.  Allocation is already stopped, so the count can only drop, and a
> slot stops being counted only after its folio has left the swap cache.
> An empty cluster therefore holds nothing for try_to_unuse() to act on.
> 
> Signed-off-by: Youngjun Park <[email protected]>
> Reviewed-by: Barry Song <[email protected]>
> ---
>  mm/swapfile.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index dea2d3b36e06..0d24efd32eb0 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -370,8 +370,6 @@ static void discard_swap_cluster(struct swap_info_struct *si,
>  	}
>  }
>  
> -#define LATENCY_LIMIT		256
> -
>  static inline bool cluster_is_empty(struct swap_cluster_info *info)
>  {
>  	return info->count == 0;
> @@ -2763,7 +2761,9 @@ static int unuse_mm(struct mm_struct *mm, unsigned int type)
>  static unsigned int find_next_to_unuse(struct swap_info_struct *si,
>  					unsigned int prev)
>  {
> -	unsigned int i;
> +	struct swap_cluster_info *ci;
> +	unsigned long i, end;
> +	unsigned int ci_off;
>  	unsigned long swp_tb;
>  
>  	/*
> @@ -2772,19 +2772,36 @@ static unsigned int find_next_to_unuse(struct swap_info_struct *si,
>  	 * hits are okay, and sys_swapoff() has already prevented new
>  	 * allocations from this area (while holding swap_lock).
>  	 */
> -	for (i = prev + 1; i < si->max; i++) {
> -		swp_tb = swap_table_get(__swap_offset_to_cluster(si, i),
> -					i % SWAPFILE_CLUSTER);
> -		if (!swp_tb_is_null(swp_tb) && !swp_tb_is_bad(swp_tb))
> -			break;
> -		if ((i % LATENCY_LIMIT) == 0)
> +	i = prev + 1;
> +	while (i < si->max) {
> +		ci = __swap_offset_to_cluster(si, i);
> +		end = min_t(unsigned long,
> +			    ALIGN_DOWN(i, SWAPFILE_CLUSTER) + SWAPFILE_CLUSTER,
> +			    si->max);
> +
> +		/*
> +		 * An empty cluster has no slot in use, so skip it whole.
> +		 * A slot is uncounted only after its folio left the swap
> +		 * cache, so there is nothing here for try_to_unuse() to act on.
> +		 * Count only drops here, so a READ_ONCE() without ci->lock is
> +		 * enough, unlike in every other cluster_is_empty() caller.
> +		 */
> +		if (!READ_ONCE(ci->count)) {
> +			i = end;
>  			cond_resched();
> -	}
> +			continue;
> +		}
>  
> -	if (i == si->max)
> -		i = 0;
> +		ci_off = i % SWAPFILE_CLUSTER;
> +		for (; i < end; ci_off++, i++) {
> +			swp_tb = swap_table_get(ci, ci_off);
> +			if (!swp_tb_is_null(swp_tb) && !swp_tb_is_bad(swp_tb))
> +				return i;

I would remove ci_off to save one local variable, but it's only personal
preference, not strong opinion.

	for (; i < end; i++) {
	    swp_tb = swap_table_get(ci, i % SWAPFILE_CLUSTER);
	    ...
	}

Other than the nitpick, this is a great optimization patch.

Reviewed-by: Baoquan He <[email protected]>


> +		}
> +		cond_resched();
> +	}
>  
> -	return i;
> +	return 0;
>  }
>  
>  static int try_to_unuse(unsigned int type)
> -- 
> 2.48.1
> 
>