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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aSANhZC3sMUFxYuc@yjaykim-PowerEdge-T330>
Date: Fri, 21 Nov 2025 15:58:13 +0900
From: YoungJun Park <youngjun.park@....com>
To: Kairui Song <ryncsn@...il.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
	Baoquan He <bhe@...hat.com>, Barry Song <baohua@...nel.org>,
	Chris Li <chrisl@...nel.org>, Nhat Pham <nphamcs@...il.com>,
	Yosry Ahmed <yosry.ahmed@...ux.dev>,
	David Hildenbrand <david@...nel.org>,
	Johannes Weiner <hannes@...xchg.org>,
	Hugh Dickins <hughd@...gle.com>,
	Baolin Wang <baolin.wang@...ux.alibaba.com>,
	Ying Huang <ying.huang@...ux.alibaba.com>,
	Kemeng Shi <shikemeng@...weicloud.com>,
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	"Matthew Wilcox (Oracle)" <willy@...radead.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 10/19] mm, swap: consolidate cluster reclaim and check
 logic

On Thu, Nov 20, 2025 at 11:32:37PM +0800, Kairui Song wrote:
...

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

That's right! I missed that. Thanks for the clarification

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

Great~ I look forward to seeing the updated version. Thanks for
considering the suggestion.

Youngjun Park