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

[Yosry Ahmed <yosry@kernel.org>]

> Thinking some more about how stats are used... I think there's two
> categories the stats fall into: a) hot in-kernel ones (eg LRU sizes,
> workingset counters, dirty/writeback, zswap) which are read very
> frequently from the reclaim, refault, writeback and zswap paths and b)
> userspace-only ones (everything else) that are read through
> memory.stat / bpf / etc and much less often. The hot in-kernel ones
> are a small subset (~20 of ~80). Most of the stats fall into category
> b). However right now, a flush always aggregates *all* of these stats,
> even when the caller only needs / cares about the hot in-kernel ones.
> Flushing does all the work for the stats in category b), which the
> in-kernel flushers don't need. With the per-node lruvec copies, a
> flush goes through 100+ counters per cgroup per CPU vs ~30 for just
> the hot in-kernel ones (on a single-node machine).

I didn't measure this before, but I did tree adding a bitmask to check
which stats have updates and that actually made flushing more expensive.
It could have been the case that hot stats were distributed enough that
all the cachelines were being fetched anyway and the bitmask was just
added overhead.

I think you should do a simple experiment first about how much flushing
gets faster if you skip the stats not used by the kernel. It may or may
not matter, depends on prefetching (conversely, I wonder if explicitly
prefetching the entire stats array on each CPU would help).

I think it would matter less if we manage to completely get rid of root
flushes (except for userspace stat reads).

> 
> What do you think about splitting the stats into two rstat trees, one
> for each category? In-kernel flushers would then only flush the hot
> tree (~30 counters). memory.stat would still flush both (eg userspace
> would see the same thing as it sees today). Each tree would have its
> own lock and its own stats_updates threshold.  I can run some
> benchmarks but I think this would make flushes a lot cheaper, and
> since flushes would hold the lock for less time, it should also reduce
> the contention we see between concurrent flushers right now. Together
> with using node counters for root readers, hopefully this will make
> the non-ratelimited flush cheap enough for all non-root in-kernel
> readers and ideally let us get rid of the ratelimiting variant
> altogether.
> 
> Do you think something like this makes sense?

We have to consider the update side of things as well. How many code
paths will need to update both trees? That could be a problem.

From a code perspective, it would also be a bit annoying to maintain. I
would check if we can eliminate root flushes first tbh, then check any
other potential optimizations on top. Subtrees should me a lot smaller
than the entire tree.

> Separate from this but also kind of semi-related, I noticed
> css_rstat_flush() takes the rstat lock for every possible cpu, even
> cpus that have nothing to flush for the subtree. Maybe another
> improvement is to just first check locklessly whether there's any
> updates and then just skip the cpu if there aren't any. I can send a
> patch out for this.

Only the updated cgroups are on the update tree, not their ancestors,
not until the flush time. We used to put the ancestors on the tree on
the update side, but we don't anymore.

So we can only check the if the per-subsystem list is empty, not the
subtree specifically, right? Can we walk the per-subsystem list without
holding the lock or deleting first?

> 
> Thanks,
> Joanne