Re: [PATCH 1/3] mm/mglru: improve readability of isolate_folios()

[Baoquan He]

On 08/28/26 at 11:22am, Barry Song wrote:
> On Fri, Aug 28, 2026 at 11:15 AM Baoquan He <baoquan.he@xxxxxxxxx> wrote:
> >
> > On 08/20/26 at 12:56pm, Barry Song (Xiaomi) wrote:
> > > From: Ridong Chen <chenridong@xxxxxxxxxx>
> > >
> > > The for_each_evictable_type() loop in isolate_folios()
> > > is misleading: it does not actually iterate over each
> > > evictable type. Instead, get_type_to_scan() selects the
> > > type to scan, while the iterator `i` merely bounds the
> > > number of attempts.
> > >
> > > Make the fallback behavior explicit in the code and remove the
> > > opaque for_each_evictable_type(i, swappiness).
> > >
> > > Signed-off-by: Ridong Chen <chenridong@xxxxxxxxxx>
> > > Co-developed-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > > Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > > ---
> > >  mm/vmscan.c | 46 ++++++++++++++++++++++++++--------------------
> > >  1 file changed, 26 insertions(+), 20 deletions(-)
> >
> > The subject doens't reflect the truth. This patch changes the behaviour,
> > but not improve readability of isolate_folios() only.
> >
> > I also noticed the confusion of isolate_folios() implementation, and made
> > a patch to only change the local variable and added code comment to
> > explain it in my local branch. Surely refactorying is also good.
> >
> > +        * Scan at most one type per evictable type (anon/file), starting with
> > +        * the type get_type_to_scan() picked as statistically colder.
> > +        *
> > +        * After a scan:
> > +        *  - isolated > 0:  got folios, record the type and return.
> > +        *  - scanned == 0:  the type is empty; fall back to the other type.
> > +        *  - otherwise:     the type has folios but all were hot (or lost an
> > +        *                   isolate race); retry the same type rather than
> > +        *                   switch, so positive_ctrl_err()'s refault
> > +        *                   statistics stay unbiased.
> > +        */
> >
> > While in Ridong's patch, the 3rd case disappeared. It doesn't rescan with
> > the original type as the old code is doing, but return directly.
> >
> 
> right, the 3rd case comes back in patch 3/3:
> https://lore.kernel.org/linux-mm/20260820045603.68809-4-baohua@xxxxxxxxxx/
> 
> so i guess we can rename patch 1/3?

Both is fine as long as thing is correct and clear, thanks.