Re: [PATCH] mm: memcg: use ratelimited stats flush in obj_cgroup_may_zswap()

[Yosry Ahmed]

On Wed, Aug 26, 2026 at 4:08 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:
>
> On Wed, Aug 26, 2026 at 02:55:57PM -0700, Yosry Ahmed wrote:
> > > >
> [...]
> > > > If (c) is holding up for writeback and vmscan, I would question a more
> > > > radical approach of tearing apart the rstat framework and using it
> > > > across the board. That is obviously a heavier lift and more
> > > > controversial,
> > >
> > > Yes controversial because we used to have similar mechanism which did not work
> > > and we had to move to rstat. Check the commit 42a300353577 and fixes to it.
> >
> > IIUC, in that commit, the update side modifies one per-CPU per-cgroup
> > counters, and when those counters exceed a threshold atomic updates
> > are performed on all parents, which I think is the slow side. Then,
> > the read side just reads the potentially stale atomic. That was
> > problematic because:
> > (1) The update side can end up performing atomic updates on all parents.
> > (2) The read side can be inaccurate by up to MEMCG_CHARGE_BATCH *
> > nr_cpus * nr_children
> >
> > What I am proposing in (c) is different:
> >
> > (1) The update side always updates per-CPU per-cgroup counters in all
> > the parents. It never does atomic operations. The parent iteration is
> > not cheap, but it is cheaper than atomics for sure and we already do
> > parent iteration today in some cases in memcg_rstat_updated().So the
> > "slow" path should be the same as today.
>
> Yes but that parent traversal is short circuited when we hit the flush
> threshold. At Meta scale, we have observed significant cpus being spent on
> memcg_rstat_updated before that.

Interesting. So the upward per-CPU traversal takes time? Perhaps due
to concurrent flushers causing the cachelines to bounce?

>
> > (2) The read side always iterates per-CPU counters in this cgroup, so
> > it's always accurate. This might be more expensive on an rstat flush
> > on average (e.g. if memcg_vmstats_needs_flush() skips the flush), but
> > it is much more consistent and the tail latency is much better.
> >
>
> It might be or not. Usually such things are more clear at scale in production
> instead of benchmarks.

Agreed.

>
> > >
> > > This worked for zswap stats because their update and consumption are not on
> > > performance critical code paths i.e. these are on the way to compress or
> > > decompress or in reclaim context.
> >
> > Well, decompression is in the fault path, that's performance critical
> > to some extent (although not like other stats updated by networking).
> > Anyway, I think the update path shouldn't regress much with the
> > approach described in (c).
>
> Decompression is on the scale of micro-seconds (and I suspect the simple minor
> page faults are on nano-seconds scale) and the upward traversal is definitely
> much cheaper and will be a noise there.
>
> >
> > >
> > > > but if we can get away with it, I think it will
> > > > simplify things greatly and honestly rstat has been causing a lot of
> > > > trouble in the last few years.
> > > >
> > > > But this can be done incrementally too, we can start by separating out
> > > > the problematic stats to use the new update/flushing scheme,
> > >
> > > This I think we do need but for specific stats. I think the stats which have
> > > in-kernel consumers need this. However not all such stats might be fine with
> > > slow update side like zswap. So, we need to evaluate thoroughly.
> >
> > I think in the proposed approach (c) the main concern is actually read
> > latency, not update latency. So if it works for in-kernel consumers it
> > should definitely work for userspace consumers as well?
>
> Here I was mainly talking about consumers which has more strict staleness
> requirements and those are mainly kernel consumers. Rstat flushing is definitely
> more expensive than this for-each-cpu traversal and most of the time they just
> need it for one or very small set of stats.
>
> I am imagining once we have this special mechanism for selected stats, we can
> potentially remove ratelimited flush and stats update threshold code.

Yeah that's the hope. I was basically saying that I am hoping for
more, to transition everything to this flushing scheme if it works
well.

Anyway, as you mentioned, ideally we need production data to proceed.
Joanne, is this something that you are trying to collect by any
chance? :)