Re: [PATCH RFC 06/15] mm/mglru: use explicit tier range in read_ctrl_pos()

[Kairui Song]

On Fri, Aug 14, 2026 at 1:25 PM Barry Song <baohua@xxxxxxxxxx> wrote:
>
> On Tue, Aug 4, 2026 at 3:47 AM Kairui Song via B4 Relay
> <devnull+kasong.tencent.com@xxxxxxxxxx> wrote:
> >
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > read_ctrl_pos() encodes the tier range in a single "tier" parameter
> > via "tier % MAX_NR_TIERS" as the start and "min(tier, MAX_NR_TIERS-1)"
> > as the end.  This is hard to follow or maintain or extend. Tier
> > values 0..3 select a single tier, while tier == MAX_NR_TIERS selects
> > the full range.
> >
> > Replace it with explicit (tier_min, tier_max) parameters using a
> > half-open [tier_min, tier_max) interval, which is the conventional C
> > idiom.  The call sites become self-documenting:
> >
> >   - get_tier_idx:   (0, 1) for tier 0, (tier, tier+1) for each tier
> >   - get_type_to_scan: (0, MAX_NR_TIERS) for the full range
>
> I agree, this is a nice cleanup. The existing code is really hard to
> read. The magic is in i = tier % MAX_NR_TIERS, but I've been bitten
> by this magic a couple of times.
>
> But could we make the range inclusive? Right now, it still feels a bit
> difficult to follow.
>
> Since tier_min and tier_max are tier indices rather than a typical
> array range, would an inclusive [tier_min, tier_max] range be easier
> to read here? In particular, tier, tier + 1 at the call sites still
> requires the reader to know that tier_max is exclusive.
>
> >
> > No functional change.
> >
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > ---
> >  mm/vmscan.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index c2ea92c2b69e..a359d5a1ff41 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -3192,8 +3192,8 @@ struct ctrl_pos {
> >         int gain;
> >  };
> >
> > -static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
> > -                         struct ctrl_pos *pos)
> > +static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier_min,
> > +                         int tier_max, int gain, struct ctrl_pos *pos)
> >  {
> >         int i;
> >         struct lru_gen_folio *lrugen = &lruvec->lrugen;
> > @@ -3202,7 +3202,7 @@ static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
> >         pos->gain = gain;
> >         pos->refaulted = pos->total = 0;
> >
> > -       for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
> > +       for (i = tier_min; i < tier_max; i++) {
> >                 pos->refaulted += lrugen->avg_refaulted[type][i] +
> >                                   atomic_long_read(&lrugen->refaulted[hist][type][i]);
> >                 pos->total += lrugen->avg_total[type][i] +
> > @@ -4805,9 +4805,9 @@ static int get_tier_idx(struct lruvec *lruvec, int type)
> >          * This value is chosen because any other tier would have at least twice
> >          * as many refaults as the first tier.
> >          */
> > -       read_ctrl_pos(lruvec, type, 0, 2, &sp);
> > +       read_ctrl_pos(lruvec, type, 0, 1, 2, &sp);
>
> It's still a bit of a headache when three integers are put together.
> Could we somehow make the 0 more self-explanatory, such as using
> TIER_MIN or a named constant?
>
> >         for (tier = 1; tier < MAX_NR_TIERS; tier++) {
> > -               read_ctrl_pos(lruvec, type, tier, 3, &pv);
> > +               read_ctrl_pos(lruvec, type, tier, tier + 1, 3, &pv);
> >                 if (!positive_ctrl_err(&sp, &pv))
> >                         break;
> >         }
> > @@ -4828,8 +4828,8 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)
> >          * Compare the sum of all tiers of anon with that of file to determine
> >          * which type to scan.
> >          */
> > -       read_ctrl_pos(lruvec, LRU_GEN_ANON, MAX_NR_TIERS, swappiness, &sp);
> > -       read_ctrl_pos(lruvec, LRU_GEN_FILE, MAX_NR_TIERS, MAX_SWAPPINESS - swappiness, &pv);
> > +       read_ctrl_pos(lruvec, LRU_GEN_ANON, 0, MAX_NR_TIERS, swappiness, &sp);
> > +       read_ctrl_pos(lruvec, LRU_GEN_FILE, 0, MAX_NR_TIERS, MAX_SWAPPINESS - swappiness, &pv);
> >
>
> Maybe we could introduce macros such as TIER_MIN and TIER_MAX to
> make the intent clearer?
>
> #define TIER_MIN 0
> #define TIER_MAX (MAX_NR_TIERS - 1)
>
> Best Regards
> Barry

Sounds good to me. Thanks for the review!