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

[Yosry Ahmed]

[..]
> > > I now prefer (c) as well. I hadn't realized setting zswap.max is a
> > > rare path until Johannes mentioned it, and with your mention of
> > > zswap_load() sitting in the fault-critical path, I think the benchmark
> > > results show a clear improvement for c) over d) at higher depths.
> > >
> > > I was uneasy about the cost of c)'s reads scaling linearly with the #
> > > of cpus on a system (ie for_each_possible_cpu per level), but given
> > > the uncommonness of the zswap.max path, I think that's the right thing
> > > to trade away. Either way, it's a big improvement over the baseline a)
> > > path that currently exists anyways.
> > >
> > > > with more concurrency/CPUs and should generalize better to other
> > > > stats. But I am obviously biased :P
> > >
> > > I'm still investigating the writeback and vmscan cases. For writeback,
> > > using (c) is more complicated since NR_FILE_DIRTY and NR_WRITEBACK are
> > > node stats. I'm planning to spend time this week running benchmarks
> > > for it.
> > >
> > > If for those cases, (c) is viable, then I'll send a patch that adds
> > > (c) as general infrastructure. Otherwise, I'll send out (c) as a zswap
> > > specific patch.
> > >
> > > Does this sound good to everyone? If there are any objections, please
> > > let me know.
> >
> > 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.
(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.

>
> 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).

>
> > 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?