[PATCH v2 0/2] mm/swap: skip empty clusters in the swapoff scan

[PATCH v2 0/2] mm/swap: skip empty clusters in the swapoff scan

[Youngjun Park]

From: Youngjun Park <her0gyugyu@gmail.com>

find_next_to_unuse() walks a swap device one offset at a time.  Slot
state now lives in a per cluster swap table, so patch 2 dismisses an
empty cluster with one counter read instead of SWAPFILE_CLUSTER table
reads.

Patch 1 is an unrelated one line comment fix noticed on the way.

A debug test confirmed the skip path runs, and swapoff completed
under load with no DEBUG_VM or lockdep splats.

Changes in v2:
- 1/2: reword the comment to "array, one entry per cluster", dropping
  the redundant "on every device" (Barry)
- 1/2: pick up Kairui's Acked-by
- 2/2: drop the min(), the swap table is always SWAPFILE_CLUSTER entries
  and swapon() masks the tail past si->max as bad (Kairui)
- 2/2: mark the unlocked ci->count read with READ_ONCE() for KCSAN
  instead of cluster_is_empty(), whose other callers hold ci->lock
  (Kairui)
- 2/2: expand the commit message to cover both of the above
- Rebased on mm-new
- Link to v1: https://lore.kernel.org/r/20260728155907.391820-1-youngjun.park@lge.com

Youngjun Park (2):
  mm/swap: fix stale comment on swap_info_struct::cluster_info
  mm/swap: scan by cluster in find_next_to_unuse()

 include/linux/swap.h |  2 +-
 mm/swapfile.c        | 41 ++++++++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 14 deletions(-)


base-commit: 0b53bff4fa05ff0d3ffbd3d3bb10fae69dfab498
--
2.48.1

[PATCH v2 1/2] mm/swap: fix stale comment on swap_info_struct::cluster_info

[Youngjun Park]

setup_swap_clusters_info() allocates cluster_info for every swap area,
not only for SSDs.

Signed-off-by: Youngjun Park <youngjun.park@lge.com>
Acked-by: Kairui Song <kasong@tencent.com>
---
 include/linux/swap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/swap.h b/include/linux/swap.h
index 2cb1d29307c5..2b14e2e9673b 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -246,7 +246,7 @@ struct swap_info_struct {
 	struct plist_node list;		/* entry in swap_active_head */
 	signed char	type;		/* strange name for an index */
 	unsigned int	max;		/* size of this swap device */
-	struct swap_cluster_info *cluster_info; /* cluster info. Only for SSD */
+	struct swap_cluster_info *cluster_info; /* array, one entry per cluster */
 	struct list_head free_clusters; /* free clusters list */
 	struct list_head full_clusters; /* full clusters list */
 	struct list_head nonfull_clusters[SWAP_NR_ORDERS];

base-commit: 0b53bff4fa05ff0d3ffbd3d3bb10fae69dfab498
-- 
2.48.1

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

[Youngjun Park]

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 <youngjun.park@lge.com>
---
 mm/swapfile.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index dea2d3b36e06..36e4e8884b76 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,34 @@ 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 = 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;
+		}
+		cond_resched();
+	}
 
-	return i;
+	return 0;
 }
 
 static int try_to_unuse(unsigned int type)
-- 
2.48.1

Re: [PATCH v2 1/2] mm/swap: fix stale comment on swap_info_struct::cluster_info

[Barry Song]

On Wed, Aug 5, 2026 at 10:11 PM Youngjun Park <her0gyugyu@gmail.com> wrote:
>
> setup_swap_clusters_info() allocates cluster_info for every swap area,
> not only for SSDs.
>
> Signed-off-by: Youngjun Park <youngjun.park@lge.com>
> Acked-by: Kairui Song <kasong@tencent.com>

Reviewed-by: Barry Song <baohua@kernel.org>

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

[Barry Song]

On Wed, Aug 5, 2026 at 10:12 PM Youngjun Park <her0gyugyu@gmail.com> 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 <youngjun.park@lge.com>

Reviewed-by: Barry Song <baohua@kernel.org>

[...]

> +       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

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

[Youngjun Park]

On Thu, Aug 06, 2026 at 10:33:54AM +0800, Barry Song wrote:
> Reviewed-by: Barry Song <baohua@kernel.org>

Hi Barry,

Thanks for the review :)

> [...]
>
> > +       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.

Yeah right. If I remain the code as it is, I will move this changelog on to the
code itself.

> 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?

Not expensive.
Kairui suggested keeping the end calculation simple(As I assume his intention?), 
so I dropped the min() in v1.

But after thinking about the retry case, keeping the min_t() seems clearer and
can also avoid walking the masked tail of the last cluster before retrying.

So I think I will keep the min_t() version (inclding move ci_off calculation only
to where it is needed) like below

+		ci = __swap_offset_to_cluster(si, i);
+		end = min_t(unsigned long, i - ci_off + 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;
+		}
+
+		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;
+		}
+		cond_resched();
+	}

I think both of good enough.
But, IMHO, remaining min_t calculation is my preference at now.

Kairui and Barry how do you think?

- Follow Barry's suggestion. remain min_t calculation.
- Add comment why we don't need min_t calculation.(also barry's suggestion)

Thanks
Youngjun