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

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.

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.

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.

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.

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.

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.

Thanks,
Joanne