Re: [PATCH RFC v2 02/15] mm/mglru: make generation page counters atomic

[Kairui Song]

Sashiko reviewed this, I think it's just false positives, see "===" below.

commit 5d43c1cf23cfcb5744723aabee21da2f8bffb50a
Author: Kairui Song <kasong@xxxxxxxxxxx>

mm/mglru: make generation page counters atomic

This commit converts the MGLRU generation page counters to atomic_long_t.
This enables lockless updates to the counters without holding the LRU lock
while ensuring final consistency.

> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> index 597f013c8e046..f52f02e8e5be1 100644
> --- a/include/linux/mm_inline.h
> +++ b/include/linux/mm_inline.h
[ ... ]

This is a pre-existing issue, but looking at lru_gen_add_folio() in
include/linux/mm_inline.h, it updates the folio's MGLRU generation but
fails to clear its tier bits:

include/linux/mm_inline.h:lru_gen_add_folio() {
    ...
    set_mask_bits(folio_flags(folio, 0), LRU_GEN_MASK | BIT(PG_active), flags);
    ...
}

Does this leave stale tier bits intact? The MM Reclaim documentation states
that all paths modifying LRU_GEN_MASK must also clear LRU_REFS_FLAGS,
otherwise stale tier bits inflate access counts and distort MGLRU eviction
decisions. Could we add a fix for this to the series?

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index aaceed4759eeb..32890e628240f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
[ ... ]
> @@ -4720,7 +4717,7 @@ static void __lru_gen_reparent_memcg(struct lruvec *child_lruvec, struct lruvec
>
>   for (i = 0; i < get_nr_gens(child_lruvec, type); i++) {
>   int gen = lru_gen_from_seq(child_lrugen->max_seq - i);
> - long nr_pages = child_lrugen->nr_pages[gen][type][zone];
> + long nr_pages = atomic_long_read(&child_lrugen->nr_pages[gen][type][zone]);
>   int child_lru_active = lru_gen_is_active(child_lruvec, gen) ? LRU_ACTIVE : 0;
>   int parent_lru_active = lru_gen_is_active(parent_lruvec, gen) ? LRU_ACTIVE : 0;
>
> @@ -4728,9 +4725,8 @@ static void __lru_gen_reparent_memcg(struct lruvec *child_lruvec, struct lruvec
>   list_splice_tail_init(&child_lrugen->folios[gen][type][zone],
>        &parent_lrugen->folios[gen][type][zone]);
>
> - WRITE_ONCE(child_lrugen->nr_pages[gen][type][zone], 0);
> - WRITE_ONCE(parent_lrugen->nr_pages[gen][type][zone],
> -   parent_lrugen->nr_pages[gen][type][zone] + nr_pages);
> + atomic_long_set(&child_lrugen->nr_pages[gen][type][zone], 0);
> + atomic_long_add(nr_pages, &parent_lrugen->nr_pages[gen][type][zone]);

Is there a TOCTOU race in __lru_gen_reparent_memcg() overwriting concurrent
lockless updates to nr_pages?

This patch explicitly converts nr_pages to atomic_long_t to allow lockless
updates without the LRU lock. However, the code reads the counter at the top
of the loop:

    long nr_pages = atomic_long_read(&child_lrugen->nr_pages[gen][type][zone]);

and then later clears it using atomic_long_set() as shown above. If a
concurrent lockless update happens between the read and the zeroing, wouldn't
the update be permanently lost?

Could this cause permanent divergence between actual LRU list sizes and
recorded nr_pages, corrupting eviction statistics and causing over/under
reclaim logic errors? Should this safely clear the counter against concurrent
writers by using atomic_long_xchg() instead?

===

The first lru_gen_add_folio part is false positive, we want to retain
the refs info, not clean it, at least in this series. For existing
cases, maybe it's actually not a good idea to always clear the refs
info as well. But anyway it's not a bug introduced here.

The second reparent issue, __lru_gen_reparent_memcg only occurs for
dead lruvec, and all users only do this for alive ones. Maybe I can
add some VM_WARN_ON_ONCE and a Context: kdoc here in next version,
it's not a bug but good to be cleaner indeed.