On 18/04/2024 04.21, Yosry Ahmed wrote:
On Tue, Apr 16, 2024 at 10:51 AM Jesper Dangaard Brouer <hawk@xxxxxxxxxx> wrote:
This patch aims to reduce userspace-triggered pressure on the global
cgroup_rstat_lock by introducing a mechanism to limit how often reading
stat files causes cgroup rstat flushing.
In the memory cgroup subsystem, memcg_vmstats_needs_flush() combined with
mem_cgroup_flush_stats_ratelimited() already limits pressure on the
global lock (cgroup_rstat_lock). As a result, reading memory-related stat
files (such as memory.stat, memory.numa_stat, zswap.current) is already
a less userspace-triggerable issue.
However, other userspace users of cgroup_rstat_flush(), such as when
reading io.stat (blk-cgroup.c) and cpu.stat, lack a similar system to
limit pressure on the global lock. Furthermore, userspace can easily
trigger this issue by reading those stat files.
Typically, normal userspace stats tools (e.g., cadvisor, nomad, systemd)
spawn threads that read io.stat, cpu.stat, and memory.stat (even from the
same cgroup) without realizing that on the kernel side, they share the
same global lock. This limitation also helps prevent malicious userspace
applications from harming the kernel by reading these stat files in a
tight loop.
To address this, the patch introduces cgroup_rstat_flush_ratelimited(),
similar to memcg's mem_cgroup_flush_stats_ratelimited().
Flushing occurs per cgroup (even though the lock remains global) a
variable named rstat_flush_last_time is introduced to track when a given
cgroup was last flushed. This variable, which contains the jiffies of the
flush, shares properties and a cache line with rstat_flush_next and is
updated simultaneously.
For cpu.stat, we need to acquire the lock (via cgroup_rstat_flush_hold)
because other data is read under the lock, but we skip the expensive
flushing if it occurred recently.
Regarding io.stat, there is an opportunity outside the lock to skip the
flush, but inside the lock, we must recheck to handle races.
Signed-off-by: Jesper Dangaard Brouer <hawk@xxxxxxxxxx>
As I mentioned in another thread, I really don't like time-based
rate-limiting [1]. Would it be possible to generalize the
magnitude-based rate-limiting instead? Have something like
memcg_vmstats_needs_flush() in the core rstat code?
I've taken a closer look at memcg_vmstats_needs_flush(). And I'm
concerned about overhead maintaining the stats (that is used as a filter).
static bool memcg_vmstats_needs_flush(struct memcg_vmstats *vmstats)
{
return atomic64_read(&vmstats->stats_updates) >
MEMCG_CHARGE_BATCH * num_online_cpus();
}
I looked at `vmstats->stats_updates` to see how often this is getting
updated. It is updated in memcg_rstat_updated(), but it gets inlined
into a number function (__mod_memcg_state, __mod_memcg_lruvec_state,
__count_memcg_events), plus it calls cgroup_rstat_updated().
Counting invocations per sec (via funccount):
10:28:09
FUNC COUNT
__mod_memcg_state 377553
__count_memcg_events 393078
__mod_memcg_lruvec_state 1229673
cgroup_rstat_updated 2632389
I'm surprised to see how many time per sec this is getting invoked.
Originating from memcg_rstat_updated() = 2,000,304 times per sec.
(On a 128 CPU core machine with 39% idle CPU-load.)
Maintaining these stats seems excessive...
Then how often does the filter lower pressure on lock:
MEMCG_CHARGE_BATCH(64) * 128 CPU = 8192
2000304/(64*128) = 244 time per sec (every ~4ms)
(assuming memcg_rstat_updated val=1)
Also, why do we keep the memcg time rate-limiting with this patch? Is
it because we use a much larger window there (2s)? Having two layers
of time-based rate-limiting is not ideal imo.
I'm also not-a-fan of having two layer of time-based rate-limiting, but
they do operate a different time scales *and* are not active at the same
time with current patch, if you noticed the details, then I excluded
memcg from using this as I commented "memcg have own ratelimit layer"
(in do_flush_stats).
I would prefer removing the memcg time rate-limiting and use this more
granular 50ms (20 timer/sec) for memcg also. And I was planning to do
that in a followup patchset. The 50ms (20 timer/sec) limit will be per
cgroup in the system, which then "scales"/increase with the number of
cgroups, but better than unbounded read/access locks per sec.
--Jesper
[1]https://lore.kernel.org/lkml/CAJD7tkYnSRwJTpXxSnGgo-i3-OdD7cdT-e3_S_yf7dSknPoRKw@xxxxxxxxxxxxxx/
sudo ./bcc/tools/funccount -Ti 1 -d 10
'__mod_memcg_state|__mod_memcg_lruvec_state|__count_memcg_events|cgroup_rstat_updated'
