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@xxxxxxxxxx>

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