Re: [PATCH 6/7] mm: memcontrol: switch to rstat

Hello Michal,

On Thu, Feb 04, 2021 at 03:19:17PM +0100, Michal Hocko wrote:

On Tue 02-02-21 13:47:45, Johannes Weiner wrote:

quoted

Replace the memory controller's custom hierarchical stats code with
the generic rstat infrastructure provided by the cgroup core.

The current implementation does batched upward propagation from the
write side (i.e. as stats change). The per-cpu batches introduce an
error, which is multiplied by the number of subgroups in a tree. In
systems with many CPUs and sizable cgroup trees, the error can be
large enough to confuse users (e.g. 32 batch pages * 32 CPUs * 32
subgroups results in an error of up to 128M per stat item). This can
entirely swallow allocation bursts inside a workload that the user is
expecting to see reflected in the statistics.

In the past, we've done read-side aggregation, where a memory.stat
read would have to walk the entire subtree and add up per-cpu
counts. This became problematic with lazily-freed cgroups: we could
have large subtrees where most cgroups were entirely idle. Hence the
switch to change-driven upward propagation. Unfortunately, it needed
to trade accuracy for speed due to the write side being so hot.

Rstat combines the best of both worlds: from the write side, it
cheaply maintains a queue of cgroups that have pending changes, so
that the read side can do selective tree aggregation. This way the
reported stats will always be precise and recent as can be, while the
aggregation can skip over potentially large numbers of idle cgroups.

This adds a second vmstats to struct mem_cgroup (MEMCG_NR_STAT +
NR_VM_EVENT_ITEMS) to track pending subtree deltas during upward
aggregation. It removes 3 words from the per-cpu data. It eliminates
memcg_exact_page_state(), since memcg_page_state() is now exact.

I am still digesting details and need to look deeper into how rstat
works but removing our own stats is definitely a good plan. Especially
when there are existing limitations and problems that would need fixing.

Just to check that my high level understanding is correct. The
transition is effectivelly removing a need to manually sync counters up
the hierarchy and partially outsources that decision to rstat core. The
controller is responsible just to tell the core how that syncing is done
(e.g. which specific counters etc).

Yes, exactly.

rstat implements a tree of cgroups that have local changes pending,
and a flush walk on that tree. But it's all driven by the controller.

memcg needs to tell rstat 1) when stats in a local cgroup change
e.g. when we do mod_memcg_state() (cgroup_rstat_updated), 2) when to
flush, e.g. before a memory.stat read (cgroup_rstat_flush), and 3) how
to flush one cgroup's per-cpu state and propagate it upward to the
parent during rstat's flush walk (.css_rstat_flush).

Excplicit flushes are needed when you want an exact value (e.g. when
values are presented to the userspace). I do not see any flushes to
be done by the core pro-actively except for clean up on a release.

Is the above correct understanding?

Yes, that's correct.