Re: [PATCH v3 2/6] mm/mglru: introduce helpers for manipulating gen and refs flags

[Ridong Chen <ridong.chen@linux.dev>]

On 8/29/2026 12:21 PM, Barry Song wrote:
> On Wed, Aug 26, 2026 at 1:53 AM Kairui Song via B4 Relay
> <devnull+kasong.tencent.com@kernel.org> wrote:
>>
>> From: Kairui Song <kasong@tencent.com>
>>
>> Instead of doing bit ops on folio->flags.f, introduce helpers for
>> adjusting a folio's refs and generation info, making the code easier
>> to debug and understand.
>>
>> No functional change is intended: some combined atomic operations are
>> split into two, which only creates harmless transient states. There is
>> no measurable performance impact, and some paths even look slightly
>> better in the generated assembly.
> 
> Hi Kairui,
> 
> I like your idea. Overall, it looks good to me. With some cleanup,
> we might have the following:
> 
>>
>> Signed-off-by: Kairui Song <kasong@tencent.com>
>> ---
>>   include/linux/mm_inline.h | 76 ++++++++++++++++++++++++++++++++++++++++++-----
>>   include/linux/mmzone.h    |  1 +
>>   mm/folio.c                | 19 +++++++-----
>>   mm/vmscan.c               | 61 ++++++++++++++++++++-----------------
>>   4 files changed, 114 insertions(+), 43 deletions(-)
>>
>> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
>> index 621c8653d8f7..edfaf2661812 100644
>> --- a/include/linux/mm_inline.h
>> +++ b/include/linux/mm_inline.h
>> @@ -142,10 +142,42 @@ static inline int lru_tier_from_refs(int refs, bool workingset)
>>          return workingset ? MAX_NR_TIERS - 1 : order_base_2(refs);
>>   }
>>
>> -static inline int folio_lru_refs(const struct folio *folio)
>> +/**
>> + * lru_gen_from_flags - Return the LRU generation number from folio flags.
>> + * @flags: folio flags
>> + *
>> + * Returns: A number between 0 and (MAX_NR_GENS - 1), inclusive. Returns
>> + * -1 if the flags indicate the folio is off the list (e.g., isolated).
>> + */
>> +static inline int lru_gen_from_flags(unsigned long flags)
>> +{
>> +       int gen = ((flags & LRU_GEN_MASK) >> LRU_GEN_PGOFF);
>> +
>> +       BUILD_BUG_ON(LRU_GEN_MASK & LRU_REFS_MASK);
>> +       gen -= 1;
>> +       VM_WARN_ON_ONCE(gen != -1 && gen >= MAX_NR_GENS);
> 
> Since `gen` is an `int`, it seems a bit odd to have
> `gen != -1 && gen >= MAX_NR_GENS` combined here.
> 

Agreed. Especially with the gen -= 1 right above, it took me a while to figure 
out what that actually meant.

> Do you actually mean the following instead?
> 
> VM_WARN_ON_ONCE(gen < -1 || gen >= MAX_NR_GENS);
> 

Maybe we could add a macro like the one I mentioned in my earlier reply:

GENS_UNREF -1

>> +       return gen;
>> +}
>> +
>> +/**
>> + * lru_gen_set_flags - Set the LRU generation number to specified folio flags.
>> + * @flags: pointer to the folio flags
>> + * @gen: generation number, between 0 and (MAX_NR_GENS - 1), inclusive.
>> + */
>> +static inline void lru_gen_set_flags(unsigned long *flags, int gen)
>>   {
>> -       unsigned long flags = READ_ONCE(folio->flags.f);
>> +       VM_WARN_ON_ONCE(gen >= MAX_NR_GENS || gen < 0);
>> +
>> +       *flags &= ~LRU_GEN_MASK;
>> +       *flags |= (gen + 1UL) << LRU_GEN_PGOFF;
>> +}
> 
> It seems that we are setting the gen in the flags, rather than
> setting the flags themselves.
> 
> The current name makes me think that the function is setting the
> entire flags field, especially since `lru_gen` is used as the
> namespace for various MGLRU functions, e.g.:
> 
> void lru_gen_add_mm(struct mm_struct *mm);
> void lru_gen_del_mm(struct mm_struct *mm);
> void lru_gen_migrate_mm(struct mm_struct *mm);
> struct lru_gen_folio;
> lru_gen_update_size(lruvec, folio, old_gen, new_gen);
> 
> However, this function only sets the gen bits in the flags.
> Maybe we could use a name that makes this distinction clearer?
> 
> Could they be named as below, or is there a better naming option?
> 
> set_gen_to_flags(unsigned long *flags, int gen);
> get_gen_from_flags(unsigned long flags);
> 
> Or something like flags_to_gen() or gen_from_flags()?
> 
>>
>> +/**
>> + * lru_refs_from_flags - Return LRU referenced / access count from folio flags.
>> + * @flags: folio flags
>> + */
>> +static inline int lru_refs_from_flags(unsigned long flags)
>> +{
>>          if (!(flags & BIT(PG_referenced)))
>>                  return 0;
>>          /*
>> @@ -155,11 +187,40 @@ static inline int folio_lru_refs(const struct folio *folio)
>>          return ((flags & LRU_REFS_MASK) >> LRU_REFS_PGOFF) + 1;
>>   }
>>
>> -static inline int folio_lru_gen(const struct folio *folio)
>> +/**
>> + * lru_refs_set_flags - Set the LRU referenced / access count to specified folio flags.
>> + * @flags: pointer to the folio flags
>> + * @refs: referenced / access count number, between 0 and LRU_REFS_MAX, inclusive.
>> + */
>> +static inline void lru_refs_set_flags(unsigned long *flags, unsigned int refs)
>> +{
>> +       VM_WARN_ON_ONCE(refs > LRU_REFS_MAX);
>> +       BUILD_BUG_ON(LRU_REFS_MAX != (LRU_REFS_MASK >> LRU_REFS_PGOFF) + 1);
>> +
>> +       *flags &= ~LRU_REFS_FLAGS;
>> +       if (!refs)
>> +               return;
>> +       *flags |= (BIT(PG_referenced) | ((refs - 1UL) << LRU_REFS_PGOFF));
>> +}
> 
> Similar to the above, maybe we could use more descriptive names?
> 
>> +
>> +static inline int folio_lru_refs(const struct folio *folio)
>>   {
>> -       unsigned long flags = READ_ONCE(folio->flags.f);
>> +       return lru_refs_from_flags(READ_ONCE(*const_folio_flags(folio, 0)));
>> +}
>> +
>> +static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
>> +{
>> +       unsigned long new_flags, old_flags = READ_ONCE(*folio_flags(folio, 0));
>> +
>> +       do {
>> +               new_flags = old_flags;
>> +               lru_refs_set_flags(&new_flags, refs);
>> +       } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
>> +}
>>
>> -       return ((flags & LRU_GEN_MASK) >> LRU_GEN_PGOFF) - 1;
> 
> [...]
> 
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index 73a81b4a3e16..9ee9f8dc6805 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -843,19 +843,22 @@ static bool lru_gen_set_refs(struct folio *folio, const vma_flags_t *vma_flags)
>>          if (!folio_test_referenced(folio) && !folio_test_workingset(folio)) {
>>                  /* Activate file-backed executable folios after first usage. */
>>                  if (is_exec_file_folio(folio, vma_flags)) {
>> -                       set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, BIT(PG_workingset));
>> +                       folio_set_workingset(folio);
>> +                       folio_set_lru_refs(folio, 0);
>>                          return true;
>>                  }
>>
>> -               set_mask_bits(&folio->flags.f, LRU_REFS_MASK, BIT(PG_referenced));
>> +               folio_set_lru_refs(folio, 1);
> 
> We have quite a few instances of folio_set_lru_refs(folio, 1) and
> folio_set_lru_refs(folio, 0). Do these values have any special
> meaning? Could we add wrapper helpers to make the code more readable?
> 
> `1` just means `PG_referenced`, right? Could we add a comment or
> changelog description to clarify that `1` actually means `refs = 0`,
> but `PG_referenced` is set?
> 
> Thanks
> Barry

-- 
Best regards
Ridong