Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()

[wangzicheng]

On 2026-10-22 14:41 +0800, Ridong Chen wrote:
> 
> 
> On 10/21/2026 4:38 PM, wangzicheng@xxxxxxxxx wrote:
> > On 2026-10-20 10:35 +0800, Ridong Chen wrote:
> >> From: Ridong Chen <chenridong@xxxxxxxxxx>
> >>
> >> lru_gen_folio_seq() computes how many generations below max_seq a
> >> folio should be added to, but stores it in a variable named "gen".
> >> Elsewhere in MGLRU "gen" means an index into lrugen->folios[], i.e.
> >> seq % MAX_NR_GENS, so reusing the name for a relative offset is
> >> confusing.
> >>
> >> The comment table above the code also no longer matches it. It only
> >> covers the PG_active and the final branch, leaving out the reclaiming
> >> and "can't evict yet" cases, and it lists LRU_REFS_FLAGS as an input
> >> although lru_gen_folio_seq() never looks at it.
> >>
> >> Rename the local variable to "age", redraw the table so that each
> >> branch maps to the age and seq it produces, and note that the result
> >> is clamped to min_seq[type].
> >>
> >> No functional change.
> >>
> >> Assisted-by: LLM
> >> Signed-off-by: Ridong Chen <chenridong@xxxxxxxxxx>
> >> ---
> >>   include/linux/mm_inline.h | 48 ++++++++++++++++++++++++++-------------
> >>   1 file changed, 32 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> >> index ab69b9930893..a933d8c14558 100644
> >> --- a/include/linux/mm_inline.h
> >> +++ b/include/linux/mm_inline.h
> >> @@ -302,34 +302,50 @@ static inline unsigned long lru_gen_folio_seq(const struct lruvec *lruvec,
> >>   					      const struct folio *folio,
> >>   					      bool reclaiming)
> >>   {
> >> -	int gen;
> >> +	int age;
> >>   	int type = folio_is_file_lru(folio);
> >>   	const struct lru_gen_folio *lrugen = &lruvec->lrugen;
> >>   
> >>   	/*
> >> -	 * +-----------------------------------+-----------------------------------+
> >> -	 * | Accessed through page tables and  | Accessed through file descriptors |
> >> -	 * | promoted by folio_update_gen()    | and protected by folio_inc_gen()  |
> >> -	 * +-----------------------------------+-----------------------------------+
> >> -	 * | PG_active (set while isolated)    |                                   |
> >> -	 * +-----------------+-----------------+-----------------+-----------------+
> >> -	 * |  PG_workingset  |  PG_referenced  |  PG_workingset  |  LRU_REFS_FLAGS |
> >> -	 * +-----------------------------------+-----------------------------------+
> >> -	 * |<---------- MIN_NR_GENS ---------->|                                   |
> >> -	 * |<---------------------------- MAX_NR_GENS ---------------------------->|
> >> +	 *            +-----------------+-----------------+-----------------+-----------------+
> >> +	 * age        |        1        |        2        |        3        |        4        |
> > Hi Ridong,
> >    
> > Agreed, I also found this part confusing.
> > 
> > One problem with the new name: in MGLRU a larger seq/gen means a
> > younger generation, but a larger age means an older one, so every
> > read needs a flip (max_seq - age + 1). Assigning the seq directly in
> > each branch might read more naturally.
> > 
> 
> Hi Zicheng,
> 
> Thank you for your reply. We both agree that this part of the code is confusing. 
> 'age' is not perfect, but it is much better. The bigger the age, the elder it 
> is, just like human age. And it is a local variable, not used elsewhere, so it 
> is fine for me.
> 
Hi Ridong,
   
Thanks for the explanation. To be clear, I have no problem with the
name itself: it is a local variable and there is no functional
impact either way.

> > Also, both tables run past 80 columns (~95 for the art in mglru-fg
> > 4/17, ~96 for the redrawn one here, tab as 8). Would you consider
> > something vertical and shorter? I had prepared a comment for
> > mglru-fg, and since this is being discussed here too, here it is
> > (conditions are per mglru-fg, where the flags became the refs
> > count):
> > 
> >      /*
> >          * The seq is clamped to min_seq[type]:
> >          *
> >          *   destination     | folio state
> >          *   ----------------+--------------------------------------------
> >          *   youngest        | PG_active, refs >= LRU_REFS_WORKINGSET
> >          *   (max_seq)       |
> >          *
> >          *   2nd youngest    | PG_active, refs < LRU_REFS_WORKINGSET
> >          *   (max_seq - 1)   | anon folio not yet in swapcache
> >          *                   | PG_reclaim folio, dirty or under writeback
> >          *
> >          *   2nd oldest      | refs >= LRU_REFS_WORKINGSET
> >          *   (max_seq - 2)   |
> >          *
> >          *   oldest          | reclaiming
> >          *   (min_seq[type]) | otherwise: refs < LRU_REFS_WORKINGSET
> >          */
> > 
> > With a comment like this, developers can see the generation
> > placement directly, instead of working it out from the code.
> > 
> 
> Thanks for the table,
> 
> As Kairui replied, he did not want more code details in the comment, which may 
> change a lot. And Kairui will pick this patch with his FG series. Although I 
> wanted to add the table, Kairui felt there shouldn't be too much detail.
> 
> What is more, I think the following is needed—it is what this function initially 
> implemented.
> 
> +-----------------------------------+-----------------------------------+
> | Accessed through page tables and  | Accessed through file descriptors |
> | promoted by folio_update_gen()    | and protected by folio_inc_gen()  |
> +-----------------+-----------------+-----------------+-----------------+
> 
> > Best,
> > Zicheng
> > 
> > 
> >> +	 *            +-----------------+-----------------+-----------------+-----------------+
> >> +	 * seq        |     max_seq     |    max_seq-1    |    max_seq-2    |    max_seq-3    |
> >> +	 *            +-----------------------------------+-----------------------------------+
> >> +	 *            |<---------- MIN_NR_GENS ---------->|                                   |
> >> +	 *            |<---------------------------- MAX_NR_GENS ---------------------------->|
> >> +	 *            +-----------------------------------+-----------------------------------+
> >> +	 *            | Accessed through page tables and  | Accessed through file descriptors |
> >> +	 *            | promoted by folio_update_gen()    | and protected by folio_inc_gen()  |
> >> +	 *            +-----------------+-----------------+-----------------+-----------------+
> >> +	 * PG_active  |  PG_workingset  | !PG_workingset  |                 |                 |
> >> +	 *            +-----------------+-----------------+-----------------+-----------------+
> >> +	 * reclaiming |                 |                 |                 |     always      |
> >> +	 *            +-----------------+-----------------+-----------------+-----------------+
> >> +	 * can't      |                 |  anon && not in |                 |                 |
> >> +	 * evict yet  |                 |  swapcache, or  |                 |                 |
> >> +	 *            |                 |  PG_reclaim &&  |                 |                 |
> >> +	 *            |                 | dirty/writeback |                 |                 |
> >> +	 *            +-----------------+-----------------+-----------------+-----------------+
> >> +	 * else       |                 |                 |  PG_workingset  | !PG_workingset  |
> >> +	 *            |                 |                 |       or        |       &&        |
> >> +	 *            |                 |                 |  PG_referenced  | !PG_referenced  |
> >> +	 *            +-----------------+-----------------+-----------------+-----------------+
> >> +	 *
> >> +	 * The resulting seq is clamped to min_seq[type].
> >>   	 */
> >>   	if (folio_test_active(folio))
> >> -		gen = MIN_NR_GENS - folio_test_workingset(folio);
> >> +		age = MIN_NR_GENS - folio_test_workingset(folio);
> >>   	else if (reclaiming)
> >> -		gen = MAX_NR_GENS;
> >> +		age = MAX_NR_GENS;
> >>   	else if ((!folio_is_file_lru(folio) && !folio_test_swapcache(folio)) ||
> >>   		 (folio_test_reclaim(folio) &&
> >>   		  (folio_test_dirty(folio) || folio_test_writeback(folio))))
> >> -		gen = MIN_NR_GENS;
> >> +		age = MIN_NR_GENS;
> >>   	else
> >> -		gen = MAX_NR_GENS - (folio_test_workingset(folio) || folio_test_referenced(folio));
> >> +		age = MAX_NR_GENS - (folio_test_workingset(folio) || folio_test_referenced(folio));
> >>   
> >> -	return max(READ_ONCE(lrugen->max_seq) - gen + 1, READ_ONCE(lrugen->min_seq[type]));
> >> +	return max(READ_ONCE(lrugen->max_seq) - age + 1, READ_ONCE(lrugen->min_seq[type]));
> >>   }

My real point was about the form of the computation: whatever the
variable is called, the return still does max_seq - X + 1 and then
clamps to min_seq[type], so a reader has to juggle a plus, a minus
and a clamp at the same time. When I first read it I had to go back
and forth. By comparison, the 6.6-era code assigned the placement 
directly in each branch (roughly `seq = max_seq or
seq = min_seq + 1` style), which is a bit more verbose but much more
direct.

Anyway, these are minor points with no functional impact. I don't
feel strongly about this, so I will defer to you and Kairui.

Thanks,
Zicheng

> >>   
> >>   static inline bool lru_gen_add_folio(struct lruvec *lruvec, struct folio *folio, bool reclaiming)
> >> -- 
> >> 2.34.1
> >>
> >>
> > 
> 
> -- 
> Best regards
> Ridong
> 
>