Re: memcg stats flushing (WAS "Re: [PATCH] fuse: disable default bdi strictlimiting")

On Wed, Aug 12, 2026 at 1:59 PM Joanne Koong <[email protected]> wrote:
>
> On Mon, Aug 10, 2026 at 5:48 PM Yosry Ahmed <[email protected]> wrote:
> >
> > On Mon, Aug 10, 2026 at 09:49:31AM -0700, Joanne Koong wrote:
> > > On Thu, Aug 6, 2026 at 2:31 PM Yosry Ahmed <[email protected]> wrote:
> > > >
> > > > On Thu, Aug 6, 2026 at 2:04 PM Joanne Koong <[email protected]> wrote:
> > > > >
> > > > > I'm not too familiar with memcg or vmstat so apologies if this is a
> > > > > naive question, but does it make sense for memcg to just do a similar
> > > > > approach to what vmstat does? As I understand it, mod_lruvec_state()
> > > > > updates the node counter and the memcg counter (if memcg applies),
> > > > > where for updating the node counter, mod_node_state() does some
> > > > > batching where it updates the global counter for that stat only once
> > > > > it crosses some threshold (pcp->stat_threshold). Could memcg keep its
> > > > > per-CPU counter as the accumulator but once it reaches some threshold,
> > > > > it then propagates it up the parent/ancestors into a per-memcg atomic
> > > > > global counter? The atomic would then only have to be touched once per
> > > > > batch than on every update, which might eliminate the overhead of the
> > > > > per-memcg atomic you saw previously? It seems like this would get rid
> > > > > of needing to do any flushing altogether, as we could just read that
> > > > > global counter directly.
> > > >
> > > > The memcg stats perform similar per-CPU counting and thresholding. The
> > > > main difference from vmstat AFAICT is the cgroup hierarchy. In vmstat
> > > > we have per-CPU counters and a global atomic counter for each stat.
> > > > For memcg, we have the same for each cgroup, then we also have a
> > > > cgroup tree. When we read the stats of a cgroup, we usually want the
> > > > hierarchical stats including all of its children, so we can't just
> > > > read the global counter.
> > > >
> > > > We use rstat, a cgroup framework that keeps track of which cgroups
> > > > have updates on which CPUs, and then memcg has thresholding logic on
> > > > top to only flush if the number of pending updates exceeds a
> > > > threshold. Essentially, if the magnitude updates on a CPU exceeds
> > > > MEMCG_CHARGE_BATCH, we add it to a global per-memcg counter. We only
> > > > flush if that global per-memcg counter has > NR_CPUS *
> > > > MEMCG_CHARGE_BATCH updates. So in theory I think we tolerate up to 2 *
> > > > NR_CPUS * MEMCG_CHARGE_BATCH of stale stats (combined for all the
> > > > stats).
> > > >
> > > > See memcg_rstat_updated() and memcg_vmstats_needs_flush() for this logic.
> > > >
> > > > Looking at this code again, it made me realize that we already iterate
> > > > the per-CPU stats_updates on every update to increment the number of
> > > > pending stats on this CPU. I wonder if we can replace this with
> > > > actually updating the stat on each parent, then the flush path only
> > > > needs to accumulate CPU counters. Or maybe as you mention, we update
> > > > the global per-memcg counter when updates on a CPU exceed a threshold,
> > > > and forgo the rstat flushing logic completely.
> > > >
> > >
> > > I can run some experiments and see if updating the global per-memcg
> > > counter per batch and forgoing flushing could be a viable solution.
> > > I can use my setup to test it for writeback, but for the zswap use
> > > case, what's the best way to test if the changes are too expensive? Is
> > > there a benchmark program you're running?
> >
> > I was running stress-ng and measuring the latency of zswap_load() and
> > zswap_store() with bpftrace. But I suspect whatever works for writeback
> > should work for zswap :)
> >
> > >
> > > For updating the stat on each parent and having the flush path only
> > > accumulate CPU counters, I think this means we would have to
> > > unconditionally walk the ancestors + update its counters on every
> > > update, as we wouldn't be able to early break if the flushable
> > > threshold was already exceeded. I don't think we could replace the
> > > stats_update update, as it seems like readers still need some way of
> > > knowing when to flush. It seems like this could be a nontrivial
> > > performance hit, but maybe it's a non-factor in reality. If you think
> > > this would be useful to get benchmark numbers on, I can run some
> > > experiments on this too.
> >
> > I think we need to experiment with how much work we can shift around
> > between the update and reader sides.
> >
> > Right now, on the update side, we call __css_rstat_updated(), which
> > iterates the parents and puts them on the per-CPU rstat tree, then we
> > iterate the parents again in memcg_rstat_updated() and:
> > - Check if flushing threshold is already met.
> > - Update per-CPU counter for stats_updates.
> > - If per-CPU update exceeds (another) threshold, we update an atomic for
> >   stats_updates.
> >
> > The flush side checks the flushing threshold and does an rstat flush,
> > which will walk the update trees on all CPUs and iterates all child
> > cgroups on it. Pretty expensive when the threshold is met.
> >
> > I think we discussed two different options:
> >
> > (a) On stats updates, iterate the parents and update the per-CPU
> > counters of the stat directly.
> >
> > In this case, we can't keep using rstat, as we would still need to add
> > the cgroup and all its ancestors to the update tree anyway and the flush
> > cost will remain similar. So on the flush path we'll need to iterate all
> > CPUs and add the counters. We can explore if use a cpumask can help
> > here, but perhaps not since we'll need to update it atomically on all
> > stat updates.
> >
> > I imagine that if we do this we'll drop the stats_updates optimization
> > to avoid adding more cost to the update side. We'll be replacing parent
> > stats_updates modification with parent stat counter modification. But
> > maybe it's feasible to keep both to limit the cost of flusing.
> >
> > This only works if the cost of iterate the CPUs on the flush side ends
> > up being cheaper than the current flush. I imagine the average cost will
> > be higher but the cost should be much more consistent vs the current
> > flush, so the tail should be better.
> >
> > (b) On stats updates, iterate the parents and update the per-CPU
> > counters of the stat directly AND update a global atomic if the per-CPU
> > counter exceeds a threshold. I think updates may get too expensive as we
> > may need to update multiple atomics, especially when updating the root
> > counter as it will be contended by all cgroups.
> >
> > The flush side becomes an atomic read, so should be very cheap, and we
> > can drop other heuristics and optimizations.
> >
> > ---
> >
> > I think ultimately we need to experiment with these approachs (and
> > perhaps others) to check what actually works in practice. You'll
> > probably want some benchmarks or synthetic tests with many cgroups to
> > exercise the worst case scenarios.
> >
> > For the zswap stats, I used to run stress-ng with a bunch of workers
> > (20, 50, 100, ..) in cgroups of different levels. Since all workers are
> > in the same cgroups, all the parents are common and all threads end up
> > competing on the same atomics, so worst case scenario. I was also
> > testing in a VM that has more CPUs than stress-ng workers, to exercise
> > the full extent of the concurrency and contention.
> >
> > This could be a lot of work, so I am not really asking you to do it, but
> > just brainstorming and sharing ideas :)
>
> Thanks for your guidance and insights on this, Yosry! Your responses
> have been very helpful.
>
> I'll do some experimentation with this and try to get a sense of what
> the update costs are.

Looking forward to that!

>
> One thing I was unclear about and have been looking at is how
> pervasive/important this issue is outside of the writeback case. It
> looks like there are roughly 4 other categories of users that rely on
> stats that can be 2s stale: zswap, cachestat(), working set, and
> vmscan.
>
> Of these, vmscan is the only one that looks troublesome. Some armchair
> analysis below:
>
> a) zswap (zswap_shrinker_count()): 2s of staleness looks fine. the
> stats are only used to calculate the compression ratio and the ratio
> scales a count that comes from a list_lru and *isn't* stale. The ratio
> is an average over the whole pool, so it seems like that should be
> fairly steady across 2s. A stale result doesn't seem too bad - afaict,
> it is a heuristic of how zswap should get weighed against the other
> shrinkers.

There is also the zswap limits calculation, which Song sent a patch to
change to using ratelimited flushing. I think that case is more
problematic as it can cause OOM kills if we use stale stats (e.g.
after a burst of writeback frees up space in zswap). Adding Song here.

The shrinker case is more benign.

>
> b) cachestat() syscall (filemap_cachestat()): seems preferable to just
> rely on the 2s flusher worker. The results are returned back to
> userspace and not used for any internal kernel decision-making, seems
> ok to have one field in it (nr_recently_evicted) potentially be stale.
> Seems better to err on the side of less frequent flushing anyways, as
> the call path is invoked from unprivileged userspace and flushes the
> root memcg.

I am not sure how the cachestat() syscall is used, but my preference
would be to treat it like memory.stat and do a full flush on userspace
read, unless it's a syscall that is invoked frequently for some
reason.

>
> c) workingset: looking at the original commits, the calculations are
> estimates that don't require exact stats -
>
> refaulting (workingset_test_recent()): commit 9b3016154c91 ("memcg:
> sync flush only if periodic flush is delayed")
>     Now the question: what are the side-effects of this change? The worst
>     that can happen is the refault codepath will see 4sec old lruvec stats
>     and may cause false (or missed) activations of the refaulted page which
>     may under-or-overestimate the workingset size.  Though that is not very
>     concerning as the kernel can already miss or do false activations.

Incorrect activations can lead to thrashing in some cases, but I am
not sure how practical the concern is.

>
> shadow-node shrinker (count_shadow_nodes()): commit d4a5b369ad6d ("mm:
> ratelimit stat flush from workingset shrinker")
>     On further inspection it seems like we don't really need
>     accurate stats in this function as it was already approximating the amount
>     of appropriate shadow entries to keep for maintaining the refault
>     information.
>
> d) vmscan (prepare_scan_control()): for determining whether reclaim
> can deactivate, there's this logic
>
>                 /*
>                  * When refaults are being observed, it means a new
>                  * workingset is being established. Deactivate to get
>                  * rid of any stale active pages quickly.
>                  */
>                 refaults = lruvec_page_state(target_lruvec,
>                                 WORKINGSET_ACTIVATE_ANON);
>                 if (refaults != target_lruvec->refaults[WORKINGSET_ANON] ||
>                         inactive_is_low(target_lruvec, LRU_INACTIVE_ANON))
>                         sc->may_deactivate |= DEACTIVATE_ANON;
>                 else
>                         sc->may_deactivate &= ~DEACTIVATE_ANON;
>
> (and same for DEACTIVATE_FILE for file-backed folios)
>
> target_lruvec->refaults[] gets set at the end of the reclaim pass. It
> stores the same lruvec_page_state() value that prepare_scan_control()
> will read at the start of the next reclaim pass, which means if
> reclaim passes happen more frequently than the 2s flushes (eg under
> sustained/heavy memory pressure), this logic will incorrectly assume
> no refaults occurred and deactivation is disabled, which afaict means
> folios won't be moved from the active -> inactive list, and reclaim
> will start evicting pages from the inactive list that are hot (eg more
> refaults).
>
> This is from reading the code rather than a reproducer though, so I
> don't know how much this matters in practice. Unlike for the writeback
> case, I don't think flushign more frequently solves the problem. It
> would shrink the staleness window but not eliminate it. Maybe one
> solution is to keep the deactivation state for a period of time if
> refaults were observed.

We do have an internal test that runs a VM under memory pressure and
we started observing OOM kills after the ratelimited flush was added
in vmscan, so I think it's more problematic than just reclaiming hot
memory, but I didn't look too closely.

>
> If it turns out iterating the parents and updating the per-CPU
> counters of the stat on updates is too expensive, perhaps it makes
> sense to keep the existing rstat logic as is and have caller-side
> workarounds, as only writeback and vmscan would require them.

Also zswap limits calculation. I am not sure how benign the workingset
case is, but it has been ratelimited for a while now AFAICT, so maybe
it's fine.

>
> Thanks,
> Joanne