Re: [PATCH v2 2/4] mm: memcg: read root lruvec stats from the node counters

["Michal Koutný" <mkoutny@suse.com>]

On Tue, Nov 10, 2026 at 03:10:54PM -0800, Joanne Koong <joannelkoong@gmail.com> wrote:
> On Mon, Nov 9, 2026 at 6:23 PM <sashiko-bot@kernel.org> wrote:
> >
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue to consider:
> > - [Medium] mm: memcg: node_stat_includes_unaccounted() omits file and
> >   writeback stats
> > --
> >
> > commit d2ba314724a8ba015e3bf2a30f155f1beba25a8c
> > Author: Joanne Koong <joannelkoong@gmail.com>
> >
> > mm: memcg: read root lruvec stats from the node counters
> >
> > This commit changes lruvec_page_state() and lruvec_page_state_monotonic()
> > to read per-node vmstat counters instead of rstat values for the root
> > memcg when querying stats that only track memory charged to a memcg.
> >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 1ebc9330aeae5..2575bf2ba0ea6 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> >
> > [ ... ]
> >
> > > @@ -504,13 +511,52 @@ struct lruvec_stats {
> > >       long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
> > >  };
> > >
> > > +/*
> > > + * Whether the node counter for @idx can also count memory that isn't charged
> > > + * to a memcg (eg kernel memory that isn't accounted). For these stats, the
> > > + * node counter can be larger than the root memcg's value, so the root memcg
> > > + * has to read its rstat value instead.
> > > + */
> > > +static bool node_stat_includes_unaccounted(enum node_stat_item idx)
> > > +{
> > > +     switch (idx) {
> > > +     case NR_SLAB_RECLAIMABLE_B:
> > > +     case NR_SLAB_UNRECLAIMABLE_B:
> > > +     case WORKINGSET_NODERECLAIM:
> > > +     case NR_VMALLOC:
> > > +     case NR_KERNEL_STACK_KB:
> > > +     case NR_PAGETABLE:
> > > +     case NR_SECONDARY_PAGETABLE:
> > > +#ifdef CONFIG_HUGETLB_PAGE
> > > +     case NR_HUGETLB:
> > > +#endif
> > > +             return true;
> > > +     default:
> > > +             return false;
> > > +     }
> > > +}
> >
> > [Severity: Medium]
> > Should node_stat_includes_unaccounted() also include NR_FILE_PAGES,
> > NR_FILE_MAPPED, and NR_WRITEBACK?
> >
> > For NR_FILE_PAGES, swapcache operations such as __swap_cache_add_folio(),
> > __swap_cache_del_folio(), and __swap_cache_alloc() update NR_FILE_PAGES
> > only in the node counter while accounting swapcache in the lruvec under
> > NR_SWAPCACHE:
> >
> > mm/swap_state.c:__swap_cache_add_folio() {
> >         ...
> >         node_stat_mod_folio(folio, NR_FILE_PAGES, nr_pages);
> >         lruvec_stat_mod_folio(folio, NR_SWAPCACHE, nr_pages);
> >         ...
> > }
> >
> > Reading node_page_state(pgdat, NR_FILE_PAGES) for the root lruvec
> > therefore includes all swapcache pages, which inflates the "file" counter
> > in the root cgroup's memory.numa_stat (double-counting swapcache alongside
> > "swapcached") and causes it to diverge from memory.stat even after a
> > flush.
> >
> > Similarly, __folio_mod_stat() -> lruvec_stat_mod_folio() updates the node
> > NR_FILE_MAPPED counter without updating memcg rstat whenever an uncharged
> > folio (folio_memcg(folio) == NULL, such as a driver mapping or a
> > root-cgroup __GFP_ACCOUNT kernel folio) is mapped into userspace, and
> > nfs_folio_mark_unstable() and nfs_folio_clear_commit() update
> > NR_WRITEBACK only in the node counter for unstable NFS pages awaiting
> > server COMMIT.
> >
> > --
> > Sashiko AI review · https://sashiko.dev/#/patchset/20261110015321.611093-1-joannelkoong@gmail.com?part=2
> 
> Thanks, this is a good point about the folio stats.
> 
> Hmm... I wonder if we should go with an allowlist instead of a
> denylist. The allowlist is longer (31 stats vs 11) but would be more
> resilient to failures (a stat missing from the allowlist just reads
> the rstat value whereas a stat missing from the denylist will silently
> report the wrong value) for any stats added in the future. Any
> thoughts?

I agree the opposite predicate would be more robust and potentially
easier to review (review only the _listed_ cases, not the "unknown"
complement).

Also, instead of

       if (something && !mem_cgroup_is_root(eviction_memcg))
                mem_cgroup_flush_stats_ratelimited(eviction_memcg);

I'd consider

       if (something)
                mem_cgroup_flush_stats_ratelimited_opt(eviction_memcg,
		1UL<<NR_ACTIVE_ANON|
		1UL<<NR_INACTIVE_ANON| ... );

and collect the exceptional handling in memcontrol.c:

        void mem_cgroup_flush_stats_ratelimited_opt(struct mem_cgroup *memcg, unsigned long needs)
        {
                if ((needs & ~node_stat_equivalent) == 0 &&
                        mem_cgroup_is_root(mem_cgroup))
                        return;
		...

This way in-kernel flushers will be expicitly annotated so that
potential mismatch is more obvious to spot (and it adds possibility of
future optimizations).

(This is all only a concept, it may turn out uglier when worked out to
details...)

HTH,
Michal