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

Got this from sashiko (I consider it false negative since that is
extremely unlikely to be an issue):

--- Patch [2]: [PATCH v2 2/6] mm/mglru: introduce helpers for
manipulating gen and refs flags ---
commit f0369e7dfac3789b1433352b10e442443527f4cc
Author: Kairui Song <[email protected]>

mm/mglru: introduce helpers for manipulating gen and refs flags

This commit introduces helpers for adjusting a folio's refs and generation
info. It replaces bit operations on folio->flags.f, splitting some combined
operations under the assumption that the resulting transient states are
harmless.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c1404a59523d6..080132997d876 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_lru_refs(folio, 0);
> + folio_set_workingset(folio);

The commit message mentions that splitting these atomic operations only
creates harmless transient states, but could this specific split break
lockless MGLRU protection?

Because lru_gen_set_refs() can be called locklessly (e.g., from
lru_gen_look_around() holding only the page table lock), this creates a
transient window where the folio has both its LRU references cleared
(refs = 0) and the PG_workingset flag not yet set.

Concurrently, evict_folios() runs under the LRU lock and evaluates folios
for eviction using sort_folio():

mm/vmscan.c:sort_folio() {
    int refs = folio_lru_refs(folio);
    bool workingset = folio_test_workingset(folio);
    int tier = lru_tier_from_refs(refs, workingset);
    ...
    if (tier > tier_idx || refs + workingset == BIT(LRU_REFS_WIDTH) + 1) {
    ...
}

If sort_folio() reads the folio's flags during this transient window,
it will see refs = 0 and workingset = false, causing tier to evaluate to 0.

Will this cause the MGLRU protection check (tier > tier_idx) to fail and
incorrectly isolate actively used executable file folios for eviction?

>   return true;
>   }
>
> - set_mask_bits(&folio->flags.f, LRU_REFS_MASK, BIT(PG_referenced));
> + folio_set_lru_refs(folio, 1);
>   return false;
>   }
>
>   /* Promote on second access */
> - if (folio_lru_refs(folio) > 1)
> - set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, BIT(PG_workingset));
> - else
> + if (folio_lru_refs(folio) > 1) {
> + folio_set_lru_refs(folio, 0);
> + folio_set_workingset(folio);

Does this have the same transient state race with sort_folio() as the split
above, potentially causing actively used folios to be wrongly evicted on
their second access?

> + } else {
>   folio_mark_accessed(folio);
> + }
>   return true;
>  }

===

In theory, yes. In practise, it's very unlikely to occur, and even if
it did, we are totally fine.

And I have often observed that folios stuck in highest tier due to
PG_workingset is actually causing worse reclaim. But if we are very
paranoid here we can do folio_set_workingset and then
folio_set_lru_refs. (This may cause a in theory, still very very
unlikely, aggressive protection. And in fact I think we are definitely
fine either way). I might update this if there is a v3.