Re: [PATCH v3 2/6] mm/mglru: introduce helpers for manipulating gen and refs flags
From: Ridong Chen
Date: Sun Aug 30 2026 - 05:08:45 EST
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@xxxxxxxxxx> wrote:
From: Kairui Song <kasong@xxxxxxxxxxx>
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@xxxxxxxxxxx>
---
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