Hello.
TL;DR rstats are slow but accurate on reader side. To tackle the
performance regression no flush seems simpler than this patch.
So, I've made an overview for myself what were the relevant changes with
rstat introduction.
The amount of work is:
- before
R: O(1)
W: O(tree_depth)
- after
R: O(nr_cpus * nr_cgroups(subtree) * nr_counters)
W: O(tree_depth)
That doesn't look like a positive change especially on the reader side.
(In theory, the reader's work would be amortized but as the original
report here shows, not all workloads are diluting the flushes
sufficiently. [1])
The benefit this was traded for was the greater accuracy, the possible
error is:
- before
- O(nr_cpus * nr_cgroups(subtree) * MEMCG_CHARGE_BATCH) (1)
- after
O(nr_cpus * MEMCG_CHARGE_BATCH) // sync. flush
or
O(flush_period * max_cr) // periodic flush only (2)
// max_cr is per-counter max change rate
So we could argue that if the pre-rstat kernels did just fine with the
error (1), they would not be worse with periodic flush if we can compare
(1) and (2).
On Fri, Mar 04, 2022 at 06:40:40PM +0000, Shakeel Butt <shakeelb@xxxxxxxxxx> wrote:
> This patch fixes this regression by making the rstat flushing
> conditional in the performance critical codepaths. More specifically,
> the kernel relies on the async periodic rstat flusher to flush the stats
> and only if the periodic flusher is delayed by more than twice the
> amount of its normal time window then the kernel allows rstat flushing
> from the performance critical codepaths.
I'm not sure whether your patch attempts to solve the problem of
(a) periodic flush getting stuck or (b) limiting error on refault path.
If it's (a), it should be tackled more systematically (dedicated wq?).
If it's (b), why not just rely on periodic flush (self answer: (1) and
(2) comparison is workload dependent).
> 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.
We can't argue what's the effect of periodic only flushing so this
newly introduced factor would inherit that too. I find it superfluous.
Michal
[1] This is worth looking at in more detail.
