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

On Wed, Aug 5, 2026 at 10:12 PM Youngjun Park <[email protected]> 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 inner loop runs to the end of the cluster rather than to si->max.
> The swap table is always SWAPFILE_CLUSTER entries and swapon() masks
> [si->max, round_up(si->max, SWAPFILE_CLUSTER)) as bad, so the tail of a
> partial last cluster is rejected by swp_tb_is_bad() and never returned.
>
> 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]>

[...]

> +       i = prev + 1;
> +       while (i < si->max) {
> +               ci = __swap_offset_to_cluster(si, i);
> +               ci_off = i % SWAPFILE_CLUSTER;
> +               end = i - ci_off + SWAPFILE_CLUSTER;
> +
> +               /*
> +                * 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;
> +               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;

You have the following in the changelog:

" The inner loop runs to the end of the cluster rather than to si->max.
 The swap table is always SWAPFILE_CLUSTER entries and swapon() masks
 [si->max, round_up(si->max, SWAPFILE_CLUSTER)) as bad, so the tail of a
 partial last cluster is rejected by swp_tb_is_bad() and never returned."


But I wonder whether this explanation should be part of the code
comment instead. Otherwise, people may wonder why this is safe and
ask for the below:

end = min_t(unsigned long, i - ci_off + SWAPFILE_CLUSTER, si->max);

How expensive is the min() operation? If it is cheap enough, maybe
we should just do the min() unconditionally?

Thanks
Barry