Re: [PATCH v2 10/19] mm, swap: consolidate cluster reclaim and check logic

[Kairui Song <ryncsn@gmail.com>]

On Thu, Nov 20, 2025 at 2:47 PM YoungJun Park <youngjun.park@lge.com> wrote:
>
> On Mon, Nov 17, 2025 at 02:11:51AM +0800, Kairui Song wrote:
> > From: Kairui Song <kasong@tencent.com>
> >
> > Swap cluster cache reclaim requires releasing the lock, so some extra
> > checks are needed after the reclaim. To prepare for checking swap cache
> > using the swap table directly, consolidate the swap cluster reclaim and
> > check the logic.
> >
> > Also, adjust it very slightly. By moving the cluster empty and usable
> > check into the reclaim helper, it will avoid a redundant scan of the
> > slots if the cluster is empty.
> >
> > And always scan the whole region during reclaim, don't skip slots
> > covered by a reclaimed folio. Because the reclaim is lockless, it's
> > possible that new cache lands at any time. And for allocation, we want
> > all caches to be reclaimed to avoid fragmentation. And besides, if the
> > scan offset is not aligned with the size of the reclaimed folio, we are
> > skipping some existing caches.
> >
> > There should be no observable behavior change, which might slightly
> > improve the fragmentation issue or performance.
> >
> > Signed-off-by: Kairui Song <kasong@tencent.com>
> > ---
> >  mm/swapfile.c | 45 +++++++++++++++++++++++----------------------
> >  1 file changed, 23 insertions(+), 22 deletions(-)
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index d2e60734ce8f..d57e83a4d0a7 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -778,42 +778,50 @@ static int swap_cluster_setup_bad_slot(struct swap_cluster_info *cluster_info,
> >       return 0;
> >  }
> >
> > -static bool cluster_reclaim_range(struct swap_info_struct *si,
> > -                               struct swap_cluster_info *ci,
> > -                               unsigned long start, unsigned long end)
> > +static unsigned int cluster_reclaim_range(struct swap_info_struct *si,
> > +                                       struct swap_cluster_info *ci,
> > +                                       unsigned long start, unsigned int order)
> >  {
> > +     unsigned int nr_pages = 1 << order;
> > +     unsigned long offset = start, end = start + nr_pages;
> >       unsigned char *map = si->swap_map;
> > -     unsigned long offset = start;
> >       int nr_reclaim;
> >
> >       spin_unlock(&ci->lock);
> >       do {
> >               switch (READ_ONCE(map[offset])) {
> >               case 0:
> > -                     offset++;
> >                       break;
> >               case SWAP_HAS_CACHE:
> >                       nr_reclaim = __try_to_reclaim_swap(si, offset, TTRS_ANYWAY);
> > -                     if (nr_reclaim > 0)
> > -                             offset += nr_reclaim;
> > -                     else
> > +                     if (nr_reclaim < 0)
> >                               goto out;
> >                       break;
> >               default:
> >                       goto out;
> >               }
> > -     } while (offset < end);
> > +     } while (++offset < end);
> >  out:
> >       spin_lock(&ci->lock);
> > +
> > +     /*
> > +      * We just dropped ci->lock so cluster could be used by another
> > +      * order or got freed, check if it's still usable or empty.
> > +      */
> > +     if (!cluster_is_usable(ci, order))
> > +             return SWAP_ENTRY_INVALID;
> > +     if (cluster_is_empty(ci))
> > +             return cluster_offset(si, ci);
> > +
> >       /*
> >        * Recheck the range no matter reclaim succeeded or not, the slot
> >        * could have been be freed while we are not holding the lock.
> >        */
> >       for (offset = start; offset < end; offset++)
> >               if (READ_ONCE(map[offset]))
> > -                     return false;
> > +                     return SWAP_ENTRY_INVALID;
> >
> > -     return true;
> > +     return start;
> >  }
> >
> >  static bool cluster_scan_range(struct swap_info_struct *si,
> > @@ -901,7 +909,7 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
> >       unsigned long start = ALIGN_DOWN(offset, SWAPFILE_CLUSTER);
> >       unsigned long end = min(start + SWAPFILE_CLUSTER, si->max);
>
> The Original code. I'm wondering if there's an off-by-one error here. Looking at the code
> below, it seems the design allows the end offset to go through the
> logic as well. Shouldn't it be 'start + SWAPFILE_CLUSTER - 1' and
> 'si->max - 1'?

You mean the `offset <= end` check below? That's fine because the for
loops starts with `end -= nr_pages`.

>
> >       unsigned int nr_pages = 1 << order;
> > -     bool need_reclaim, ret;
> > +     bool need_reclaim;
> >
> >       lockdep_assert_held(&ci->lock);
> >
> > @@ -913,20 +921,13 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
> >               if (!cluster_scan_range(si, ci, offset, nr_pages, &need_reclaim))
> >                       continue;
> >               if (need_reclaim) {
> > -                     ret = cluster_reclaim_range(si, ci, offset, offset + nr_pages);
> > -                     /*
> > -                      * Reclaim drops ci->lock and cluster could be used
> > -                      * by another order. Not checking flag as off-list
> > -                      * cluster has no flag set, and change of list
> > -                      * won't cause fragmentation.
> > -                      */
> > +                     found = cluster_reclaim_range(si, ci, offset, order);
> >                       if (!cluster_is_usable(ci, order))
> >                               goto out;
>
> This check resolves the issue I mentioned in my previous review.
>
> > -                     if (cluster_is_empty(ci))
> > -                             offset = start;
> >                       /* Reclaim failed but cluster is usable, try next */
> > -                     if (!ret)
> > +                     if (!found)
> >                               continue;
> > +                     offset = found;
> >               }
> >               if (!cluster_alloc_range(si, ci, offset, usage, order))
> >                       break;
>
> I think the reason cluster_is_usable() is checked redundantly here is
> because cluster_reclaim_range() returns an unsigned int (offset), making
> it impossible to distinguish error values.
>
> What if we make offset an output parameter (satisfying the assumption
> that it can be changed in reclaim_range) and return an error value
> instead? This would eliminate the redundant cluster_is_usable() check
> and simplify the logic. Also, the consecutive "offset = found, found =
> offset" is a bit confusing, and this approach could eliminate that as
> well.
>
> What do you think?

That's a good suggestion indeed, I'll try to make the code cleaner
this way. Thanks!