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

[Youngjun Park <youngjun.park@lge.com>]

...

Hello Kairui
Thanks for the review.

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

Yes. I remove it.

> > +
> > +               /*
> > +                * 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.

Right it is better to use READ_ONCE. I also applied it and submited v2!

Youngjun