* [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
@ 2026-10-20 2:35 Ridong Chen
2026-10-20 5:43 ` Barry Song
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Ridong Chen @ 2026-10-20 2:35 UTC (permalink / raw)
To: Andrew Morton, David Hildenbrand
Cc: Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Kairui Song, Qi Zheng,
Shakeel Butt, Barry Song, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Baoquan He, Baolin Wang, linux-mm, linux-kernel, Ridong Chen,
Ridong Chen
From: Ridong Chen <chenridong@xiaomi.com>
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@xiaomi.com>
---
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 |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * 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]));
}
static inline bool lru_gen_add_folio(struct lruvec *lruvec, struct folio *folio, bool reclaiming)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-20 2:35 [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq() Ridong Chen
@ 2026-10-20 5:43 ` Barry Song
2026-10-20 6:27 ` Ridong Chen
2026-10-20 8:20 ` Kairui Song
2026-10-21 8:38 ` wangzicheng
2 siblings, 1 reply; 10+ messages in thread
From: Barry Song @ 2026-10-20 5:43 UTC (permalink / raw)
To: Ridong Chen
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Kairui Song, Qi Zheng,
Shakeel Butt, Axel Rasmussen, Yuanchu Xie, Wei Xu, Baoquan He,
Baolin Wang, linux-mm, linux-kernel, Ridong Chen
On Tue, Oct 20, 2026 at 10:36 AM Ridong Chen <ridong.chen@linux.dev> wrote:
>
> From: Ridong Chen <chenridong@xiaomi.com>
>
> 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@xiaomi.com>
> ---
Hi Ridong,
The cleanup might be right, but this part is changing rapidly
with Kairui's MGLRU-FG series. Maybe we should wait until
that series lands and then see if this cleanup is still needed?
Best Regards
Barry
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-20 5:43 ` Barry Song
@ 2026-10-20 6:27 ` Ridong Chen
2026-10-20 8:21 ` Kairui Song
0 siblings, 1 reply; 10+ messages in thread
From: Ridong Chen @ 2026-10-20 6:27 UTC (permalink / raw)
To: Barry Song, Kairui Song
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Qi Zheng, Shakeel Butt,
Axel Rasmussen, Yuanchu Xie, Wei Xu, Baoquan He, Baolin Wang,
linux-mm, linux-kernel, Ridong Chen
On 10/20/2026 1:43 PM, Barry Song wrote:
> On Tue, Oct 20, 2026 at 10:36 AM Ridong Chen <ridong.chen@linux.dev> wrote:
>>
>> From: Ridong Chen <chenridong@xiaomi.com>
>>
>> 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@xiaomi.com>
>> ---
>
> Hi Ridong,
>
> The cleanup might be right, but this part is changing rapidly
> with Kairui's MGLRU-FG series. Maybe we should wait until
> that series lands and then see if this cleanup is still needed?
>
Sure, I'm fine to wait. Looking at MGLRU-FG v4, though, only [4/17]
touches this function, and it mostly adapts the workingset checks to
LRU refs. The local is still called "gen" and the table still leaves
out the reclaiming and "can't evict yet" branches, so the function
stays just as confusing after the series.
Or,Kairui, do you think this is worth cleaning up? If so, would you
mind adding a patch to MGLRU-FG to clean it up, so it doesn't
conflict with your changes?
--
Best regards
Ridong
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-20 2:35 [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq() Ridong Chen
2026-10-20 5:43 ` Barry Song
@ 2026-10-20 8:20 ` Kairui Song
2026-10-20 8:29 ` Barry Song
2026-10-21 8:38 ` wangzicheng
2 siblings, 1 reply; 10+ messages in thread
From: Kairui Song @ 2026-10-20 8:20 UTC (permalink / raw)
To: Ridong Chen
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Qi Zheng, Shakeel Butt,
Barry Song, Axel Rasmussen, Yuanchu Xie, Wei Xu, Baoquan He,
Baolin Wang, linux-mm, linux-kernel, Ridong Chen
On Tue, Oct 20, 2026 at 10:36 AM Ridong Chen <ridong.chen@linux.dev> wrote:
>
> From: Ridong Chen <chenridong@xiaomi.com>
>
> 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@xiaomi.com>
> ---
> 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 |
> + * +-----------------+-----------------+-----------------+-----------------+
> + * seq | max_seq | max_seq-1 | max_seq-2 | max_seq-3 |
I think this comment update is not beneficial, we might have more than
4 gens in the furture, so hardcoding a number is not a good idea.
> + * +-----------------------------------+-----------------------------------+
> + * |<---------- 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 | | |
I think it's better not to describe too many details in the comment as
it actually easier to just look at the code.
> + * +-----------------+-----------------+-----------------+-----------------+
> + * 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);
That's a very nice rename, the `gen` variable is so confusing to me
for a long time, I thought about renaming it to `gap`, `age` looks
much better. Thanks!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-20 6:27 ` Ridong Chen
@ 2026-10-20 8:21 ` Kairui Song
0 siblings, 0 replies; 10+ messages in thread
From: Kairui Song @ 2026-10-20 8:21 UTC (permalink / raw)
To: Ridong Chen
Cc: Barry Song, Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Qi Zheng, Shakeel Butt,
Axel Rasmussen, Yuanchu Xie, Wei Xu, Baoquan He, Baolin Wang,
linux-mm, linux-kernel, Ridong Chen
On Tue, Oct 20, 2026 at 2:28 PM Ridong Chen <ridong.chen@linux.dev> wrote:
> On 10/20/2026 1:43 PM, Barry Song wrote:
> > On Tue, Oct 20, 2026 at 10:36 AM Ridong Chen <ridong.chen@linux.dev> wrote:
> >>
> >> From: Ridong Chen <chenridong@xiaomi.com>
> >>
> >> 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@xiaomi.com>
> >> ---
> >
> > Hi Ridong,
> >
> > The cleanup might be right, but this part is changing rapidly
> > with Kairui's MGLRU-FG series. Maybe we should wait until
> > that series lands and then see if this cleanup is still needed?
> >
> Sure, I'm fine to wait. Looking at MGLRU-FG v4, though, only [4/17]
> touches this function, and it mostly adapts the workingset checks to
> LRU refs. The local is still called "gen" and the table still leaves
> out the reclaiming and "can't evict yet" branches, so the function
> stays just as confusing after the series.
>
> Or,Kairui, do you think this is worth cleaning up? If so, would you
> mind adding a patch to MGLRU-FG to clean it up, so it doesn't
> conflict with your changes?
Sure, I can rebase or add this patch in next FG series, BTW for the
comment part I left some review reply, I think we better keep that
part simpler as the if branches ain't too complex to follow, repeating
it again in the comment seems not helpful. How do you think?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-20 8:20 ` Kairui Song
@ 2026-10-20 8:29 ` Barry Song
2026-10-20 9:27 ` Ridong Chen
0 siblings, 1 reply; 10+ messages in thread
From: Barry Song @ 2026-10-20 8:29 UTC (permalink / raw)
To: Kairui Song
Cc: Ridong Chen, Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Qi Zheng, Shakeel Butt,
Axel Rasmussen, Yuanchu Xie, Wei Xu, Baoquan He, Baolin Wang,
linux-mm, linux-kernel, Ridong Chen
On Tue, Oct 20, 2026 at 4:20 PM Kairui Song <ryncsn@gmail.com> wrote:
>
> On Tue, Oct 20, 2026 at 10:36 AM Ridong Chen <ridong.chen@linux.dev> wrote:
> >
> > From: Ridong Chen <chenridong@xiaomi.com>
> >
> > 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@xiaomi.com>
> > ---
> > 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 |
> > + * +-----------------+-----------------+-----------------+-----------------+
> > + * seq | max_seq | max_seq-1 | max_seq-2 | max_seq-3 |
>
> I think this comment update is not beneficial, we might have more than
> 4 gens in the furture, so hardcoding a number is not a good idea.
>
> > + * +-----------------------------------+-----------------------------------+
> > + * |<---------- 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 | | |
>
> I think it's better not to describe too many details in the comment as
> it actually easier to just look at the code.
>
> > + * +-----------------+-----------------+-----------------+-----------------+
> > + * 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);
>
> That's a very nice rename, the `gen` variable is so confusing to me
> for a long time, I thought about renaming it to `gap`, `age` looks
> much better. Thanks!
Agreed with the rename part. Maybe this doesn't conflict much
with MGLRU-FG either. It could be cherry-picked before
MGLRU-FG or included as part of the MGLRU-FG series.
I'd vote for the latter.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-20 8:29 ` Barry Song
@ 2026-10-20 9:27 ` Ridong Chen
2026-10-20 16:00 ` Kairui Song
0 siblings, 1 reply; 10+ messages in thread
From: Ridong Chen @ 2026-10-20 9:27 UTC (permalink / raw)
To: Barry Song, Kairui Song
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Qi Zheng, Shakeel Butt,
Axel Rasmussen, Yuanchu Xie, Wei Xu, Baoquan He, Baolin Wang,
linux-mm, linux-kernel, Ridong Chen
On 10/20/2026 4:29 PM, Barry Song wrote:
> On Tue, Oct 20, 2026 at 4:20 PM Kairui Song <ryncsn@gmail.com> wrote:
>>
>> On Tue, Oct 20, 2026 at 10:36 AM Ridong Chen <ridong.chen@linux.dev> wrote:
>>>
>>> From: Ridong Chen <chenridong@xiaomi.com>
>>>
>>> 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@xiaomi.com>
>>> ---
>>> 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 |
>>> + * +-----------------+-----------------+-----------------+-----------------+
>>> + * seq | max_seq | max_seq-1 | max_seq-2 | max_seq-3 |
>>
>> I think this comment update is not beneficial, we might have more than
>> 4 gens in the furture, so hardcoding a number is not a good idea.
>>
>>> + * +-----------------------------------+-----------------------------------+
>>> + * |<---------- 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 | | |
>>
>> I think it's better not to describe too many details in the comment as
>> it actually easier to just look at the code.
>>
Fair points.
What confused me about the current table is that it is not clear
which dimension it is drawn along. The columns are generations, but
the rows mix access paths (page tables vs. file descriptors) with
the flags that protect each of them, and some of those, like
LRU_REFS_FLAGS, are not inputs to lru_gen_folio_seq() at all. So
when reading it next to the code, it is hard to tell which branch
puts a folio where.
How about keeping the existing table for the design overview, and
just adding a short note above it on what "age" means, without
hardcoding the number of generations, e.g.:
/*
* The folio is added to seq = max_seq + 1 - age, so age 1 is
* the youngest generation. Ages up to MIN_NR_GENS are active,
* and the result is clamped to min_seq[type].
*/
>>> + * +-----------------+-----------------+-----------------+-----------------+
>>> + * 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);
>>
>> That's a very nice rename, the `gen` variable is so confusing to me
>> for a long time, I thought about renaming it to `gap`, `age` looks
>> much better. Thanks!
>
> Agreed with the rename part. Maybe this doesn't conflict much
> with MGLRU-FG either. It could be cherry-picked before
> MGLRU-FG or included as part of the MGLRU-FG series.
> I'd vote for the latter.
Sounds good to me. Kairui, would you like to pick up the rename, and
the comment change above if you agree, as part of MGLRU-FG?
--
Best regards
Ridong
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-20 9:27 ` Ridong Chen
@ 2026-10-20 16:00 ` Kairui Song
0 siblings, 0 replies; 10+ messages in thread
From: Kairui Song @ 2026-10-20 16:00 UTC (permalink / raw)
To: Ridong Chen
Cc: Barry Song, Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Qi Zheng, Shakeel Butt,
Axel Rasmussen, Yuanchu Xie, Wei Xu, Baoquan He, Baolin Wang,
linux-mm, linux-kernel, Ridong Chen
On Tue, Oct 20, 2026 at 5:27 PM Ridong Chen <ridong.chen@linux.dev> wrote:
> On 10/20/2026 4:29 PM, Barry Song wrote:
> > On Tue, Oct 20, 2026 at 4:20 PM Kairui Song <ryncsn@gmail.com> wrote:
> >>
> >> On Tue, Oct 20, 2026 at 10:36 AM Ridong Chen <ridong.chen@linux.dev> wrote:
> >>>
> >>> From: Ridong Chen <chenridong@xiaomi.com>
> >>>
> >>> 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@xiaomi.com>
> >>> ---
> >>> 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 |
> >>> + * +-----------------+-----------------+-----------------+-----------------+
> >>> + * seq | max_seq | max_seq-1 | max_seq-2 | max_seq-3 |
> >>
> >> I think this comment update is not beneficial, we might have more than
> >> 4 gens in the furture, so hardcoding a number is not a good idea.
> >>
> >>> + * +-----------------------------------+-----------------------------------+
> >>> + * |<---------- 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 | | |
> >>
> >> I think it's better not to describe too many details in the comment as
> >> it actually easier to just look at the code.
> >>
>
> Fair points.
>
> What confused me about the current table is that it is not clear
> which dimension it is drawn along. The columns are generations, but
> the rows mix access paths (page tables vs. file descriptors) with
> the flags that protect each of them, and some of those, like
> LRU_REFS_FLAGS, are not inputs to lru_gen_folio_seq() at all. So
> when reading it next to the code, it is hard to tell which branch
> puts a folio where.
>
> How about keeping the existing table for the design overview, and
> just adding a short note above it on what "age" means, without
> hardcoding the number of generations, e.g.:
>
> /*
> * The folio is added to seq = max_seq + 1 - age, so age 1 is
> * the youngest generation. Ages up to MIN_NR_GENS are active,
> * and the result is clamped to min_seq[type].
> */
>
> >>> + * +-----------------+-----------------+-----------------+-----------------+
> >>> + * 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);
> >>
> >> That's a very nice rename, the `gen` variable is so confusing to me
> >> for a long time, I thought about renaming it to `gap`, `age` looks
> >> much better. Thanks!
> >
> > Agreed with the rename part. Maybe this doesn't conflict much
> > with MGLRU-FG either. It could be cherry-picked before
> > MGLRU-FG or included as part of the MGLRU-FG series.
> > I'd vote for the latter.
>
> Sounds good to me. Kairui, would you like to pick up the rename, and
> the comment change above if you agree, as part of MGLRU-FG?
Good idea, will do!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-20 2:35 [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq() Ridong Chen
2026-10-20 5:43 ` Barry Song
2026-10-20 8:20 ` Kairui Song
@ 2026-10-21 8:38 ` wangzicheng
2026-10-22 6:41 ` Ridong Chen
2 siblings, 1 reply; 10+ messages in thread
From: wangzicheng @ 2026-10-21 8:38 UTC (permalink / raw)
To: Ridong Chen
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Kairui Song, Qi Zheng,
Shakeel Butt, Barry Song, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Baoquan He, Baolin Wang, linux-mm, linux-kernel, Ridong Chen
On 2026-10-20 10:35 +0800, Ridong Chen wrote:
> From: Ridong Chen <chenridong@xiaomi.com>
>
> 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@xiaomi.com>
> ---
> 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.
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.
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]));
> }
>
> static inline bool lru_gen_add_folio(struct lruvec *lruvec, struct folio *folio, bool reclaiming)
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()
2026-10-21 8:38 ` wangzicheng
@ 2026-10-22 6:41 ` Ridong Chen
0 siblings, 0 replies; 10+ messages in thread
From: Ridong Chen @ 2026-10-22 6:41 UTC (permalink / raw)
To: wangzicheng
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
Suren Baghdasaryan, Michal Hocko, Kairui Song, Qi Zheng,
Shakeel Butt, Barry Song, Axel Rasmussen, Yuanchu Xie, Wei Xu,
Baoquan He, Baolin Wang, linux-mm, linux-kernel, Ridong Chen
On 10/21/2026 4:38 PM, wangzicheng@honor.com wrote:
> On 2026-10-20 10:35 +0800, Ridong Chen wrote:
>> From: Ridong Chen <chenridong@xiaomi.com>
>>
>> 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@xiaomi.com>
>> ---
>> 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.
> 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]));
>> }
>>
>> static inline bool lru_gen_add_folio(struct lruvec *lruvec, struct folio *folio, bool reclaiming)
>> --
>> 2.34.1
>>
>>
>
--
Best regards
Ridong
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-10-22 6:41 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-20 2:35 [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq() Ridong Chen
2026-10-20 5:43 ` Barry Song
2026-10-20 6:27 ` Ridong Chen
2026-10-20 8:21 ` Kairui Song
2026-10-20 8:20 ` Kairui Song
2026-10-20 8:29 ` Barry Song
2026-10-20 9:27 ` Ridong Chen
2026-10-20 16:00 ` Kairui Song
2026-10-21 8:38 ` wangzicheng
2026-10-22 6:41 ` Ridong Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®