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

[Kairui Song]

On Wed, Jul 29, 2026 at 12:52 AM Youngjun Park <her0gyugyu@xxxxxxxxx> wrote:
>
> From: Youngjun Park <her0gyugyu@xxxxxxxxx>
>
> From: Youngjun Park <youngjun.park@xxxxxxx>
>
> 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 with cluster_is_empty() 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.
>
> cluster_is_empty() reads ci->count without ci->lock, like the rest of
> this scan.  A slot stops being counted only after its folio has left the
> swap cache, so an empty cluster holds nothing for try_to_unuse() to act
> on and skipping it loses no work.

A really nice optimization, thanks!

>
> Signed-off-by: Youngjun Park <youngjun.park@xxxxxxx>
> ---
>  mm/swapfile.c | 39 ++++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 5d15913dcf86..230abb276ceb 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,32 @@ 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);
> +               ci_off = i % SWAPFILE_CLUSTER;
> +               end = min(si->max, i - ci_off + SWAPFILE_CLUSTER);

Do we need the min here? Table size is always SWAPFILE_CLUSTER aligned.

> +
> +               /*
> +                * 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.
> +                */
> +               if (cluster_is_empty(ci)) {

Hmm, it's not wrong, but this is indeed the only user calling
cluster_is_empty without holding a lock, and not in initilization
path, perhaps we should at least make it READ_ONCE? Maybe KCSAN will
not be happy, I guess? Just nitpick.