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

On 8/19/26 5:37 PM, Kairui Song wrote:
> On Wed, Aug 19, 2026 at 5:03 PM Baolin Wang
> <[email protected]> wrote:
>> On 8/18/26 1:38 PM, Kairui Song via B4 Relay wrote:
>>> From: Kairui Song <[email protected]>
>>>
>>> Instead of doing bit ops on folio->flags.f, introduce helpers for
>>> adjusting folio's refs and gen info, make 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.
>>>
>>> Signed-off-by: Kairui Song <[email protected]>
>>> ---
>>
>> Thanks for the cleanups. One comment below.
>>
> 
> Thanks for the review!
> 
>>> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
>>> index 94f9c3ff5416..32d9354a754f 100644
>>> --- a/include/linux/mmzone.h
>>> +++ b/include/linux/mmzone.h
>>> @@ -496,7 +496,9 @@ enum lruvec_flags {
>>>    #ifndef __GENERATING_BOUNDS_H
>>>
>>>    #define LRU_GEN_MASK                ((BIT(LRU_GEN_WIDTH) - 1) << LRU_GEN_PGOFF)
>>> +#define LRU_GEN_MAX          (BIT(LRU_GEN_WIDTH - 1) - 1)
>>
>> There are still many places using MAX_NR_GENS to compare gen counters,
>> so I'm concerned that introducing a new macro would make the use of the
>> max gen counter even more confusing.
> 
> I think I'd like to get rid of all LRU_GEN_MASK and LRU_REFS_MASK
> users, MAX_NR_GENS is fine I think? Manipulating the bitmask looks a
> bit hard to follow for me since:
> 
> set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, BIT(PG_workingset));
> set_mask_bits(&folio->flags.f, LRU_REFS_MASK, BIT(PG_referenced));
> set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, <number>);
> set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, BIT(PG_active));
> set_mask_bits(&folio->flags.f, LRU_REFS_MASK, <number>);
> new_flags = old_flags & ~(LRU_GEN_MASK | LRU_REFS_FLAGS);
> new_flags |= ((gen + 1UL) << LRU_GEN_PGOFF) | BIT(PG_workingset);
> 
> Things like this are everywhere and I found it really hard to follow
> what is actually going on. And adding more operations to refs/gen
> looks ugly and painful to do, and, it's impossible to add any sanity
> check. With proper gen and refs helpers, we can ensure every time
> gen/refs is modified, the value is valid (in debug builds).

I agree LRU_GEN_MASK and LRU_REFS_MASK can be cleaned up further.

> 
> Mean while, a "% MAX_NR_GENS" or "/ MIN_NR_GEN" seems not hard to understand.
> 
> Oh and this LRU_GEN_MAX is only used for sanity check for now.

But my point is that people may not know when to use LRU_GEN_MAX vs. 
MAX_NR_GENS, since they seem to have the same semantics.